FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-150191: Avoid data race in test_sni_callback_race by colesbury · Pull Request #150193 · python/cpython · GitHub

/ cpython Public

gh-150191: Avoid data race in test_sni_callback_race - #150193

Merged
kumaraditya303 merged 2 commits into
python:mainfrom
colesbury:gh-150191-fix-test-sni-callback-race
Jul 7, 2026
ConversationCommits (2)ChecksFiles changed
Merged

gh-150191: Avoid data race in test_sni_callback_race#150193
kumaraditya303 merged 2 commits into
python:mainfrom
colesbury:gh-150191-fix-test-sni-callback-race

Conversation

colesbury commented May 21, 2026 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

Use a single handshake thread to avoid OpenSSL-internal data races on shared SSL_CTX state, while keeping multiple togglers to exercise the sni_callback setter race from gh-149816.

pythongh-150191: Avoid data race in test_sni_callback_race
5ec6b3d 
Use a single handshake thread to avoid OpenSSL-internal data races on
shared SSL_CTX state, while keeping multiple togglers to exercise the
sni_callback setter race from pythongh-149816.
colesbury requested review from encukou and mpage May 21, 2026 16:26
bedevere-app Bot added the tests Tests in the Lib/test dir label May 21, 2026
bedevere-app Bot mentioned this pull request May 21, 2026
Open
colesbury added skip news needs backport to 3.14 bugs and security fixes needs backport to 3.15 pre-release feature fixes, bugs and security fixes labels May 21, 2026

colesbury commented May 21, 2026

Copy link
Copy Markdown
Contributor Author

A few other related changes:

  • Run the test in the GIL-enabled build as well. "test is only useful if the GIL is disabled" is wrong. The data race reported exists in the GIL-enabled build as well. In general, we should write these tests so that they run in both the GIL-enabled and free-threaded build.
  • Use threading_helper.run_concurrently, which sets up the needed exception tracking and uses a barrier, which helps catch data races by making it more likely that threads run at the same time.
  • Run the threads for 0.1 seconds. Previously "done.set()" was called immediately so that the threads barely had time to run, if at all. (Alternatively, we could run them for a fixed number of iterations)

colesbury marked this pull request as ready for review May 21, 2026 16:30
colesbury requested review from gpshead and picnixz as code owners May 21, 2026 16:30

kiri11 commented May 21, 2026

Copy link
Copy Markdown
Contributor

@colesbury Thanks for the test improvements!

  • Run the threads for 0.1 seconds. Previously "done.set()" was called immediately so that the threads barely had time to run, if at all. (Alternatively, we could run them for a fixed number of iterations)

Running it for at least 0.1s does dramatically increase the consistency, although I've been told we shouldn't add static delays to the tests, even as small as 0.1s.

encukou approved these changes May 27, 2026
View reviewed changes

encukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thank you for the fix! I have a suggestion to avoid depending on clock time:

Comment thread Lib/test/test_ssl.py

def do_handshakes():
while not done.is_set():
while time.monotonic() < deadline:

encukou May 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What do you think about running a fixed number of iterations here, and when this thread setting an event when it's done?

vstinner commented Jun 11, 2026

Copy link
Copy Markdown
Member

Tests / Ubuntu (free-threading) / build and test (ubuntu-24.04-arm) (pull_request): Failing

Strange. That's already the 3rd attempt. Anyway, I restarted the failing job.

kumaraditya303 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

I am merging this PR as it is and will implement the suggestion to make it fixed iterations in a followup.

kumaraditya303 merged commit fd9d8c3 into python:main Jul 7, 2026
47 of 48 checks passed

miss-islington-app Bot commented Jul 7, 2026

Copy link
Copy Markdown

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

bedevere-app Bot commented Jul 7, 2026

Copy link
Copy Markdown

GH-153269 is a backport of this pull request to the 3.15 branch.

bedevere-app Bot removed the needs backport to 3.15 pre-release feature fixes, bugs and security fixes label Jul 7, 2026

bedevere-app Bot commented Jul 7, 2026

Copy link
Copy Markdown

GH-153270 is a backport of this pull request to the 3.14 branch.

bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label Jul 7, 2026
kumaraditya303 pushed a commit that referenced this pull request Jul 7, 2026
[3.15] gh-150191: Avoid data race in test_sni_callback_race (GH-150193)…
9ae12fb 
… (#153269)

gh-150191: Avoid data race in test_sni_callback_race (GH-150193)
(cherry picked from commit fd9d8c3)

Co-authored-by: Sam Gross <colesbury@gmail.com>
kumaraditya303 pushed a commit that referenced this pull request Jul 8, 2026
[3.14] gh-150191: Avoid data race in test_sni_callback_race (GH-150193)…
50300ac 
… (#153270)

gh-150191: Avoid data race in test_sni_callback_race (GH-150193)
(cherry picked from commit fd9d8c3)

Co-authored-by: Sam Gross <colesbury@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

encukou encukou approved these changes
kumaraditya303 kumaraditya303 approved these changes
mpage Awaiting requested review from mpage
gpshead Awaiting requested review from gpshead gpshead is a code owner
picnixz Awaiting requested review from picnixz picnixz is a code owner

Assignees

No one assigned

Labels

skip news tests Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL