Merge pull request #1485 from gevent/issue1441

More simplification and unification of the file objects

CHANGES.rst

@@ -15,7 +15,8 @@
   consistently text and binary modes. If neither 'b' nor 't' is given
   in the mode, they will read and write native strings. If 't' is
   given, they will always work with unicode strings, and 'b' will
-  always work with byte strings. See :issue:`1441`.
+  always work with byte strings. (FileObjectPosix already worked this
+  way.) See :issue:`1441`.
 
 - The file objects accept *encoding*, *errors* and *newline*
   arguments. On Python 2, these are only used if 't' is in the mode.
@@ -24,6 +25,10 @@
   ``r``, for consistency with the other file objects and the standard
   ``open`` and ``io.open`` functions.
 
+- Fix ``FilObjectPosix`` improperly being used from multiple
+  greenlets. Previously this was hidden by forcing buffering, which
+  raised ``RuntimeError``.
+
 
 1.5a2 (2019-10-21)
 ==================

src/gevent/_socket3.py

@@ -261,7 +261,9 @@ class socket(object):
         except the only mode characters supported are 'r', 'w' and 'b'.
         The semantics are similar too.
         """
-        # (XXX refactor to share code?)
+        # XXX refactor to share code? We ought to be able to use our FileObject,
+        # adding the appropriate amount of refcounting. At the very least we can use our
+        # OpenDescriptor to handle the parsing.
         for c in mode:
             if c not in {"r", "w", "b"}:
                 raise ValueError("invalid mode %r (only r, w, b allowed)")

src/gevent/_sslgte279.py

@@ -56,14 +56,16 @@ if 'namedtuple' in __all__:
 
 # See notes in _socket2.py. Python 3 returns much nicer
 # `io` object wrapped around a SocketIO class.
-assert not hasattr(__ssl__._fileobject, '__enter__') # pylint:disable=no-member
+if hasattr(__ssl__, '_fileobject'):
+    assert not hasattr(__ssl__._fileobject, '__enter__') # pylint:disable=no-member
 
-class _fileobject(__ssl__._fileobject): # pylint:disable=no-member
+class _fileobject(getattr(__ssl__, '_fileobject', object)): # pylint:disable=no-member
 
     def __enter__(self):
         return self
 
     def __exit__(self, *args):
+        # pylint:disable=no-member
         if not self.closed:
             self.close()
 
@@ -96,14 +98,14 @@ def create_default_context(purpose=Purpose.SERVER_AUTH, cafile=None,
     context = SSLContext(PROTOCOL_SSLv23)
 
     # SSLv2 considered harmful.
-    context.options |= OP_NO_SSLv2
+    context.options |= OP_NO_SSLv2 # pylint:disable=no-member
 
     # SSLv3 has problematic security and is only required for really old
     # clients such as IE6 on Windows XP
-    context.options |= OP_NO_SSLv3
+    context.options |= OP_NO_SSLv3 # pylint:disable=no-member
 
     # disable compression to prevent CRIME attacks (OpenSSL 1.0+)
-    context.options |= getattr(_ssl, "OP_NO_COMPRESSION", 0)
+    context.options |= getattr(_ssl, "OP_NO_COMPRESSION", 0) # pylint:disable=no-member
 
     if purpose == Purpose.SERVER_AUTH:
         # verify certs and host name in client mode
@@ -112,11 +114,11 @@ def create_default_context(purpose=Purpose.SERVER_AUTH, cafile=None,
     elif purpose == Purpose.CLIENT_AUTH:
         # Prefer the server's ciphers by default so that we get stronger
         # encryption
-        context.options |= getattr(_ssl, "OP_CIPHER_SERVER_PREFERENCE", 0)
+        context.options |= getattr(_ssl, "OP_CIPHER_SERVER_PREFERENCE", 0) # pylint:disable=no-member
 
         # Use single use keys in order to improve forward secrecy
-        context.options |= getattr(_ssl, "OP_SINGLE_DH_USE", 0)
-        context.options |= getattr(_ssl, "OP_SINGLE_ECDH_USE", 0)
+        context.options |= getattr(_ssl, "OP_SINGLE_DH_USE", 0) # pylint:disable=no-member
+        context.options |= getattr(_ssl, "OP_SINGLE_ECDH_USE", 0) # pylint:disable=no-member
 
         # disallow ciphers with known vulnerabilities
         context.set_ciphers(_RESTRICTED_SERVER_CIPHERS)
@@ -146,10 +148,10 @@ def _create_unverified_context(protocol=PROTOCOL_SSLv23, cert_reqs=None,
 
     context = SSLContext(protocol)
     # SSLv2 considered harmful.
-    context.options |= OP_NO_SSLv2
+    context.options |= OP_NO_SSLv2 # pylint:disable=no-member
     # SSLv3 has problematic security and is only required for really old
     # clients such as IE6 on Windows XP
-    context.options |= OP_NO_SSLv3
+    context.options |= OP_NO_SSLv3 # pylint:disable=no-member
 
     if cert_reqs is not None:
         context.verify_mode = cert_reqs

src/gevent/fileobject.py

 """
 Wrappers to make file-like objects cooperative.
 
-.. class:: FileObject
+.. class:: FileObject(fobj, mode='r', buffering=-1, closefd=True, encoding=None, errors=None, newline=None)
 
-   The main entry point to the file-like gevent-compatible behaviour. It will be defined
-   to be the best available implementation.
+    The main entry point to the file-like gevent-compatible behaviour. It
+    will be defined to be the best available implementation.
+
+    All the parameters are as for :func:`io.open`.
+
+    :param fobj: Usually a file descriptor of a socket. Can also be
+        another object with a ``fileno()`` method, or an object that can
+        be passed to ``io.open()`` (e.g., a file system path). If the object
+        is not a socket, the results will vary based on the platform and the
+        type of object being opened.
+
+        All supported versions of Python allow :class:`os.PathLike` objects.
+
+    .. versionchanged:: 1.5
+       Accept str and ``PathLike`` objects for *fobj* on all versions of Python.
+    .. versionchanged:: 1.5
+       Add *encoding*, *errors* and *newline* arguments.
+    .. versionchanged:: 1.5
+       Accept *closefd* and *buffering* instead of *close* and *bufsize* arguments.
+       The latter remain for backwards compatibility.
 
 There are two main implementations of ``FileObject``. On all systems,
 there is :class:`FileObjectThread` which uses the built-in native
@@ -12,9 +30,11 @@ threadpool to avoid blocking the entire interpreter. On UNIX systems
 (those that support the :mod:`fcntl` module), there is also
 :class:`FileObjectPosix` which uses native non-blocking semantics.
 
-A third class, :class:`FileObjectBlock`, is simply a wrapper that executes everything
-synchronously (and so is not gevent-compatible). It is provided for testing and debugging
-purposes.
+A third class, :class:`FileObjectBlock`, is simply a wrapper that
+executes everything synchronously (and so is not gevent-compatible).
+It is provided for testing and debugging purposes.
+
+All classes have the same signature; some may accept extra keyword arguments.
 
 Configuration
 =============
@@ -27,8 +47,10 @@ You may also set it to the fully qualified class name of another
 object that implements the file interface to use one of your own
 objects.
 
-.. note:: The environment variable must be set at the time this module
-   is first imported.
+.. note::
+
+    The environment variable must be set at the time this module
+    is first imported.
 
 Classes
 =======
@@ -58,4 +80,6 @@ from gevent._fileobjectcommon import FileObjectBlock
 
 # None of the possible objects can live in this module because
 # we would get an import cycle and the config couldn't be set from code.
+# TODO: zope.hookable would be great for allowing this to be imported
+# without requiring configuration but still being very fast.
 FileObject = config.fileobject

src/gevent/subprocess.py

@@ -37,7 +37,9 @@ import os
 import signal
 import sys
 import traceback
+
 from gevent.event import AsyncResult
+from gevent.exceptions import ConcurrentObjectUseError
 from gevent.hub import _get_hub_noargs as get_hub
 from gevent.hub import linkproxy
 from gevent.hub import sleep
@@ -428,7 +430,7 @@ else:
     _set_inheritable = lambda i, v: True
 
 
-def FileObject(*args):
+def FileObject(*args, **kwargs):
     # Defer importing FileObject until we need it
     # to allow it to be configured more easily.
     from gevent.fileobject import FileObject as _FileObject
@@ -611,26 +613,20 @@ class Popen(object):
             if PY3 and text_mode:
                 # Under Python 3, if we left on the 'b' we'd get different results
                 # depending on whether we used FileObjectPosix or FileObjectThread
-                self.stdin = FileObject(p2cwrite, 'wb', bufsize)
-                self.stdin.translate_newlines(None,
-                                              write_through=True,
-                                              line_buffering=(bufsize == 1),
-                                              encoding=self.encoding, errors=self.errors)
+                self.stdin = FileObject(p2cwrite, 'w', bufsize,
+                                        encoding=self.encoding, errors=self.errors)
             else:
                 self.stdin = FileObject(p2cwrite, 'wb', bufsize)
         if c2pread != -1:
             if universal_newlines or text_mode:
                 if PY3:
-                    # FileObjectThread doesn't support the 'U' qualifier
-                    # with a bufsize of 0
-                    self.stdout = FileObject(c2pread, 'rb', bufsize)
+                    self.stdout = FileObject(c2pread, 'r', bufsize,
+                                             encoding=self.encoding, errors=self.errors)
                     # NOTE: Universal Newlines are broken on Windows/Py3, at least
                     # in some cases. This is true in the stdlib subprocess module
                     # as well; the following line would fix the test cases in
                     # test__subprocess.py that depend on python_universal_newlines,
                     # but would be inconsistent with the stdlib:
-                    #msvcrt.setmode(self.stdout.fileno(), os.O_TEXT)
-                    self.stdout.translate_newlines('r', encoding=self.encoding, errors=self.errors)
                 else:
                     self.stdout = FileObject(c2pread, 'rU', bufsize)
             else:
@@ -638,8 +634,8 @@ class Popen(object):
         if errread != -1:
             if universal_newlines or text_mode:
                 if PY3:
-                    self.stderr = FileObject(errread, 'rb', bufsize)
-                    self.stderr.translate_newlines(None, encoding=encoding, errors=errors)
+                    self.stderr = FileObject(errread, 'r', bufsize,
+                                             encoding=encoding, errors=errors)
                 else:
                     self.stderr = FileObject(errread, 'rU', bufsize)
             else:
@@ -756,7 +752,13 @@ class Popen(object):
             def _read():
                 try:
                     data = pipe.read()
-                except RuntimeError:
+                except (
+                        # io.Buffered* can raise RuntimeError: 'reentrant call'
+                        RuntimeError,
+                        # unbuffered Posix IO that we're already waiting on
+                        # can raise this. Closing the pipe will free those greenlets up.
+                        ConcurrentObjectUseError
+                ):
                     return
                 if not data:
                     return

src/gevent/testing/sysinfo.py

@@ -24,9 +24,13 @@ import sys
 import gevent.core
 from gevent import _compat as gsysinfo
 
+VERBOSE = sys.argv.count('-v') > 1
+
+# Python implementations
 PYPY = gsysinfo.PYPY
 CPYTHON = not PYPY
-VERBOSE = sys.argv.count('-v') > 1
+
+# Platform/operating system
 WIN = gsysinfo.WIN
 LINUX = gsysinfo.LINUX
 OSX = gsysinfo.OSX

src/gevent/tests/test__fileobject.py

 from __future__ import print_function
+
+import functools
+import gc
+import io
 import os
 import sys
 import tempfile
-import gc
 import unittest
 
 import gevent
 from gevent import fileobject
+from gevent._fileobjectcommon import OpenDescriptor
+
 from gevent._compat import PY2
+from gevent._compat import PY3
+from gevent._compat import text_type
 
 import gevent.testing as greentest
-from gevent.testing.sysinfo import PY3
-from gevent.testing.flaky import reraiseFlakyTestRaceConditionLibuv
-from gevent.testing.skipping import skipOnLibuvOnCIOnPyPy
-from gevent.testing.skipping import skipOnLibuv
+from gevent.testing import sysinfo
 
 try:
     ResourceWarning
@@ -35,8 +39,18 @@ def close_fd_quietly(fd):
     except (IOError, OSError):
         pass
 
+def skipUnlessWorksWithRegularFiles(func):
+    @functools.wraps(func)
+    def f(self):
+        if not self.WORKS_WITH_REGULAR_FILES:
+            self.skipTest("Doesn't work with regular files")
+        func(self)
+    return f
+
 class TestFileObjectBlock(greentest.TestCase): # serves as a base for the concurrent tests too
 
+    WORKS_WITH_REGULAR_FILES = True
+
     def _getTargetClass(self):
         return fileobject.FileObjectBlock
 
@@ -86,8 +100,7 @@ class TestFileObjectBlock(greentest.TestCase): # serves as a base for the concur
     def test_del_close(self):
         self._test_del(close=True)
 
-    @skipOnLibuvOnCIOnPyPy("This appears to crash on libuv/pypy/travis.")
-    # No idea why, can't duplicate locally.
+    @skipUnlessWorksWithRegularFiles
     def test_seek(self):
         fileno, path = tempfile.mkstemp('.gevent.test__fileobject.test_seek')
         self.addCleanup(os.remove, path)
@@ -102,16 +115,7 @@ class TestFileObjectBlock(greentest.TestCase): # serves as a base for the concur
             native_data = f.read(1024)
 
         with open(path, 'rb') as f_raw:
-            try:
-                f = self._makeOne(f_raw, 'rb', close=False)
-            except ValueError:
-                # libuv on Travis can raise EPERM
-                # from FileObjectPosix. I can't produce it on mac os locally,
-                # don't know what the issue is. This started happening on Jan 19,
-                # in the branch that caused all watchers to be explicitly closed.
-                # That shouldn't have any effect on io watchers, though, which were
-                # already being explicitly closed.
-                reraiseFlakyTestRaceConditionLibuv()
+            f = self._makeOne(f_raw, 'rb', close=False)
 
             if PY3 or hasattr(f, 'seekable'):
                 # On Python 3, all objects should have seekable.
@@ -128,33 +132,66 @@ class TestFileObjectBlock(greentest.TestCase): # serves as a base for the concur
         self.assertEqual(native_data, s)
         self.assertEqual(native_data, fileobj_data)
 
-    def test_str_default_to_native(self):
-        # With no 'b' or 't' given, read and write native str.
-        fileno, path = tempfile.mkstemp('.gevent_test_str_default')
+    def __check_native_matches(self, byte_data, open_mode,
+                               meth='read', **open_kwargs):
+        fileno, path = tempfile.mkstemp('.gevent_test_' + open_mode)
         self.addCleanup(os.remove, path)
 
-        os.write(fileno, b'abcdefg')
+        os.write(fileno, byte_data)
         os.close(fileno)
 
-        with open(path, 'r') as f:
-            native_data = f.read()
+        with io.open(path, open_mode, **open_kwargs) as f:
+            native_data = getattr(f, meth)()
 
-        with self._makeOne(path, 'r') as f:
-            gevent_data = f.read()
+        with self._makeOne(path, open_mode, **open_kwargs) as f:
+            gevent_data = getattr(f, meth)()
 
         self.assertEqual(native_data, gevent_data)
+        return gevent_data
 
-    def test_text_encoding(self):
-        fileno, path = tempfile.mkstemp('.gevent_test_str_default')
-        self.addCleanup(os.remove, path)
+    @skipUnlessWorksWithRegularFiles
+    def test_str_default_to_native(self):
+        # With no 'b' or 't' given, read and write native str.
+        gevent_data = self.__check_native_matches(b'abcdefg', 'r')
+        self.assertIsInstance(gevent_data, str)
 
-        os.write(fileno, u'\N{SNOWMAN}'.encode('utf-8'))
-        os.close(fileno)
+    @skipUnlessWorksWithRegularFiles
+    def test_text_encoding(self):
+        gevent_data = self.__check_native_matches(
+            u'\N{SNOWMAN}'.encode('utf-8'),
+            'r+',
+            buffering=5, encoding='utf-8'
+        )
+        self.assertIsInstance(gevent_data, text_type)
+
+    @skipUnlessWorksWithRegularFiles
+    def test_does_not_leak_on_exception(self):
+        # If an exception occurs during opening,
+        # everything still gets cleaned up.
+        pass
 
-        with self._makeOne(path, 'r+', bufsize=5, encoding='utf-8') as f:
-            gevent_data = f.read()
+    @skipUnlessWorksWithRegularFiles
+    def test_rbU_produces_bytes(self):
+        # Including U in rb still produces bytes.
+        # Note that the universal newline behaviour is
+        # essentially ignored in explicit bytes mode.
+        gevent_data = self.__check_native_matches(
+            b'line1\nline2\r\nline3\rlastline\n\n',
+            'rbU',
+            meth='readlines',
+        )
+        self.assertIsInstance(gevent_data[0], bytes)
+        self.assertEqual(len(gevent_data), 4)
+
+    @skipUnlessWorksWithRegularFiles
+    def test_rU_produces_native(self):
+        gevent_data = self.__check_native_matches(
+            b'line1\nline2\r\nline3\rlastline\n\n',
+            'rU',
+            meth='readlines',
+        )
+        self.assertIsInstance(gevent_data[0], str)
 
-        self.assertEqual(u'\N{SNOWMAN}', gevent_data)
 
     def test_close_pipe(self):
         # Issue #190, 203
@@ -264,6 +301,12 @@ class TestFileObjectThread(ConcurrentFileObjectMixin,
 class TestFileObjectPosix(ConcurrentFileObjectMixin,
                           TestFileObjectBlock):
 
+    if sysinfo.LIBUV and sysinfo.LINUX:
+        # On Linux, initializing the watcher for a regular
+        # file results in libuv raising EPERM. But that works
+        # fine on other platforms.
+        WORKS_WITH_REGULAR_FILES = False
+
     def _getTargetClass(self):
         return fileobject.FileObjectPosix
 
@@ -293,14 +336,6 @@ class TestFileObjectPosix(ConcurrentFileObjectMixin,
         self.assertEqual(io_ex.args, os_ex.args)
         self.assertEqual(str(io_ex), str(os_ex))
 
-    @skipOnLibuv("libuv on linux raises EPERM ") # but works fine on macOS
-    def test_str_default_to_native(self):
-        TestFileObjectBlock.test_str_default_to_native(self)
-
-    @skipOnLibuv("libuv in linux raises EPERM")
-    def test_text_encoding(self):
-        TestFileObjectBlock.test_text_encoding(self)
-
 class TestTextMode(unittest.TestCase):
 
     def test_default_mode_writes_linesep(self):
@@ -323,6 +358,39 @@ class TestTextMode(unittest.TestCase):
 
         self.assertEqual(data, os.linesep.encode('ascii'))
 
+class TestOpenDescriptor(greentest.TestCase):
+
+    def _makeOne(self, *args, **kwargs):
+        return OpenDescriptor(*args, **kwargs)
+
+    def _check(self, regex, kind, *args, **kwargs):
+        with self.assertRaisesRegex(kind, regex):
+            self._makeOne(*args, **kwargs)
+
+    case = lambda re, **kwargs: (re, TypeError, kwargs)
+    vase = lambda re, **kwargs: (re, ValueError, kwargs)
+    CASES = (
+        case('mode', mode=42),
+        case('buffering', buffering='nope'),
+        case('encoding', encoding=42),
+        case('errors', errors=42),
+        vase('mode', mode='aoeug'),
+        vase('mode U cannot be combined', mode='wU'),
+        vase('text and binary', mode='rtb'),
+        vase('append mode at once', mode='rw'),
+        vase('exactly one', mode='+'),
+        vase('take an encoding', mode='rb', encoding='ascii'),
+        vase('take an errors', mode='rb', errors='strict'),
+        vase('take a newline', mode='rb', newline='\n'),
+    )
+
+def pop():
+    for regex, kind, kwargs in TestOpenDescriptor.CASES:
+        setattr(
+            TestOpenDescriptor, 'test_' + regex,
+            lambda self, _re=regex, _kind=kind, _kw=kwargs: self._check(_re, _kind, 1, **_kw)
+        )
+pop()
 
 
 if __name__ == '__main__':

src/gevent/tests/test__monkey_queue.py

@@ -57,7 +57,7 @@ class BlockingTestMixin(object):
             self.fail("blocking function '%r' appeared not to block" %
                       block_func)
         self.t.join(10) # make sure the thread terminates
-        if self.t.isAlive():
+        if self.t.is_alive():
             self.fail("trigger function '%r' appeared to not return" %
                       trigger_func)
         return self.result
@@ -72,7 +72,7 @@ class BlockingTestMixin(object):
                 block_func(*block_args)
         finally:
             self.t.join(10) # make sure the thread terminates
-            if self.t.isAlive():
+            if self.t.is_alive():
                 self.fail("trigger function '%r' appeared to not return" %
                           trigger_func)
             if not self.t.startedEvent.isSet():