bpo-33694: Fix race condition on proactor recv() #7498
Conversation
The cancellation of an overlapped WSARecv() has a race condition which causes data loss because of the current implementation of proactor in asyncio. No longer cancel overlapped WSARecv() in _ProactorReadPipeTransport to work around the race condition. Remove the optimized recv_into() implementation to get simple implementation of pause_reading() using the single _pending_data attribute.
|
I ran manually race.py of https://bugs.python.org/issue33694:
This script didn't detect any regression, whereas these two tests always reproduced the race condition in a reliable way. |
Lib/asyncio/proactor_events.py
Outdated
| nbytes = len(data) | ||
| if nbytes: | ||
| try: | ||
| buf = self._protocol.get_buffer(-1) |
There was a problem hiding this comment.
You should pass nbytes to get_buffer(). Also, get_buffer() can return a smaller buffer than requested. So you better should use sslproto._feed_data_to_bufferred_proto helper to make sure that all of the received data is passed to the BufferedProtocol.
There was a problem hiding this comment.
you better should use sslproto._feed_data_to_bufferred_proto helper
Ok, done.
Lib/asyncio/proactor_events.py
Outdated
| return | ||
|
|
||
| if isinstance(self._protocol, protocols.BufferedProtocol): | ||
| try: | ||
| sslproto._feed_data_to_bufferred_proto(self._protocol, data) |
There was a problem hiding this comment.
I'd also move _feed_data_to_bufferred_proto helper to asyncio/protocols.py so that this is less confusing for whoever reads the code.
| @@ -459,6 +459,8 @@ def test_dont_pause_writing(self): | |||
| self.assertFalse(self.protocol.pause_writing.called) | |||
|
|
|||
|
|
|||
| @unittest.skip('FIXME: bpo-33694: these tests are too close ' | |||
| 'to the implementation and should be refactored or removed') | |||
|
I tested manually the latest change ("Move _feed_data_to_bufferred_proto() to protocols") on my Windows VM:
Each of these tests always reproduced the race condition with the fix. Note: test_asyncio pass as well ;-) |
Here, a tired but happy smiley face from me: 😍 |
|
Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7. |
The cancellation of an overlapped WSARecv() has a race condition which causes data loss because of the current implementation of proactor in asyncio. No longer cancel overlapped WSARecv() in _ProactorReadPipeTransport to work around the race condition. Remove the optimized recv_into() implementation to get simple implementation of pause_reading() using the single _pending_data attribute. Move _feed_data_to_bufferred_proto() to protocols.py. Remove set_protocol() method which became useless. (cherry picked from commit 79790bc) Co-authored-by: Victor Stinner <vstinner@redhat.com>
|
GH-7499 is a backport of this pull request to the 3.7 branch. |
The cancellation of an overlapped WSARecv() has a race condition which causes data loss because of the current implementation of proactor in asyncio. No longer cancel overlapped WSARecv() in _ProactorReadPipeTransport to work around the race condition. Remove the optimized recv_into() implementation to get simple implementation of pause_reading() using the single _pending_data attribute. Move _feed_data_to_bufferred_proto() to protocols.py. Remove set_protocol() method which became useless. (cherry picked from commit 79790bc) Co-authored-by: Victor Stinner <vstinner@redhat.com>
|
I didn't look at the code but fyi in passing: "bufferred" is misspelled |
The cancellation of an overlapped WSARecv() has a race condition
which causes data loss because of the current implementation of
proactor in asyncio.
No longer cancel overlapped WSARecv() in _ProactorReadPipeTransport
to work around the race condition.
Remove the optimized recv_into() implementation to get simple
implementation of pause_reading() using the single _pending_data
attribute.
https://bugs.python.org/issue33694