python · vstinner · Jun 12, 2019
diff --git 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
@@ -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
@@ -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
@@ -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) {