From 6e7316d202e085565a00c1fbd9a2967fe701ab7b Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Wed, 12 Jun 2019 04:28:27 +0200 Subject: [PATCH 1/2] bpo-37223: io.BufferedWrite.close() don't call flush() if already closed io.BufferedWrite.close() no longer call its flush() method if close() has already been closed. * buffered_close() no longer call buffered_flush() if self->buffer is NULL * _pyio.BufferedWriter.close() now sets _write_buf to None. Moreover, it doesn't call flush() anymore if _write_buf is None. * Add test_flush_error_on_close() to test_io. --- Lib/_pyio.py | 12 ++++++-- Lib/test/test_io.py | 27 +++++++++++++++++ .../2019-06-12-04-33-59.bpo-37223.oZqbr9.rst | 3 ++ Modules/_io/bufferedio.c | 29 +++++++++++++------ 4 files changed, 59 insertions(+), 12 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2019-06-12-04-33-59.bpo-37223.oZqbr9.rst diff --git a/Lib/_pyio.py b/Lib/_pyio.py index 0b6493bc8dc926..df31c18935b811 100644 --- a/Lib/_pyio.py +++ b/Lib/_pyio.py @@ -1279,7 +1279,7 @@ def flush(self): self._flush_unlocked() def _flush_unlocked(self): - if self.closed: + if self.closed or self._write_buf is None: raise ValueError("flush on closed file") while self._write_buf: try: @@ -1314,9 +1314,15 @@ def close(self): # a subclass or the user set self.flush to something. This is the same # behavior as the C implementation. try: - # may raise BlockingIOError or BrokenPipeError etc - self.flush() + # bpo-37223: If close() has already been called, _write_buf is set + # to None. In this case, flush() would raise + # ValueError("flush on closed file"). + if self._write_buf is not None: + # may raise BlockingIOError or BrokenPipeError etc + self.flush() finally: + self._write_buf = None + with self._write_lock: self.raw.close() diff --git a/Lib/test/test_io.py b/Lib/test/test_io.py index 102679b1d34243..193f1fed2ab0a3 100644 --- a/Lib/test/test_io.py +++ b/Lib/test/test_io.py @@ -1869,6 +1869,33 @@ 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.assertFalse(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() class CBufferedWriterTest(BufferedWriterTest, SizeofTest): diff --git a/Misc/NEWS.d/next/Library/2019-06-12-04-33-59.bpo-37223.oZqbr9.rst b/Misc/NEWS.d/next/Library/2019-06-12-04-33-59.bpo-37223.oZqbr9.rst new file mode 100644 index 00000000000000..41a9d14e254a4d --- /dev/null +++ b/Misc/NEWS.d/next/Library/2019-06-12-04-33-59.bpo-37223.oZqbr9.rst @@ -0,0 +1,3 @@ +:class:`io.BufferedWrite.close` no longer call its +:meth:`~io.BufferedWrite.flush` method if :class:`~io.BufferedWrite.close` has +already been closed. diff --git a/Modules/_io/bufferedio.c b/Modules/_io/bufferedio.c index 44e12db6a30ed0..08ee55fe25c0ed 100644 --- a/Modules/_io/bufferedio.c +++ b/Modules/_io/bufferedio.c @@ -511,15 +511,26 @@ buffered_close(buffered *self, PyObject *args) else PyErr_Clear(); } - /* flush() will most probably re-take the lock, so drop it first */ - LEAVE_BUFFERED(self) - res = PyObject_CallMethodObjArgs((PyObject *)self, _PyIO_str_flush, NULL); - if (!ENTER_BUFFERED(self)) - return NULL; - if (res == NULL) - PyErr_Fetch(&exc, &val, &tb); - else - Py_DECREF(res); + + /* bpo-37223: If buffered_close() has already been called, self->buffer is set + to NULL. In this case, calling buffered_flush() would raise + ValueError("flush of closed file"). */ + if (self->buffer != NULL) { + /* flush() will most probably re-take the lock, so drop it first */ + LEAVE_BUFFERED(self) + + res = PyObject_CallMethodObjArgs((PyObject *)self, _PyIO_str_flush, NULL); + + if (!ENTER_BUFFERED(self)) { + return NULL; + } + if (res == NULL) { + PyErr_Fetch(&exc, &val, &tb); + } + else { + Py_DECREF(res); + } + } res = PyObject_CallMethodObjArgs(self->raw, _PyIO_str_close, NULL); From 6af7aa334513239d33865c2f5e6027b0cd23a04a Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Wed, 12 Jun 2019 22:43:16 +0200 Subject: [PATCH 2/2] Enhance _pyio.BufferedWriter Add more unit tests --- Lib/_pyio.py | 22 ++++++++++++++-------- Lib/test/test_io.py | 8 +++++++- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/Lib/_pyio.py b/Lib/_pyio.py index df31c18935b811..e88b9a30b21da8 100644 --- a/Lib/_pyio.py +++ b/Lib/_pyio.py @@ -1239,12 +1239,15 @@ def __init__(self, raw, buffer_size=DEFAULT_BUFFER_SIZE): def writable(self): return self.raw.writable() + def _check_closed(self, err_msg): + if self.closed or self._write_buf is None: + raise ValueError(err_msg) + def write(self, b): if isinstance(b, str): raise TypeError("can't write str to binary stream") with self._write_lock: - if self.closed: - raise ValueError("write to closed file") + self._check_closed("write to closed file") # XXX we can implement some more tricks to try and avoid # partial writes if len(self._write_buf) > self.buffer_size: @@ -1279,8 +1282,7 @@ def flush(self): self._flush_unlocked() def _flush_unlocked(self): - if self.closed or self._write_buf is None: - raise ValueError("flush on closed file") + self._check_closed("flush on closed file") while self._write_buf: try: n = self.raw.write(self._write_buf) @@ -1296,7 +1298,10 @@ def _flush_unlocked(self): del self._write_buf[:n] def tell(self): - return _BufferedIOMixin.tell(self) + len(self._write_buf) + pos = _BufferedIOMixin.tell(self) + if self._write_buf is not None: + pos += len(self._write_buf) + return pos def seek(self, pos, whence=0): if whence not in valid_seek_flags: @@ -1309,6 +1314,8 @@ def close(self): with self._write_lock: if self.raw is None or self.closed: return + has_write_buf = (self._write_buf is not None) + # 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,13 +1324,12 @@ def close(self): # bpo-37223: If close() has already been called, _write_buf is set # to None. In this case, flush() would raise # ValueError("flush on closed file"). - if self._write_buf is not None: + if has_write_buf: # may raise BlockingIOError or BrokenPipeError etc self.flush() finally: - self._write_buf = None - with self._write_lock: + self._write_buf = None self.raw.close() diff --git a/Lib/test/test_io.py b/Lib/test/test_io.py index 193f1fed2ab0a3..2bca5c9425dd62 100644 --- a/Lib/test/test_io.py +++ b/Lib/test/test_io.py @@ -1196,7 +1196,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)) @@ -1897,6 +1897,12 @@ def bad_flush2(): # 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): tp = io.BufferedWriter