Skip to content

Conversation

@Fidget-Spinner
Copy link
Member

@Fidget-SpinnerFidget-Spinner commented Oct 26, 2022

@Fidget-Spinner
Copy link
MemberAuthor

@kumaraditya303 I don't know why but this started showing on our tests after #98572. I can't tell if that PR introduced a bug or it just exposed something wrong with our implementation.

IMO, it seems it's something wrong with our implementation.

@kumaraditya303
Copy link
Contributor

asyncio always has surprises hence it is my favorite :)

Seriously though this has uncovered another race in our implementation. The code does not guard against concurrent closing of the stream which leads to this failure. The connection_lost callback should be called only once and here it is being called more than once.

Here's a better fix:

diff --git a/Lib/asyncio/proactor_events.py b/Lib/asyncio/proactor_events.py index 2685a3376c..c6aab408fc 100644 --- a/Lib/asyncio/proactor_events.py+++ b/Lib/asyncio/proactor_events.py@@ -152,6 +152,8 @@ def _force_close(self, exc): self._loop.call_soon(self._call_connection_lost, exc) def _call_connection_lost(self, exc): + if self._called_connection_lost:+ return try: self._protocol.connection_lost(exc) finally: 

Feel free to update this PR with the patch above, I have verified this.

Co-Authored-By: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
@Fidget-Spinner
Copy link
MemberAuthor

Feel free to update this PR with the patch above, I have verified this.

Awesome! Thank you for the fix. I was trying to find something more elegant but I'm a noob at asyncio internals :). Your fix is way nicer.

Co-Authored-By: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
Copy link
Contributor

@kumaraditya303kumaraditya303 left a comment

Choose a reason for hiding this comment

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

LGTM thanks for the PR!

@kumaraditya303
Copy link
Contributor

I created #98730 to add tests for this.

Copy link
Member

@gvanrossumgvanrossum left a comment

Choose a reason for hiding this comment

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

Ironic.

@miss-islington
Copy link
Contributor

Thanks @Fidget-Spinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

@Fidget-SpinnerFidget-Spinner deleted the fix_asyncio_test_drain_release branch October 27, 2022 02:37
@miss-islington
Copy link
Contributor

Sorry @Fidget-Spinner, I had trouble checking out the 3.11 backport branch.
Please retry by removing and re-adding the "needs backport to 3.11" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker 8a755423eba8e19704d96905730cf7f50083eb23 3.11

@bedevere-bot
Copy link

GH-98753 is a backport of this pull request to the 3.10 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 27, 2022
…lost multiple times (pythonGH-98704) Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com> (cherry picked from commit 8a75542) Co-authored-by: Ken Jin <kenjin@python.org>
@bedevere-botbedevere-bot removed the needs backport to 3.10 only security fixes label Oct 27, 2022
@Fidget-SpinnerFidget-Spinner added needs backport to 3.11 only security fixes and removed needs backport to 3.11 only security fixes labels Oct 27, 2022
@miss-islington
Copy link
Contributor

Thanks @Fidget-Spinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-98754 is a backport of this pull request to the 3.11 branch.

@bedevere-botbedevere-bot removed the needs backport to 3.11 only security fixes label Oct 27, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 27, 2022
…lost multiple times (pythonGH-98704) Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com> (cherry picked from commit 8a75542) Co-authored-by: Ken Jin <kenjin@python.org>
miss-islington added a commit that referenced this pull request Oct 27, 2022
…ultiple times (GH-98704) Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com> (cherry picked from commit 8a75542) Co-authored-by: Ken Jin <kenjin@python.org>
miss-islington added a commit that referenced this pull request Oct 27, 2022
…ultiple times (GH-98704) Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com> (cherry picked from commit 8a75542) Co-authored-by: Ken Jin <kenjin@python.org>
gvanrossum pushed a commit to gvanrossum/cpython that referenced this pull request Oct 28, 2022
…lost multiple times (pythonGH-98704) Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
Sign up for freeto 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.

5 participants

@Fidget-Spinner@kumaraditya303@miss-islington@bedevere-bot@gvanrossum