Rewrite fileobject core to handle always create FileIO if needed, not just on Py2.

Keeping control this way simplifies and unifies the implementations.

- Fix a concurrency bug in FileObjectPosix that we were hiding with buffering and a RuntimeError.
- Add all the error checking the stdlib does, with a few exceptions that make sense.

See #1441.

CHANGES.rst

@@ -24,6 +24,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/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():