From 93273bf1392c946d7d5a18d9117c034666fb6096 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Wed, 12 Jun 2019 22:53:40 +0200 Subject: [PATCH] bpo-37223: Second call to BufferedWrite.close() does nothing When io.BufferedWrite.close() is called twice, the second call now does nothing. Previously, the flush() method and raw.close() were called at the second call. Moreover, even if close() raises an exception, the file is now considered as closed. --- Lib/_pyio.py | 26 +++++--- Lib/test/test_io.py | 63 ++++++++++++++----- .../2019-06-12-22-56-49.bpo-37223.ILECyA.rst | 5 ++ Modules/_io/bufferedio.c | 8 ++- 4 files changed, 77 insertions(+), 25 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2019-06-12-22-56-49.bpo-37223.ILECyA.rst diff --git a/Lib/_pyio.py b/Lib/_pyio.py index 0b6493bc8dc9262..760fb4d15b48213 100644 --- a/Lib/_pyio.py +++ b/Lib/_pyio.py @@ -786,6 +786,7 @@ class _BufferedIOMixin(BufferedIOBase): def __init__(self, raw): self._raw = raw + self._closed = False ### Positioning ### @@ -821,12 +822,17 @@ def flush(self): self.raw.flush() def close(self): - if self.raw is not None and not self.closed: + if self.raw is None or self.closed: + return + + try: + # may raise BlockingIOError or BrokenPipeError etc + self.flush() + finally: try: - # may raise BlockingIOError or BrokenPipeError etc - self.flush() - finally: self.raw.close() + finally: + self._closed = True def detach(self): if self.raw is None: @@ -847,7 +853,7 @@ def raw(self): @property def closed(self): - return self.raw.closed + return self._closed or self.raw.closed @property def name(self): @@ -1045,6 +1051,7 @@ def __init__(self, raw, buffer_size=DEFAULT_BUFFER_SIZE): self.buffer_size = buffer_size self._reset_read_buf() self._read_lock = Lock() + self._closed = False def readable(self): return self.raw.readable() @@ -1235,6 +1242,7 @@ def __init__(self, raw, buffer_size=DEFAULT_BUFFER_SIZE): self.buffer_size = buffer_size self._write_buf = bytearray() self._write_lock = Lock() + self._closed = False def writable(self): return self.raw.writable() @@ -1309,6 +1317,7 @@ def close(self): with self._write_lock: if self.raw is None or self.closed: return + # We have to release the lock and call self.flush() (which will # probably just re-take the lock) in case flush has been overridden in # a subclass or the user set self.flush to something. This is the same @@ -1317,8 +1326,11 @@ def close(self): # may raise BlockingIOError or BrokenPipeError etc self.flush() finally: - with self._write_lock: - self.raw.close() + try: + with self._write_lock: + self.raw.close() + finally: + self._closed = True class BufferedRWPair(BufferedIOBase): diff --git a/Lib/test/test_io.py b/Lib/test/test_io.py index 102679b1d342435..8483202f91da504 100644 --- a/Lib/test/test_io.py +++ b/Lib/test/test_io.py @@ -995,7 +995,7 @@ def flush(self): # This would cause an assertion failure. self.assertRaises(OSError, f.close) - # Silence destructor error + # Silence f destructor error R.flush = lambda self: None @@ -1137,7 +1137,7 @@ def test_flush_error_on_close(self): raw = self.MockRawIO() closed = [] def bad_flush(): - closed[:] = [b.closed, raw.closed] + closed.extend([b.closed, raw.closed]) raise OSError() raw.flush = bad_flush b = self.tp(raw) @@ -1163,11 +1163,10 @@ def bad_close(): self.assertEqual(err.exception.args, ('close',)) self.assertIsInstance(err.exception.__context__, OSError) self.assertEqual(err.exception.__context__.args, ('flush',)) - self.assertFalse(b.closed) + self.assertTrue(b.closed) - # Silence destructor error + # Silence raw destructor error raw.close = lambda: None - b.flush = lambda: None def test_nonnormalized_close_error_on_close(self): # Issue #21677 @@ -1184,10 +1183,9 @@ def bad_close(): self.assertIn('non_existing_close', str(err.exception)) self.assertIsInstance(err.exception.__context__, NameError) self.assertIn('non_existing_flush', str(err.exception.__context__)) - self.assertFalse(b.closed) + self.assertTrue(b.closed) - # Silence destructor error - b.flush = lambda: None + # Silence raw destructor error raw.close = lambda: None def test_multi_close(self): @@ -1196,7 +1194,7 @@ def test_multi_close(self): b.close() b.close() b.close() - self.assertRaises(ValueError, b.flush) + self.assertRaisesRegex(ValueError, 'closed', b.flush) def test_unseekable(self): bufio = self.tp(self.MockUnseekableIO(b"A" * 10)) @@ -1490,7 +1488,7 @@ def test_misbehaved_io(self): self.assertRaises(OSError, bufio.seek, 0) self.assertRaises(OSError, bufio.tell) - # Silence destructor error + # Silence bufio destructor error bufio.close = lambda: None def test_no_extraneous_read(self): @@ -1841,7 +1839,7 @@ def test_misbehaved_io(self): self.assertRaises(OSError, bufio.tell) self.assertRaises(OSError, bufio.write, b"abcdef") - # Silence destructor error + # Silence bufio destructor error bufio.close = lambda: None def test_max_buffer_size_removal(self): @@ -1869,6 +1867,39 @@ def test_slow_close_from_thread(self): self.assertTrue(bufio.closed) t.join() + def test_close_dont_call_flush(self): + # bpo-37223: This test is specific to the C implementation (_io). + raw = self.MockRawIO() + def bad_flush(): + raise OSError() + orig_flush = raw.flush + raw.flush = bad_flush + def bad_close(): + raw.flush() + orig_close = raw.close + raw.close = bad_close + + b = self.tp(raw) + b.write(b'spam') + self.assertRaises(OSError, b.close) # exception not swallowed + self.assertTrue(b.closed) + self.assertFalse(raw.closed) + + def bad_flush2(): + raise AssertionError + + b.flush = bad_flush2 + raw.close = orig_close + raw.flush = orig_flush + + # if b is already closed, b.flush() must not be called + b.close() + + def test_closed_write(self): + raw = self.MockRawIO() + b = self.tp(raw) + b.close() + self.assertRaisesRegex(ValueError, 'closed', b.write, b'abc') class CBufferedWriterTest(BufferedWriterTest, SizeofTest): @@ -2051,7 +2082,7 @@ def reader_close(): self.assertFalse(reader.closed) self.assertTrue(writer.closed) - # Silence destructor error + # Silence reader destructor error reader.close = lambda: None def test_writer_close_error_on_close(self): @@ -2064,11 +2095,11 @@ def writer_close(): with self.assertRaises(NameError) as err: pair.close() self.assertIn('writer_non_existing', str(err.exception)) - self.assertFalse(pair.closed) + self.assertTrue(pair.closed) self.assertTrue(reader.closed) self.assertFalse(writer.closed) - # Silence destructor error + # Silence writer destructor error writer.close = lambda: None def test_reader_writer_close_error_on_close(self): @@ -2086,11 +2117,11 @@ def writer_close(): self.assertIn('reader_non_existing', str(err.exception)) self.assertIsInstance(err.exception.__context__, NameError) self.assertIn('writer_non_existing', str(err.exception.__context__)) - self.assertFalse(pair.closed) + self.assertTrue(pair.closed) self.assertFalse(reader.closed) self.assertFalse(writer.closed) - # Silence destructor error + # Silence reader and writer destructor error reader.close = lambda: None writer.close = lambda: None diff --git a/Misc/NEWS.d/next/Library/2019-06-12-22-56-49.bpo-37223.ILECyA.rst b/Misc/NEWS.d/next/Library/2019-06-12-22-56-49.bpo-37223.ILECyA.rst new file mode 100644 index 000000000000000..774ccec49706f4c --- /dev/null +++ b/Misc/NEWS.d/next/Library/2019-06-12-22-56-49.bpo-37223.ILECyA.rst @@ -0,0 +1,5 @@ +When :meth:`io.BufferedReader.close` or :meth:`io.BufferedWrite.close` is +called twice, the second call now does nothing. Previously, the ``flush()`` +method and ``raw.close()`` were called at the second call. Moreover, even if +``close()`` raises an exception, the file is now considered as closed (even if +the raw file is not considered as closed). diff --git a/Modules/_io/bufferedio.c b/Modules/_io/bufferedio.c index 44e12db6a30ed05..62371a54bbc2559 100644 --- a/Modules/_io/bufferedio.c +++ b/Modules/_io/bufferedio.c @@ -482,7 +482,11 @@ static PyObject * buffered_closed_get(buffered *self, void *context) { CHECK_INITIALIZED(self) - return PyObject_GetAttr(self->raw, _PyIO_str_closed); + int closed = IS_CLOSED(self); + if (closed < 0) { + return NULL; + } + return PyBool_FromLong(closed); } static PyObject * @@ -495,7 +499,7 @@ buffered_close(buffered *self, PyObject *args) if (!ENTER_BUFFERED(self)) return NULL; - r = buffered_closed(self); + r = IS_CLOSED(self); if (r < 0) goto end; if (r > 0) {