Skip to content

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
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

@colesbury 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.

@colesbury
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 colesbury requested review from encukou and mpage May 21, 2026 16:26
@bedevere-app bedevere-app Bot added the tests Tests in the Lib/test dir label May 21, 2026
@bedevere-app bedevere-app Bot mentioned this pull request May 21, 2026
Open
@colesbury 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

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 colesbury marked this pull request as ready for review May 21, 2026 16:30
@colesbury colesbury requested review from gpshead and picnixz as code owners May 21, 2026 16:30
@kiri11

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
encukou approved these changes May 27, 2026
View reviewed changes

@encukou 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.

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 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.

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

@vstinner

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

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 kumaraditya303 merged commit fd9d8c3 into python:main Jul 7, 2026
47 of 48 checks passed
@miss-islington-app

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

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 bedevere-app Bot removed the needs backport to 3.15 pre-release feature fixes, bugs and security fixes label Jul 7, 2026
@bedevere-app

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 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
@miss-islington @colesbury
[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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@encukou encukou encukou approved these changes
@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes
@mpage mpage Awaiting requested review from mpage
@gpshead gpshead Awaiting requested review from gpshead gpshead is a code owner
@picnixz 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

@colesbury @kiri11 @vstinner @kumaraditya303 @encukou