Skip to content

WIP: bpo-33694, asyncio: fix proactor transport pause/resume #7479

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.

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

Closed
wants to merge 1 commit into from
Closed

WIP: bpo-33694, asyncio: fix proactor transport pause/resume #7479

wants to merge 1 commit into from

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Jun 7, 2018

Fix pause_reading()/resume_reading() of proactor transpors: don't
cancel overlapped WSARecv() anymore. Sometimes, cancelling an
overlapped WSARecv() looses data since the I/O already completed, but
Overlapped.cancel() silently ignores the ERROR_NOT_FOUND error of
CancelIoEx().

If the transport is paused, store the result in the transport if it's
paused, and only call data_received()/buffer_updated() on resuming
reading.

The complex part is when the read starts with data_received() but
completes during reading is paused and a new protocol using
buffer_updated() is set. Or the opposite: transition from
buffer_updated() to data_received().

FIXME: get_buffer(-1) error is not handled yet and a fix unit tests
on proactor which rely on implementation details are failing, but
functional tests pass.

https://bugs.python.org/issue33694

Fix pause_reading()/resume_reading() of proactor transpors: don't
cancel overlapped WSARecv() anymore. Sometimes, cancelling an
overlapped WSARecv() looses data since the I/O already completed, but
Overlapped.cancel() silently ignores the ERROR_NOT_FOUND error of
CancelIoEx().

If the transport is paused, store the result in the transport if it's
paused, and only call data_received()/buffer_updated() on resuming
reading.

The complex part is when the read starts with data_received() but
completes during reading is paused and a new protocol using
buffer_updated() is set. Or the opposite: transition from
buffer_updated() to data_received().

FIXME: get_buffer(-1) error is not handled yet and a fix unit tests
on proactor which rely on implementation details are failing, but
functional tests pass.
assert self._pending_data is None
self._pending_data = data
else:
self._data_received(data)
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved this code because it was easier for me, to be able to implement the switch from data_received to buffer_updated: "if self._protocol_use_buffer(): self._loop_reading__get_buffer(None)" below. I'm not sure if moving this code changes the semantics? It shouldn't, but 3 proactor tests are failing.

@vstinner vstinner changed the title WIP: bpo-33743, asyncio: fix proactor transport pause/resume WIP: bpo-33694, asyncio: fix proactor transport pause/resume Jun 7, 2018
@vstinner
Copy link
Member Author

vstinner commented Jun 7, 2018

@1st1: What do you think of the overall design? Does it sound reasonable or crazy?

@vstinner
Copy link
Member Author

vstinner commented Jun 7, 2018

I'm aware that 3 tests of test_asyncio are failing. I prefer to wait for a first round of review before looking at fixing them.

@1st1
Copy link
Member

1st1 commented Jun 7, 2018

I actually like the fix and I think it's a very nice way of solving the race between different protocols types after set_protocol is called.

@vstinner
Copy link
Member Author

vstinner commented Jun 7, 2018

I actually like the fix and I think it's a very nice way of solving the race between different protocols types after set_protocol is called.

I don't think that my code for data_received() <=> buffer_updated() protocol transitions is safe. My code keeps a reference to the buffer of the previous protocol (after set_protocol()) and even write into the old buffer, again, even after set_protocol(). That's weird and counter-intuitive, and may lead to crazy bugs.

All the complexity of my PR comes from the fact that proactor has two implementation to read: recv() and recv_into(). If we reimplemented recv_into() on top of recv(), the code becomes again managable, simple and straightforward. Well, it's not my idea, it's @1st1 idea :-)

Since we are close to 3.7.0 final, I would prefer to first to loose data randomly, and then reconsider implementing recv_into() optimization :-)

@1st1
Copy link
Member

1st1 commented Jun 7, 2018

Here's an alternative PR, which I believe is simpler and more robust: #7486

@vstinner
Copy link
Member Author

vstinner commented Jun 8, 2018

This PR was really too complex. I decided with @1st1 to merge a much simpler approach instead: #7498

@1st1
Copy link
Member

1st1 commented Jun 8, 2018

This can now be closed, right?

@1st1 1st1 closed this Jun 8, 2018
@vstinner vstinner deleted the proactor_cancel branch June 8, 2018 00:41
@vstinner
Copy link
Member Author

vstinner commented Jun 8, 2018

This can now be closed, right?

I wanted to close it, right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants