Merge pull request #1747 from gevent/issue1745

Handle .name on FileObject more like the stdlib.

docs/changes/1745.bugfix

@@ -0,0 +1,10 @@
+Make gevent ``FileObjects`` more closely match the semantics of native
+file objects for the ``name`` attribute:
+
+- Objects opened from a file descriptor integer have that integer as
+  their ``name.`` (Note that this is the Python 3 semantics; Python 2
+  native file objects returned from ``os.fdopen()`` have the string
+  "<fdopen>" as their name , but here gevent always follows Python 3.)
+- The ``name`` remains accessible after the file object is closed.
+
+Thanks to Dan Milon.

src/gevent/_ffi/loop.py

@@ -437,6 +437,8 @@ def _init_loop_and_aux_watchers(self, flags=None, default=None):
         self._init_callback_timer()
 
         self._threadsafe_async = self.async_(ref=False)
+        # No need to do anything with this on ``fork()``, both libev and libuv
+        # take care of creating a new pipe in their respective ``loop_fork()`` methods.
         self._threadsafe_async.start(lambda: None)
         # TODO: We may be able to do something nicer and use the existing python_callback
         # combined with onerror and the class check/timer/prepare to simplify things

src/gevent/_fileobjectcommon.py

@@ -428,6 +428,30 @@ def __wrapped(self, raw):
         return result
 
 
+class _ClosedIO(object):
+    # Used for FileObjectBase._io when FOB.close()
+    # is called. Lets us drop references to ``_io``
+    # for GC/resource cleanup reasons, but keeps some useful
+    # information around.
+    __slots__ = ('name',)
+
+    def __init__(self, io_obj):
+        try:
+            self.name = io_obj.name
+        except AttributeError:
+            pass
+
+    def __getattr__(self, name):
+        if name == 'name':
+            # We didn't set it in __init__ because there wasn't one
+            raise AttributeError
+        raise FileObjectClosed
+
+    def __bool__(self):
+        return False
+    __nonzero__ = __bool__
+
+
 class FileObjectBase(object):
     """
     Internal base class to ensure a level of consistency
@@ -472,7 +496,6 @@ def __init__(self, descriptor):
         # We don't actually use this property ourself, but we save it (and
         # pass it along) for compatibility.
         self._close = descriptor.closefd
-
         self._do_delegate_methods()
 
 
@@ -503,14 +526,14 @@ def _wrap_method(self, method):
     @property
     def closed(self):
         """True if the file is closed"""
-        return self._io is None
+        return isinstance(self._io, _ClosedIO)
 
     def close(self):
-        if self._io is None:
+        if isinstance(self._io, _ClosedIO):
             return
 
         fobj = self._io
-        self._io = None
+        self._io = _ClosedIO(self._io)
         try:
             self._do_close(fobj, self._close)
         finally:
@@ -525,8 +548,6 @@ def _do_close(self, fobj, closefd):
         raise NotImplementedError()
 
     def __getattr__(self, name):
-        if self._io is None:
-            raise FileObjectClosed()
         return getattr(self._io, name)
 
     def __repr__(self):
@@ -656,8 +677,6 @@ def close(_fobj=fobj):
 
     def _do_delegate_methods(self):
         FileObjectBase._do_delegate_methods(self)
-        # if not hasattr(self, 'read1') and 'r' in getattr(self._io, 'mode', ''):
-        #     self.read1 = self.read
         self.__io_holder[0] = self._io
 
     def _extra_repr(self):
@@ -676,7 +695,7 @@ def thread_method(*args, **kwargs):
                 # This is different than FileObjectPosix, etc,
                 # because we want to save the expensive trip through
                 # the threadpool.
-                raise FileObjectClosed()
+                raise FileObjectClosed
             with lock:
                 return threadpool.apply(method, args, kwargs)
 

src/gevent/_fileobjectposix.py

@@ -43,6 +43,7 @@ def __init__(self, fileno, open_descriptor, closefd=True):
 
         self._closefd = closefd
         self._fileno = fileno
+        self.name = fileno
         self.mode = open_descriptor.fileio_mode
         make_nonblocking(fileno)
         readable = open_descriptor.can_read
@@ -235,6 +236,11 @@ def _do_open_raw(self):
             fileno = raw.fileno()
             fileio = GreenFileDescriptorIO(fileno, self, closefd=closefd)
             fileio._keep_alive = raw
+            # We can usually do better for a name, though.
+            try:
+                fileio.name = raw.name
+            except AttributeError:
+                del fileio.name
         return fileio
 
     def _make_atomic_write(self, result, raw):

src/gevent/tests/test__fileobject.py

@@ -1,4 +1,5 @@
-from __future__ import print_function, absolute_import
+from __future__ import print_function
+from __future__ import absolute_import
 
 import functools
 import gc
@@ -51,7 +52,24 @@ def f(self):
         func(self)
     return f
 
-class TestFileObjectBlock(greentest.TestCase): # serves as a base for the concurrent tests too
+
+class CleanupMixin(object):
+    def _mkstemp(self, suffix):
+        fileno, path = tempfile.mkstemp(suffix)
+        self.addCleanup(os.remove, path)
+        self.addCleanup(close_fd_quietly, fileno)
+        return fileno, path
+
+    def _pipe(self):
+        r, w = os.pipe()
+        self.addCleanup(close_fd_quietly, r)
+        self.addCleanup(close_fd_quietly, w)
+        return r, w
+
+
+class TestFileObjectBlock(CleanupMixin,
+                          greentest.TestCase):
+    # serves as a base for the concurrent tests too
 
     WORKS_WITH_REGULAR_FILES = True
 
@@ -62,10 +80,7 @@ def _makeOne(self, *args, **kwargs):
         return self._getTargetClass()(*args, **kwargs)
 
     def _test_del(self, **kwargs):
-        r, w = os.pipe()
-        self.addCleanup(close_fd_quietly, r)
-        self.addCleanup(close_fd_quietly, w)
-
+        r, w = self._pipe()
         self._do_test_del((r, w), **kwargs)
 
     def _do_test_del(self, pipe, **kwargs):
@@ -104,10 +119,10 @@ def test_del(self):
     def test_del_close(self):
         self._test_del(close=True)
 
+
     @skipUnlessWorksWithRegularFiles
     def test_seek(self):
-        fileno, path = tempfile.mkstemp('.gevent.test__fileobject.test_seek')
-        self.addCleanup(os.remove, path)
+        fileno, path = self._mkstemp('.gevent.test__fileobject.test_seek')
 
         s = b'a' * 1024
         os.write(fileno, b'B' * 15)
@@ -139,8 +154,7 @@ def test_seek(self):
     def __check_native_matches(self, byte_data, open_mode,
                                meth='read', open_path=True,
                                **open_kwargs):
-        fileno, path = tempfile.mkstemp('.gevent_test_' + open_mode)
-        self.addCleanup(os.remove, path)
+        fileno, path = self._mkstemp('.gevent_test_' + open_mode)
 
         os.write(fileno, byte_data)
         os.close(fileno)
@@ -232,14 +246,75 @@ def test_close_pipe(self):
         x.close()
         y.close()
 
+    @skipUnlessWorksWithRegularFiles
+    @greentest.ignores_leakcheck
+    def test_name_after_close(self):
+        fileno, path = self._mkstemp('.gevent_test_named_path_after_close')
+
+        # Passing the fileno; the name is the same as the fileno, and
+        # doesn't change when closed.
+        f = self._makeOne(fileno)
+        nf = os.fdopen(fileno)
+        # On Python 2, os.fdopen() produces a name of <fdopen>;
+        # we follow the Python 3 semantics everywhere.
+        nf_name = '<fdopen>' if greentest.PY2 else fileno
+        self.assertEqual(f.name, fileno)
+        self.assertEqual(nf.name, nf_name)
+
+        # A file-like object that has no name; we'll close the
+        # `f` after this because we reuse the fileno, which
+        # gets passed to fcntl and so must still be valid
+        class Nameless(object):
+            def fileno(self):
+                return fileno
+            close = flush = isatty = closed = writable = lambda self: False
+            seekable = readable = lambda self: True
+
+        nameless = self._makeOne(Nameless(), 'rb')
+        with self.assertRaises(AttributeError):
+            getattr(nameless, 'name')
+        nameless.close()
+        with self.assertRaises(AttributeError):
+            getattr(nameless, 'name')
+
+        f.close()
+        try:
+            nf.close()
+        except (OSError, IOError):
+            # OSError: Py3, IOError: Py2
+            pass
+        self.assertEqual(f.name, fileno)
+        self.assertEqual(nf.name, nf_name)
+
+        def check(arg):
+            f = self._makeOne(arg)
+            self.assertEqual(f.name, path)
+            f.close()
+            # Doesn't change after closed.
+            self.assertEqual(f.name, path)
+
+        # Passing the string
+        check(path)
+
+        # Passing an opened native object
+        with open(path) as nf:
+            check(nf)
+
+        # An io object
+        with io.open(path) as nf:
+            check(nf)
+
+
+
+
 
 class ConcurrentFileObjectMixin(object):
     # Additional tests for fileobjects that cooperate
     # and we have full control of the implementation
 
     def test_read1_binary_present(self):
         # Issue #840
-        r, w = os.pipe()
+        r, w = self._pipe()
         reader = self._makeOne(r, 'rb')
         self._close_on_teardown(reader)
         writer = self._makeOne(w, 'w')
@@ -248,7 +323,7 @@ def test_read1_binary_present(self):
 
     def test_read1_text_not_present(self):
         # Only defined for binary.
-        r, w = os.pipe()
+        r, w = self._pipe()
         reader = self._makeOne(r, 'rt')
         self._close_on_teardown(reader)
         self.addCleanup(os.close, w)
@@ -257,15 +332,15 @@ def test_read1_text_not_present(self):
     def test_read1_default(self):
         # If just 'r' is given, whether it has one or not
         # depends on if we're Python 2 or 3.
-        r, w = os.pipe()
+        r, w = self._pipe()
         self.addCleanup(os.close, w)
         reader = self._makeOne(r)
         self._close_on_teardown(reader)
         self.assertEqual(PY2, hasattr(reader, 'read1'))
 
     def test_bufsize_0(self):
         # Issue #840
-        r, w = os.pipe()
+        r, w = self._pipe()
         x = self._makeOne(r, 'rb', bufsize=0)
         y = self._makeOne(w, 'wb', bufsize=0)
         self._close_on_teardown(x)
@@ -280,7 +355,7 @@ def test_bufsize_0(self):
 
     def test_newlines(self):
         import warnings
-        r, w = os.pipe()
+        r, w = self._pipe()
         lines = [b'line1\n', b'line2\r', b'line3\r\n', b'line4\r\nline5', b'\nline6']
         g = gevent.spawn(Writer, self._makeOne(w, 'wb'), lines)
 
@@ -296,7 +371,7 @@ def test_newlines(self):
             g.kill()
 
 
-class TestFileObjectThread(ConcurrentFileObjectMixin,
+class TestFileObjectThread(ConcurrentFileObjectMixin, # pylint:disable=too-many-ancestors
                            TestFileObjectBlock):
 
     def _getTargetClass(self):
@@ -329,7 +404,7 @@ def test_del_close(self):
     hasattr(fileobject, 'FileObjectPosix'),
     "Needs FileObjectPosix"
 )
-class TestFileObjectPosix(ConcurrentFileObjectMixin,
+class TestFileObjectPosix(ConcurrentFileObjectMixin, # pylint:disable=too-many-ancestors
                           TestFileObjectBlock):
 
     if sysinfo.LIBUV and sysinfo.LINUX:
@@ -345,10 +420,7 @@ def test_seek_raises_ioerror(self):
         # https://github.com/gevent/gevent/issues/1323
 
         # Get a non-seekable file descriptor
-        r, w = os.pipe()
-
-        self.addCleanup(close_fd_quietly, r)
-        self.addCleanup(close_fd_quietly, w)
+        r, _w = self._pipe()
 
         with self.assertRaises(OSError) as ctx:
             os.lseek(r, 0, os.SEEK_SET)
@@ -367,7 +439,7 @@ def test_seek_raises_ioerror(self):
         self.assertEqual(io_ex.args, os_ex.args)
         self.assertEqual(str(io_ex), str(os_ex))
 
-class TestTextMode(unittest.TestCase):
+class TestTextMode(CleanupMixin, unittest.TestCase):
 
     def test_default_mode_writes_linesep(self):
         # See https://github.com/gevent/gevent/issues/1282
@@ -376,9 +448,7 @@ def test_default_mode_writes_linesep(self):
         # First, make sure we initialize gevent
         gevent.get_hub()
 
-        fileno, path = tempfile.mkstemp('.gevent.test__fileobject.test_default')
-        self.addCleanup(os.remove, path)
-
+        fileno, path = self._mkstemp('.gevent.test__fileobject.test_default')
         os.close(fileno)
 
         with open(path, "w") as f:
@@ -389,7 +459,7 @@ def test_default_mode_writes_linesep(self):
 
         self.assertEqual(data, os.linesep.encode('ascii'))
 
-class TestOpenDescriptor(greentest.TestCase):
+class TestOpenDescriptor(CleanupMixin, greentest.TestCase):
 
     def _getTargetClass(self):
         return OpenDescriptor
@@ -421,14 +491,16 @@ def _check(self, regex, kind, *args, **kwargs):
     def test_atomicwrite_fd(self):
         from gevent._fileobjectcommon import WriteallMixin
         # It basically only does something when buffering is otherwise disabled
-        desc = self._makeOne(1, 'wb',
+        fileno, _w = self._pipe()
+        desc = self._makeOne(fileno, 'wb',
                              buffering=0,
                              closefd=False,
                              atomic_write=True)
         self.assertTrue(desc.atomic_write)
 
         fobj = desc.opened()
         self.assertIsInstance(fobj, WriteallMixin)
+        os.close(fileno)
 
 def pop():
     for regex, kind, kwargs in TestOpenDescriptor.CASES: