From 30a66aeb64894ffc9b403e636c0482b1eb9d3208 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Wed, 22 May 2019 12:59:07 +0200 Subject: [PATCH 1/3] bpo-18748: _pyio.IOBase emits unraisable exception In development (-X dev) mode and in a debug build, IOBase finalizer of the _pyio module now logs the exception if the close() method fails. The exception is ignored silently by default in release build. test_io: test_error_through_destructor() now uses support.catch_unraisable_exception() rather than capturing stderr. --- Doc/whatsnew/3.8.rst | 9 +++++++ Lib/_pyio.py | 23 ++++++++++------ Lib/test/test_io.py | 62 +++++++++++++++++++------------------------- 3 files changed, 51 insertions(+), 43 deletions(-) diff --git a/Doc/whatsnew/3.8.rst b/Doc/whatsnew/3.8.rst index 68843e2d9ed157..5ec8bf60eb4faf 100644 --- a/Doc/whatsnew/3.8.rst +++ b/Doc/whatsnew/3.8.rst @@ -300,6 +300,15 @@ for :func:`property`, :func:`classmethod`, and :func:`staticmethod`:: self.bit_rate = round(bit_rate / 1000.0, 1) self.duration = ceil(duration) +io +-- + +In development mode (:opt:`-X` ``env``) and in debug build, the +:class:`io.IOBase` finalizer now logs the exception if the ``close()`` method +fails. The exception is ignored silently by default in release build. +(Contributed by Victor Stinner in :issue:`18748`.) + + gc -- diff --git a/Lib/_pyio.py b/Lib/_pyio.py index af2ce30c278062..be5e4266daffd8 100644 --- a/Lib/_pyio.py +++ b/Lib/_pyio.py @@ -33,6 +33,10 @@ # Rebind for compatibility BlockingIOError = BlockingIOError +# Does io.IOBase finalizer log the exception if the close() method fails? +# The exception is ignored silently by default in release build. +_IOBASE_EMITS_UNRAISABLE = (hasattr(sys, "gettotalrefcount") or sys.flags.dev_mode) + def open(file, mode="r", buffering=-1, encoding=None, errors=None, newline=None, closefd=True, opener=None): @@ -378,15 +382,18 @@ def close(self): def __del__(self): """Destructor. Calls close().""" - # The try/except block is in case this is called at program - # exit time, when it's possible that globals have already been - # deleted, and then the close() call might fail. Since - # there's nothing we can do about such failures and they annoy - # the end users, we suppress the traceback. - try: + if _IOBASE_EMITS_UNRAISABLE: self.close() - except: - pass + else: + # The try/except block is in case this is called at program + # exit time, when it's possible that globals have already been + # deleted, and then the close() call might fail. Since + # there's nothing we can do about such failures and they annoy + # the end users, we suppress the traceback. + try: + self.close() + except: + pass ### Inquiries ### diff --git a/Lib/test/test_io.py b/Lib/test/test_io.py index dc44e506b1d0f4..eb748a9f4e5a66 100644 --- a/Lib/test/test_io.py +++ b/Lib/test/test_io.py @@ -67,9 +67,9 @@ class EmptyStruct(ctypes.Structure): '--with-memory-sanitizer' in _config_args ) -# Does io.IOBase logs unhandled exceptions on calling close()? -# They are silenced by default in release build. -DESTRUCTOR_LOG_ERRORS = (hasattr(sys, "gettotalrefcount") or sys.flags.dev_mode) +# Does io.IOBase finalizer log the exception if the close() method fails? +# The exception is ignored silently by default in release build. +IOBASE_EMITS_UNRAISABLE = (hasattr(sys, "gettotalrefcount") or sys.flags.dev_mode) def _default_chunk_size(): @@ -1098,23 +1098,19 @@ def test_error_through_destructor(self): # Test that the exception state is not modified by a destructor, # even if close() fails. rawio = self.CloseFailureIO() - def f(): - self.tp(rawio).xyzzy - with support.captured_output("stderr") as s: - self.assertRaises(AttributeError, f) - s = s.getvalue().strip() - if s: - # The destructor *may* have printed an unraisable error, check it - lines = s.splitlines() - if DESTRUCTOR_LOG_ERRORS: - self.assertEqual(len(lines), 5) - self.assertTrue(lines[0].startswith("Exception ignored in: "), lines) - self.assertEqual(lines[1], "Traceback (most recent call last):", lines) - self.assertEqual(lines[4], 'OSError:', lines) + try: + with support.catch_unraisable_exception() as cm: + with self.assertRaises(AttributeError): + self.tp(rawio).xyzzy + + if IOBASE_EMITS_UNRAISABLE: + self.assertIsNotNone(cm.unraisable) + self.assertEqual(cm.unraisable.exc_type, OSError) else: - self.assertEqual(len(lines), 1) - self.assertTrue(lines[-1].startswith("Exception OSError: "), lines) - self.assertTrue(lines[-1].endswith(" ignored"), lines) + self.assertIsNone(cm.unraisable) + finally: + # Explicitly break reference cycle + cm = None def test_repr(self): raw = self.MockRawIO() @@ -2859,23 +2855,19 @@ def test_error_through_destructor(self): # Test that the exception state is not modified by a destructor, # even if close() fails. rawio = self.CloseFailureIO() - def f(): - self.TextIOWrapper(rawio).xyzzy - with support.captured_output("stderr") as s: - self.assertRaises(AttributeError, f) - s = s.getvalue().strip() - if s: - # The destructor *may* have printed an unraisable error, check it - lines = s.splitlines() - if DESTRUCTOR_LOG_ERRORS: - self.assertEqual(len(lines), 5) - self.assertTrue(lines[0].startswith("Exception ignored in: "), lines) - self.assertEqual(lines[1], "Traceback (most recent call last):", lines) - self.assertEqual(lines[4], 'OSError:', lines) + try: + with support.catch_unraisable_exception() as cm: + with self.assertRaises(AttributeError): + self.TextIOWrapper(rawio).xyzzy + + if IOBASE_EMITS_UNRAISABLE: + self.assertIsNotNone(cm.unraisable) + self.assertEqual(cm.unraisable.exc_type, OSError) else: - self.assertEqual(len(lines), 1) - self.assertTrue(lines[-1].startswith("Exception OSError: "), lines) - self.assertTrue(lines[-1].endswith(" ignored"), lines) + self.assertIsNone(cm.unraisable) + finally: + # Explicitly break reference cycle + cm = None # Systematic tests of the text I/O API From 908fa5dfca092d890bfe4aab3c16c81711bff4a2 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Thu, 23 May 2019 02:33:55 +0200 Subject: [PATCH 2/3] Fix test_io --- Lib/test/test_io.py | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/Lib/test/test_io.py b/Lib/test/test_io.py index eb748a9f4e5a66..2c3bf890667997 100644 --- a/Lib/test/test_io.py +++ b/Lib/test/test_io.py @@ -1103,11 +1103,10 @@ def test_error_through_destructor(self): with self.assertRaises(AttributeError): self.tp(rawio).xyzzy - if IOBASE_EMITS_UNRAISABLE: - self.assertIsNotNone(cm.unraisable) - self.assertEqual(cm.unraisable.exc_type, OSError) - else: + if not IOBASE_EMITS_UNRAISABLE: self.assertIsNone(cm.unraisable) + elif cm.unraisable is not None: + self.assertEqual(cm.unraisable.exc_type, OSError) finally: # Explicitly break reference cycle cm = None @@ -2860,11 +2859,10 @@ def test_error_through_destructor(self): with self.assertRaises(AttributeError): self.TextIOWrapper(rawio).xyzzy - if IOBASE_EMITS_UNRAISABLE: - self.assertIsNotNone(cm.unraisable) - self.assertEqual(cm.unraisable.exc_type, OSError) - else: + if not IOBASE_EMITS_UNRAISABLE: self.assertIsNone(cm.unraisable) + elif cm.unraisable is not None: + self.assertEqual(cm.unraisable.exc_type, OSError) finally: # Explicitly break reference cycle cm = None From 7ecedfb50da91a47c7175f2d3dedb5b34ff8d2ee Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Thu, 23 May 2019 03:27:13 +0200 Subject: [PATCH 3/3] Fix reST syntax :opt: becomes :option: --- Doc/whatsnew/3.8.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Doc/whatsnew/3.8.rst b/Doc/whatsnew/3.8.rst index 5ec8bf60eb4faf..134f7002a18d7d 100644 --- a/Doc/whatsnew/3.8.rst +++ b/Doc/whatsnew/3.8.rst @@ -303,7 +303,7 @@ for :func:`property`, :func:`classmethod`, and :func:`staticmethod`:: io -- -In development mode (:opt:`-X` ``env``) and in debug build, the +In development mode (:option:`-X` ``env``) and in debug build, the :class:`io.IOBase` finalizer now logs the exception if the ``close()`` method fails. The exception is ignored silently by default in release build. (Contributed by Victor Stinner in :issue:`18748`.)