Skip to content

gh-103607: Fixing pause_reading called in connection made is ignored #17425

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

Jump to bottom
Merged
merged 14 commits into from
Apr 27, 2023
Merged

gh-103607: Fixing pause_reading called in connection made is ignored #17425

merged 14 commits into from
Apr 27, 2023

Conversation

Itayazolay
Copy link
Contributor

@Itayazolay Itayazolay commented Dec 1, 2019 •
edited by bedevere-bot
Loading

bpo-31821: Fixing pause_reading called in connection made is ignored

https://bugs.python.org/issue31821

@csabella
Copy link
Contributor

This is an alternative solution to #4053.

@csabella csabella requested a review from pitrou December 15, 2019 12:54
@pitrou
Copy link
Member

pitrou commented Dec 15, 2019

I'm not active on asyncio anymore. Perhaps @1st1 or @asvetlov can take a look. But I can still spot a problem: this PR only addresses the selector-based event loops, therefore it is incomplete.

@Itayazolay
Copy link
Contributor Author

Thanks for your response,
I included unix_events in this PR, but I don't think the ssl_events are relevant to this PR since this bug only included protocol classes that are paused on connection_made, and either way SSL is performing the handshake when a connection is made.
I also added is_reading() to unixReadPipeTransport, since it inherits from ReadTransport but isn't implemented.
As for PR #4053, I see that it's inactive since January 2019.
Please let me know if there is something I need to do further.

asvetlov
asvetlov requested changes Jan 12, 2020
View reviewed changes
@bedevere-bot
Copy link

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. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@Itayazolay
Copy link
Contributor Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@asvetlov: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from asvetlov January 12, 2020 16:47
asvetlov
asvetlov previously requested changes Jan 12, 2020
View reviewed changes
Copy link
Contributor

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

Aha, I see the point.
pause_reading belongs to ReadTransport, _SelectorTransport is used by _SelectorDatagramTransport also.
I know, we have a little mess in transport inheritance currently; it should be fixed.

Perhaps the previous version was better; I'd like to clean up the inheritance before returning to the review of this PR anyway.
Sorry for misleading.

@bedevere-bot
Copy link

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. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

gvanrossum
gvanrossum previously requested changes Apr 20, 2023
View reviewed changes
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Good work. I would like to see one more test. Also, @kumaraditya303 do you have any comments?

@kumaraditya303 kumaraditya303 removed the request for review from pitrou April 20, 2023 14:49
@kumaraditya303
Copy link
Contributor

I'll review this by tomorrow.

@kumaraditya303 kumaraditya303 self-assigned this Apr 20, 2023
kumaraditya303
kumaraditya303 reviewed Apr 21, 2023
View reviewed changes
Copy link
Contributor

@kumaraditya303 kumaraditya303 left a comment

Choose a reason for hiding this comment

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

Please add a test for proactor event loop too.

@Itayazolay
Copy link
Contributor Author

Itayazolay commented Apr 22, 2023 •
edited
Loading

Thank you both for responding!
I've refactored the test to be called from within the connection_made, and added the test to the proactor loop as well.
Is there anything else I should do?

@Itayazolay
Copy link
Contributor Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@gvanrossum, @asvetlov: please review the changes made to this pull request.

kumaraditya303
kumaraditya303 approved these changes Apr 25, 2023
View reviewed changes
Copy link
Contributor

@kumaraditya303 kumaraditya303 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!

@gvanrossum
Copy link
Member

@kumaraditya303 Feel free to merge if you are happy now.

@kumaraditya303 kumaraditya303 merged commit 78942ec into python:main Apr 27, 2023
@miss-islington
Copy link
Contributor

Thanks @Itayazolay for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 27, 2023
@Itayazolay @kumaraditya303 @miss-islington
pythongh-103607: Fix pause_reading to work when called from `connec…
3ffea86 
…tion_made` in `asyncio`. (pythonGH-17425)

(cherry picked from commit 78942ec)

Co-authored-by: Itayazolay <itayazolay@gmail.com>
Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
@bedevere-bot
Copy link

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

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Apr 27, 2023
kumaraditya303 added a commit that referenced this pull request Apr 27, 2023
@miss-islington @Itayazolay @kumaraditya303
[3.11] gh-103607: Fix pause_reading to work when called from `conne…
2cd1b9c 
…ction_made` in `asyncio`. (GH-17425) (#103918)

gh-103607: Fix `pause_reading` to work when called from `connection_made` in `asyncio`. (GH-17425)
(cherry picked from commit 78942ec)

Co-authored-by: Itayazolay <itayazolay@gmail.com>
Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
@zhengfeihe zhengfeihe mentioned this pull request Apr 15, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arhadthedev arhadthedev arhadthedev left review comments

@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes

@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner

@gvanrossum gvanrossum Awaiting requested review from gvanrossum

@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

Assignees

@kumaraditya303 kumaraditya303

Labels
topic-asyncio
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.