Skip to content

bpo-31976: Fix race condition when flushing a file is slow. #4331

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pitrou merged 4 commits into from
Nov 10, 2017
Merged

bpo-31976: Fix race condition when flushing a file is slow. #4331

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
60edd49
Fix race condition when flushing a file is slow.
benfogle Nov 8, 2017
6f3ca75
Requested changes from review.
benfogle Nov 10, 2017
be568bf
Improve precision of NEWS entry
pitrou Nov 10, 2017
d3f86a4
Added reference to issue number.
benfogle Nov 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Requested changes from review.
  • Loading branch information
@benfogle
benfogle committed Nov 10, 2017
commit 6f3ca752dc71c9eb29cb18eb209247485df8c0af
4 changes: 4 additions & 0 deletions Lib/_pyio.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1257,6 +1257,10 @@ 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
# behavior as the C implementation.
try:

@pitrou pitrou Nov 9, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@benfogle benfogle Nov 10, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# may raise BlockingIOError or BrokenPipeError etc
self.flush()
Expand Down
7 changes: 6 additions & 1 deletion Lib/test/test_io.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,12 @@ class PyMisbehavedRawIO(MisbehavedRawIO, pyio.RawIOBase):


class SlowFlushRawIO(MockRawIO):
def __init__(self):
super().__init__()
self.in_flush = threading.Event()

def flush(self):
self.in_flush.set()
time.sleep(0.25)

class CSlowFlushRawIO(SlowFlushRawIO, io.RawIOBase):
Expand Down Expand Up @@ -1742,7 +1747,7 @@ def test_slow_close_from_thread(self):
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()
rawio.in_flush.wait()
self.assertRaises(ValueError, bufio.write, b'spam')
self.assertTrue(bufio.closed)

Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2017-11-09-21-36-32.bpo-31976.EOA7qY.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fixed race condition when flushing a file is slow, which can cause a
segfault.
6 changes: 4 additions & 2 deletions Modules/_io/bufferedio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,10 +347,10 @@ _enter_buffered_busy(buffered *self)
}

#define IS_CLOSED(self) \
!self->buffer || \
(!self->buffer || \
(self->fast_closed_checks \
? _PyFileIO_closed(self->raw) \
: buffered_closed(self))
: buffered_closed(self)))

#define CHECK_CLOSED(self, error_msg) \
if (IS_CLOSED(self)) { \
Expand Down Expand Up @@ -1976,6 +1976,8 @@ _io_BufferedWriter_write_impl(buffered *self, Py_buffer *buffer)
if (!ENTER_BUFFERED(self))
return NULL;

/* Issue #31976: Check for closed file after acquiring the lock. Another
thread could be holding the lock while closing the file. */
if (IS_CLOSED(self)) {

@pitrou pitrou Nov 9, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment and a reference to the bug entry here?

PyErr_SetString(PyExc_ValueError, "write to closed file");
goto error;
Expand Down