From 0838e93dca02b39d08f78b0fc5896c934cfec7cd Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Fri, 15 Jan 2021 14:23:55 -0600 Subject: [PATCH 1/2] Handle .name on FileObject more like the stdlib. Fixes #1745 --- docs/changes/1745.bugfix | 10 +++ src/gevent/_ffi/loop.py | 2 + src/gevent/_fileobjectcommon.py | 37 ++++++-- src/gevent/_fileobjectposix.py | 6 ++ src/gevent/tests/test__fileobject.py | 126 +++++++++++++++++++++------ 5 files changed, 145 insertions(+), 36 deletions(-) create mode 100644 docs/changes/1745.bugfix diff --git a/docs/changes/1745.bugfix b/docs/changes/1745.bugfix new file mode 100644 index 000000000..4ee68b708 --- /dev/null +++ b/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 + "" as their name , but here gevent always follows Python 3.) +- The ``name`` remains accessible after the file object is closed. + +Thanks to Dan Milon. diff --git a/src/gevent/_ffi/loop.py b/src/gevent/_ffi/loop.py index 8dd3e8c06..b775dd6ff 100644 --- a/src/gevent/_ffi/loop.py +++ b/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 diff --git a/src/gevent/_fileobjectcommon.py b/src/gevent/_fileobjectcommon.py index 11b1ea34c..9dab90b51 100644 --- a/src/gevent/_fileobjectcommon.py +++ b/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) diff --git a/src/gevent/_fileobjectposix.py b/src/gevent/_fileobjectposix.py index b1e966d43..bfdf7895a 100644 --- a/src/gevent/_fileobjectposix.py +++ b/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): diff --git a/src/gevent/tests/test__fileobject.py b/src/gevent/tests/test__fileobject.py index a85625b86..7024c9f11 100644 --- a/src/gevent/tests/test__fileobject.py +++ b/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,6 +246,67 @@ 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 ; + # we follow the Python 3 semantics everywhere. + nf_name = '' 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 @@ -239,7 +314,7 @@ class ConcurrentFileObjectMixin(object): 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,7 +332,7 @@ 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) @@ -265,7 +340,7 @@ def test_read1_default(self): 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,7 +491,8 @@ 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, _path = self._mkstemp('.gevent_test_atomicwrite_fd') + desc = self._makeOne(fileno, 'wb', buffering=0, closefd=False, atomic_write=True) @@ -429,6 +500,7 @@ def test_atomicwrite_fd(self): fobj = desc.opened() self.assertIsInstance(fobj, WriteallMixin) + os.close(fileno) def pop(): for regex, kind, kwargs in TestOpenDescriptor.CASES: From 28975160488af7f37564211f54ca6c61fc3f5d72 Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Fri, 15 Jan 2021 15:18:49 -0600 Subject: [PATCH 2/2] Whoops, libuv can't handle normal files on Linux. --- src/gevent/tests/test__fileobject.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/gevent/tests/test__fileobject.py b/src/gevent/tests/test__fileobject.py index 7024c9f11..f506dfa73 100644 --- a/src/gevent/tests/test__fileobject.py +++ b/src/gevent/tests/test__fileobject.py @@ -491,7 +491,7 @@ 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 - fileno, _path = self._mkstemp('.gevent_test_atomicwrite_fd') + fileno, _w = self._pipe() desc = self._makeOne(fileno, 'wb', buffering=0, closefd=False,