Patchwork [STABLE] posix: insert seek between reads and writes on Solaris (issue4943)

login
register
mail settings
Submitter Gregory Szorc
Date Nov. 15, 2015, 10:21 p.m.
Message ID <52360879f54f28b774be.1447626072@7.1.168.192.in-addr.arpa>
Download mbox | patch
Permalink /patch/11407/
State Changes Requested
Delegated to: Pierre-Yves David
Headers show

Comments

Gregory Szorc - Nov. 15, 2015, 10:21 p.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1447625312 28800
#      Sun Nov 15 14:08:32 2015 -0800
# Branch stable
# Node ID 52360879f54f28b774be7fc8f7e207768d5851b6
# Parent  8a256cee72c890c761918ec1d244b286694ac51b
posix: insert seek between reads and writes on Solaris (issue4943)

At least some versions of Solaris fail to properly write to file
descriptors opened for both reading and writing. When the descriptor
is seeked and read, subsequent writes effectively disappear into a
black hole.

The underlying cause of this is unknown. Some versions of Solaris
appear to work. Others don't. Read the issue comments for more
details.

The issue can be worked around by inserting an extra seek between
read and write operations. This exact workaround is already in place
for Windows, where the MSDN docs call out this requirement. See
3686fa2b8eee.

This patch copies the existing workaround code into posix.py and
makes it active for Solaris only.

Yes, we copied a large block of source code. This is not ideal.
However, posix.py and windows.py are very low level modules. There
is no obvious existing module we could add the function to without
introducing an import cycle. A new module will likely need to be
invented. I'm not comfortable doing that on the stable branch.
Once this patch is merged into @, we should clean up the duplicated
code.
Adrian Buehlmann - Nov. 15, 2015, 11:34 p.m.
On 2015-11-15 23:21, Gregory Szorc wrote:
> Yes, we copied a large block of source code. This is not ideal.
> However, posix.py and windows.py are very low level modules. There
> is no obvious existing module we could add the function to without
> introducing an import cycle. A new module will likely need to be
> invented. I'm not comfortable doing that on the stable branch.
> Once this patch is merged into @, we should clean up the duplicated
> code.

Perhaps you could move mixedfilemodewrapper to util.py and do the
wrapping there for the Solaris and Windows cases.

Likely not appropriate for stable indeed...
Gregory Szorc - Nov. 27, 2015, 2:13 a.m.
On Sun, Nov 15, 2015 at 2:21 PM, Gregory Szorc <gregory.szorc@gmail.com>
wrote:

> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1447625312 28800
> #      Sun Nov 15 14:08:32 2015 -0800
> # Branch stable
> # Node ID 52360879f54f28b774be7fc8f7e207768d5851b6
> # Parent  8a256cee72c890c761918ec1d244b286694ac51b
> posix: insert seek between reads and writes on Solaris (issue4943)
>
> This (conservative) patch was not acted on. I just patchbombed basically
Adrian's counter-proposal as a V2 in case someone wants to queue the more
liberal version. Either way, we probably want something in 3.6.2 to unbust
Solaris.

Patch

diff --git a/mercurial/posix.py b/mercurial/posix.py
--- a/mercurial/posix.py
+++ b/mercurial/posix.py
@@ -11,8 +11,9 @@  import errno
 import fcntl
 import getpass
 import grp
 import os
+import platform
 import pwd
 import re
 import select
 import socket
@@ -25,9 +26,8 @@  from .i18n import _
 from . import (
     encoding,
 )
 
-posixfile = open
 normpath = os.path.normpath
 samestat = os.path.samestat
 oslink = os.link
 unlink = os.unlink
@@ -37,8 +37,86 @@  expandglobs = False
 
 umask = os.umask(0)
 os.umask(umask)
 
+# Some Solaris versions don't properly handle reads() + writes() on the same
+# file descriptor. When running on Solaris, insert an extra seek() between
+# reads and writes on dual mode file descriptors to prevent this bug.
+# See also issue4943.
+if platform.system() == 'SunOS':
+    # TODO consolidate with duplicate code in windows.py.
+    class mixedfilemodewrapper(object):
+        """Insert seek between reads and writes.
+
+        This class wraps posixfile instances when the file is opened in
+        read/write mode and automatically adds checks or inserts appropriate
+        file positioning calls when necessary.
+        """
+        OPNONE = 0
+        OPREAD = 1
+        OPWRITE = 2
+
+        def __init__(self, fp):
+            object.__setattr__(self, '_fp', fp)
+            object.__setattr__(self, '_lastop', 0)
+
+        def __getattr__(self, name):
+            return getattr(self._fp, name)
+
+        def __setattr__(self, name, value):
+            return self._fp.__setattr__(name, value)
+
+        def _noopseek(self):
+            self._fp.seek(0, os.SEEK_CUR)
+
+        def seek(self, *args, **kwargs):
+            object.__setattr__(self, '_lastop', self.OPNONE)
+            return self._fp.seek(*args, **kwargs)
+
+        def write(self, d):
+            if self._lastop == self.OPREAD:
+                self._noopseek()
+
+            object.__setattr__(self, '_lastop', self.OPWRITE)
+            return self._fp.write(d)
+
+        def writelines(self, *args, **kwargs):
+            if self._lastop == self.OPREAD:
+                self._noopeseek()
+
+            object.__setattr__(self, '_lastop', self.OPWRITE)
+            return self._fp.writelines(*args, **kwargs)
+
+        def read(self, *args, **kwargs):
+            if self._lastop == self.OPWRITE:
+                self._noopseek()
+
+            object.__setattr__(self, '_lastop', self.OPREAD)
+            return self._fp.read(*args, **kwargs)
+
+        def readline(self, *args, **kwargs):
+            if self._lastop == self.OPWRITE:
+                self._noopseek()
+
+            object.__setattr__(self, '_lastop', self.OPREAD)
+            return self._fp.readline(*args, **kwargs)
+
+        def readlines(self, *args, **kwargs):
+            if self._lastop == self.OPWRITE:
+                self._noopseek()
+
+            object.__setattr__(self, '_lastop', self.OPREAD)
+            return self._fp.readlines(*args, **kwargs)
+
+    def posixfile(name, mode='r', buffering=-1):
+        fp = open(name, mode, buffering)
+        if '+' in mode:
+            return mixedfilemodewrapper(fp)
+
+        return fp
+else:
+    posixfile = open
+
 def split(p):
     '''Same as posixpath.split, but faster
 
     >>> import posixpath
diff --git a/mercurial/windows.py b/mercurial/windows.py
--- a/mercurial/windows.py
+++ b/mercurial/windows.py
@@ -26,8 +26,9 @@  testpid = win32.testpid
 unlink = win32.unlink
 
 umask = 0o022
 
+# TODO consolidate with duplicate code in posix.py.
 class mixedfilemodewrapper(object):
     """Wraps a file handle when it is opened in read/write mode.
 
     fopen() and fdopen() on Windows have a specific-to-Windows requirement