Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-116738: Make cmath module thread-safe#142161
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
yoney commented Dec 1, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
colesbury left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good. It's subinterpreter specific and not really free threading related (i.e., you'll have the same data races with the default GIL enabled build).
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Misc/NEWS.d/next/Core_and_Builtins/2025-12-01-10-03-08.gh-issue-116738.972YsG.rst Outdated Show resolvedHide resolved
Uh oh!
There was an error while loading. Please reload this page.
yoney commented Dec 1, 2025
Thank you for the review, Sam!
Yes, I agree, this is related to subinterpreters. I was mainly checking the module for ft-python, which is why I created the PR under gh-116738 to mark it checked. |
Uh oh!
There was an error while loading. Please reload this page.
skirpichev left a comment • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but could you double check and verify that all functions include tests for all combinations of special values? I.e. when one component is a zero, infinity or nan, same other (or is a finite nonzero number, if the first is nonzero).
Tests are here: Lib/test/mathdata/cmath_testcases.txt (usually in a dedicated section, like "special values").
Test script
#!/bin/sh# a.sh file=Lib/test/mathdata/cmath_testcases.txt funcs="acos acosh asinh atanh cosh exp log sinh sqrt tanh rect" zeros="0.0 -0.0" nonfinite="-inf nan inf" all="$zeros$nonfinite [1-9]\.[0-9].* -[1-9]\.[0-9].*"forfin$funcsdofort1in$zerosdofort2in$zeros$nonfinitedo re="$f$t1$t2 -> " grep -q "$re"$file||echo"$f$t1$t2 - missing" re="$f$t2$t1 -> " grep -q "$re"$file||echo"$f$t2$t1 - missing"donedonefort1in$nonfinitedofort2in$alldo re="$f$t1$t2 -> " grep -q "$re"$file||echo"$f$t1$t2 - missing!" re="$f$t2$t1 -> " grep -q "$re"$file||echo"$f$t2$t1 - missing!"donedonedone
skirpichev left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I've checked that special values are tested, though appreciate a second look. Please adjust PR description (an maybe title).
yoney commented Dec 3, 2025
Thank you so much, @skirpichev I’ve checked them as well and they look good to me. I am just sharing how I verified this locally, since it’s easy to accidentally copy-paste something incorrectly and not notice it: I compared the statically initialized version with the previous version before deleting the old tables from the code. |
colesbury left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
bedevere-bot commented Dec 3, 2025
🤖 New build scheduled with the buildbot fleet by @colesbury for commit 64254ba 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F142161%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
colesbury commented Dec 4, 2025
Buildbot failures are unrelated |
2dac9e6 into python:mainUh oh!
There was an error while loading. Please reload this page.
Thanks @yoney for the PR, and @colesbury for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14. |
…ule (pythongh-142161) The initialization during `mod_exec` wasn't thread-safe with multiple interpreters. (cherry picked from commit 2dac9e6) Co-authored-by: Alper <alperyoney@fb.com>
GH-142261 is a backport of this pull request to the 3.14 branch. |
…ule (pythongh-142161) The initialization during `mod_exec` wasn't thread-safe with multiple interpreters.
While investigating thread safety in the
cmathmodule, I found that the module is generally thread-safe. However, there is a potential race condition during the initialization of the special value tables incmath_exec(). This is usually safe, but with subinterpreters, a race condition could occur.The fix converts all special value tables from runtime initialization to static initialization, eliminating the race condition.
I have also created a test to run under ThreadSanitizer. However, I am not including the test in the current PR because, although this PR addresses the issue in the
cmathmodule, ThreadSanitizer still reports other issues below when subinterpreters and free-threading are used together. I plan to address those issues.cc: @mpage@colesbury