Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-118950: Fix SSLProtocol.connection_lost not being called when OSError is thrown on asyncio.write.#118960

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
kumaraditya303 merged 8 commits intopython:mainfromcjavad:fix-issue-118950
Oct 24, 2024

Conversation

cjavad
Copy link
Contributor

@cjavadcjavad commentedMay 12, 2024•
edited
Loading

Let _SSLProtocolTransport.is_closing reflect SSLProtocol internal transport closing state so that StreamWriter.drain will invoke sleep(0) which calls connection_lost and correctly notifies waiters of connection lost. (#118950)

In an existing comment inasyncio/streams.py there is some logic that correctly ensures that theconnection_lost function is always called for users that keep writing and draining in some other context.

The issue here is that this code only runs when the transport is marked as closing, which in the case of the _SSLTransportProtocol only happened after connection_lost has been called.

My proposed fix is for _SSLTransportProtocol.is_closing to also return true when its inner transport is marked as closing, when connection_lost is called the inner transport is removed and instead the _closed flag is set.

No additional tests have been added (yet).

gbtami reacted with thumbs up emoji
@ghost
Copy link

ghost commentedMay 12, 2024•
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app
Copy link

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply theskip news label instead.

…ocol internal transport closing state so that StreamWriter.drain will invoke sleep(0) which calls connection_lost and correctly notifies waiters of connection lost. (python#118950)
@cjavad
Copy link
ContributorAuthor

cjavad commentedMay 12, 2024•
edited
Loading

As requested@gvanrossum i've opened up a PR for#118950

cjavad added a commit to SimplyPrint/printer-ws-client that referenced this pull requestMay 18, 2024
…e connection and cancellation and handle exceptions more gracefully. This addresses the 1006 issue.
@gvanrossum
Copy link
Member

@kumaraditya303 Do you have time to review this? I know nothing about SSL. Who else could we ask?

@kumaraditya303
Copy link
Contributor

Please add some tests, I'll try to find time to review in following week.

@Dreamsorcerer
Copy link
Contributor

I'm not familiar with the SSL code, but the change looks reasonable to me and certainly seems to fix the aiohttp issue (aio-libs/aiohttp#3122).

@cjavad
Copy link
ContributorAuthor

cjavad commentedAug 14, 2024•
edited
Loading

I will get around to adding some test cases in afew weeks today, but anyone is more than welcome to chip in, you can see my original standalone tests (especially the first one) in the issue.

Note: All tests are passing, the final check never ran due to GitHub going down.

@gvanrossumgvanrossum removed their request for reviewAugust 14, 2024 22:48
@cjavad
Copy link
ContributorAuthor

Is it realistic to expect this bug fix will be backported to 3.12? Or should we expect it in 3.13 or 3.14?

Copy link
Contributor

@willingcwillingc left a comment

Choose a reason for hiding this comment

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

These changes look good to me.

@1st1 Double-checking with you that backporting to 3.13 and 3.12 makes sense for this PR.

gbtami reacted with thumbs up emoji
@willingcwillingc added the needs backport to 3.12only security fixes labelOct 21, 2024
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, I tweaked the test a bit and the news entry.

cjavad reacted with thumbs up emojigbtami reacted with hooray emojicjavad reacted with heart emoji
@kumaraditya303kumaraditya303 merged commit3f24bde intopython:mainOct 24, 2024
35 checks passed
@miss-islington-app
Copy link

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestOct 24, 2024
…n OSError is thrown (pythonGH-118960)(cherry picked from commit3f24bde)Co-authored-by: Javad Shafique <javadshafique@hotmail.com>Co-authored-by: Kumar Aditya <kumaraditya@python.org>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestOct 24, 2024
…n OSError is thrown (pythonGH-118960)(cherry picked from commit3f24bde)Co-authored-by: Javad Shafique <javadshafique@hotmail.com>Co-authored-by: Kumar Aditya <kumaraditya@python.org>
@bedevere-app
Copy link

GH-125931 is a backport of this pull request to the3.13 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.13bugs and security fixes labelOct 24, 2024
@bedevere-app
Copy link

GH-125932 is a backport of this pull request to the3.12 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.12only security fixes labelOct 24, 2024
kumaraditya303 added a commit that referenced this pull requestOct 26, 2024
…en OSError is thrown (GH-118960) (#125931)gh-118950: Fix SSLProtocol.connection_lost not being called when OSError is thrown (GH-118960)(cherry picked from commit3f24bde)Co-authored-by: Javad Shafique <javadshafique@hotmail.com>Co-authored-by: Kumar Aditya <kumaraditya@python.org>
kumaraditya303 added a commit that referenced this pull requestOct 26, 2024
…en OSError is thrown (GH-118960) (#125932)gh-118950: Fix SSLProtocol.connection_lost not being called when OSError is thrown (GH-118960)(cherry picked from commit3f24bde)Co-authored-by: Javad Shafique <javadshafique@hotmail.com>Co-authored-by: Kumar Aditya <kumaraditya@python.org>
bdraco added a commit to home-assistant/core that referenced this pull requestNov 1, 2024
python/cpython#118960 has been fixed which meanscleanup closed should no longer be needed
bdraco added a commit to aio-libs/aiohttp that referenced this pull requestNov 9, 2024
ebonnal pushed a commit to ebonnal/cpython that referenced this pull requestJan 12, 2025
…n OSError is thrown (python#118960)Co-authored-by: Kumar Aditya <kumaraditya@python.org>
@XenoAmess
Copy link

XenoAmess commentedMar 5, 2025•
edited
Loading

would it be backport to 3.11 & 3.10? many thanks...
come to think of it, it might be somehow a security issue if you treat it as a way to perform dos

@cjavad
Copy link
ContributorAuthor

would it be backport to 3.11 & 3.10? many thanks...

come to think of it, it might be somehow a security issue if you treat it as a way to perform dos

Seehttps://github.com/SimplyPrint/printer-ws-client/blob/2f52cc85f604d42d5a6728f07cc029868102ae50/simplyprint_ws_client/_polyfill.py#L16 for an example of how to patch it dynamically.

XenoAmess reacted with hooray emoji

@XenoAmess
Copy link

would it be backport to 3.11 & 3.10? many thanks...
come to think of it, it might be somehow a security issue if you treat it as a way to perform dos

Seehttps://github.com/SimplyPrint/printer-ws-client/blob/2f52cc85f604d42d5a6728f07cc029868102ae50/simplyprint_ws_client/_polyfill.py#L16 for an example of how to patch it dynamically.

thank you sincerely

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@1st11st11st1 approved these changes

@willingcwillingcwillingc approved these changes

@kumaraditya303kumaraditya303kumaraditya303 approved these changes

@asvetlovasvetlovAwaiting requested review from asvetlov

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

7 participants
@cjavad@gvanrossum@kumaraditya303@Dreamsorcerer@1st1@willingc@XenoAmess
[8]ページ先頭
©2009-2025 Movatter.jp