bpo-31976: Fix race condition when flushing a file is slow. #4331
Conversation
* Move IS_CLOSED check to after the acquiring the lock * Add a check for self->buffer == NULL in IS_CLOSED * Make _pyio.BufferedWriter close the same way the C version does * Add unit tests
There was a problem hiding this comment.
Thank you for spotting this and proposing this fix!
Before this goes in, you should address the comments and also a NEWS entry. For the latter, you can use the "blurb" utility.
Lib/_pyio.py
Outdated
| with self._write_lock: | ||
| if self.raw is None or self.closed: | ||
| return | ||
| try: |
There was a problem hiding this comment.
This doesn't look right. The close() method should take the lock once, not take it three times in a row (one of them implicit in flush()). Note _flush_unlocked() can help you here.
There was a problem hiding this comment.
That made a bunch of tests (test_flush_error_on_close, etc.) fail since close() no longer calls flush() if the file object is subclassed or fileobj.flush is set to something.
This is actually the same behavior as buffered_close in the C implementation. I have added a comment to that effect.
Modules/_io/bufferedio.c
Outdated
|
|
||
| if (!ENTER_BUFFERED(self)) | ||
| return NULL; | ||
|
|
||
| if (IS_CLOSED(self)) { |
There was a problem hiding this comment.
Can you add a comment and a reference to the bug entry here?
Lib/test/test_io.py
Outdated
| @@ -1726,6 +1737,16 @@ def bad_write(b): | |||
| self.assertRaises(OSError, b.close) # exception not swallowed | |||
| self.assertTrue(b.closed) | |||
|
|
|||
| def test_slow_close_from_thread(self): | |||
| rawio = self.SlowFlushRawIO() | |||
There was a problem hiding this comment.
Can you add a comment pointing to the bug entry here?
There was a problem hiding this comment.
It looks like you forgot to address this comment.
There was a problem hiding this comment.
Ah, whoops. Thank you.
Lib/test/test_io.py
Outdated
| bufio = self.tp(rawio, 8) | ||
| t = threading.Thread(target=bufio.close) | ||
| t.start() | ||
| time.sleep(0.1) # Long enough that t is sleeping in bufio.close() |
There was a problem hiding this comment.
You could probably use an Event that you would set in SlowFlushRawIO.flush() instead.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @pitrou: please review the changes made to this pull request. |
|
I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @pitrou: please review the changes made to this pull request. |
|
Thanks. I'm inclined to merge this after CI approval. I'm not sure I want to backport it to 3.6, though, because there is a tiny but non-zero chance that this might break some odd usage somewhere and I don't want that to happen in a bugfix release. |
https://bugs.python.org/issue31976