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-124984: Fixssl thread safety#124993

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
vstinner merged 60 commits intopython:mainfromZeroIntensity:fix-ssl-threadsafety
Oct 19, 2024

Conversation

ZeroIntensity
Copy link
Member

@ZeroIntensityZeroIntensity commentedOct 5, 2024
edited
Loading

As it turns out, OpenSSL doesn't like being called in multiple threads. This adds a per-socket (and per-context and per-session) lock for all OpenSSL calls.

Copy link
Member

@JelleZijlstraJelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thanks, that's a lot of locks.

ZeroIntensity reacted with rocket emoji
ZeroIntensityand others added3 commitsOctober 5, 2024 10:14
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@vstinner
Copy link
Member

Merged, thank you! I replaced "Fix" with "Enhance" in the commit message: "Enhance ssl thread safety" :-)

ZeroIntensity reacted with hooray emoji

@ZeroIntensityZeroIntensity deleted the fix-ssl-threadsafety branchOctober 19, 2024 21:18
@vstinner
Copy link
Member

Remaining question: should we backport thisenhancement (bugfix?) to Python 3.13?

@ZeroIntensity
Copy link
MemberAuthor

Remaining question: should we backport thisenhancement (bugfix?) to Python 3.13?

Yeah, I had the same thought. I'll leave that decision to@Yhg1s

@Yhg1s
Copy link
Member

Backport this to 3.13? Uuhm...

image

No.

ZeroIntensity reacted with laugh emoji

@vstinner
Copy link
Member

Without this change, thessl module is not usable on a Free Threaded build (it does easily crash). The change only affects the Free Threaded build: adding@critical_section does nothing in the regular build, and the added test is ignored on regular Python (only run on Free Threading). I would be fine with no backporting it, since Free Threading remains experimental.

@colesbury
Copy link
Contributor

colesbury commentedOct 20, 2024
edited
Loading

I think that fixing the SSL module crash in the 3.13 free threading build is important -- lots of basic tasks around HTTP requests are likely to crash without it.

I don't think "lines of code changed" is a good measure of the complexity here -- theModules/_ssl.c changes are mostly mechanical, and the only behavioral changes is additional locking in the free threading build.

If backporting this PR is a nonstarter than we should consider a smaller, more targeted change that only adds@critical_section toSSLContext methods (without changing getters and setters). That seems less ideal to me, but should fix the common SSL crash and be a pretty small code change.

JelleZijlstra, AA-Turner, dolfinus, and ZeroIntensity reacted with thumbs up emoji

@colesbury
Copy link
Contributor

colesbury commentedOct 20, 2024
edited
Loading

Also, thank you@ZeroIntensity for fixing this bug and@vstinner,@corona10, and everyone else that reviewed the PR.

@vstinnervstinner added the needs backport to 3.13bugs and security fixes labelOct 21, 2024
@miss-islington-app
Copy link

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

@miss-islington-app
Copy link

Sorry,@ZeroIntensity and@vstinner, I could not cleanly backport this to3.13 due to a conflict.
Please backport usingcherry_picker on command line.

cherry_picker 4c53b2577531c77193430cdcd66ad6385fcda81f 3.13

@vstinner
Copy link
Member

@ZeroIntensity: Automated backport failed. Would you mind to backport the change manually?

With a backport, it might be easier to take a decision on fixing 3.13 or not.

@ZeroIntensity
Copy link
MemberAuthor

Yeah, I can do it later today.

@bedevere-app
Copy link

GH-125780 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 21, 2024
ZeroIntensity added a commit to ZeroIntensity/cpython that referenced this pull requestOct 21, 2024
Make SSL objects thread safe in Free Theaded build byusing critical sections.(cherry picked from commit4c53b25)Co-authored-by: Peter Bierma <zintensitydev@gmail.com>Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@vstinner
Copy link
Member

@Yhg1s: Are you still against the backport to 3.13 after@colesbury's comment?

@Yhg1s
Copy link
Member

I'm okay with a backport of just the @critical_section changes (since those expand to nothing in the normal build, and the free-threaded build is experimental anyway). It's the larger refactorings that worry me.

@vstinner
Copy link
Member

It's the larger refactorings that worry me.

Other changes are tests and changes to use the code declared with@critical_section.

@ZeroIntensity
Copy link
MemberAuthor

There isn't any other refactoring going on here, I just had to switch the getters and setters over to AC for the critical section. Another issue is that not backporting this to 3.13 will also hurt any automatic backports forssl in the future.

Yhg1s pushed a commit that referenced this pull requestDec 2, 2024
Make SSL objects thread safe in Free Theaded build byusing critical sections.(cherry picked from commit4c53b25)Co-authored-by: Peter Bierma <zintensitydev@gmail.com>Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
ebonnal pushed a commit to ebonnal/cpython that referenced this pull requestJan 12, 2025
Make SSL objects thread safe in Free Theaded build byusing critical sections.Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@gpsheadgpsheadgpshead left review comments

@vstinnervstinnervstinner approved these changes

@JelleZijlstraJelleZijlstraJelleZijlstra approved these changes

@corona10corona10corona10 approved these changes

@picnixzpicnixzpicnixz approved these changes

@AA-TurnerAA-TurnerAwaiting requested review from AA-Turner

@colesburycolesburyAwaiting requested review from colesbury

Assignees

@ZeroIntensityZeroIntensity

Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

11 participants
@ZeroIntensity@picnixz@AA-Turner@efimov-mikhail@colesbury@vstinner@bedevere-bot@Yhg1s@gpshead@JelleZijlstra@corona10

[8]ページ先頭

©2009-2025 Movatter.jp