gh-151218: Fix data race in sys_set_flag for free-threading#151220
gh-151218: Fix data race in sys_set_flag for free-threading#151220bhuvi27 wants to merge 1 commit into
Conversation
2bff4e1 to
099518a
Compare
Concurrent calls to sys.set_int_max_str_digits() in free-threaded builds could double-free the same sys.flags tuple item because sys_set_flag() updated the slot without synchronization. Protect sys.flags updates with a mutex in free-threaded builds and hold the same lock across the flag and interpreter int_max_str_digits state updates so sys.get_int_max_str_digits() stays consistent with sys.flags.
099518a to
09f17c8
Compare
|
Please do not force push. To contribute cleanly, please read https://devguide.python.org/getting-started/pull-request-lifecycle/#pullrequest. |
|
This doesn't guard the read though. And the read through |
Good point — you're right that the read path through A few options I see:
Option 1 looks cleanest and matches the "immutable" intent, but it's |
|
This conversation seems to go against our AI policy. Human interaction is necessary, don't let your agents in automated mode. Please read https://devguide.python.org/getting-started/ai-tools/#guidelines-for-using-ai-tools. |
Sorry — I used an AI assistant to draft PR replies and posted them On the technical side: I understand the mutex doesn't protect reads |
skirpichev
left a comment
There was a problem hiding this comment.
I think that exposing int_max_str_digits in the sys.flags was a mistake. This is an immutable named tuple for command-line flags.
But sys.flags.int_max_str_digits does not fit in this picture: it's for runtime settings, which can be changed by sys.set_int_max_str_digits().
I think this bug should be fixed by changing meaning of the sys.flags.int_max_str_digits. It will keep immutable value, provided as command-line option. We have a different API to access runtime settings, i.e. sys.get_int_max_str_digits(). CC @gpshead
|
|
gpshead
left a comment
There was a problem hiding this comment.
overall I don't think this PR is the right way to accomplish this. we don't need a static global mutex. use Py_BEGIN_CRITICAL_SECTION on the flags object instead.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
I don't think that works because there's no thread-safety guards on any of the reads from the flags object (because it's a tuple-like object so considered immutable)
I do agree with this though |
Fixes #151218
Concurrent calls to sys.set_int_max_str_digits() in free-threaded builds
could double-free the same sys.flags tuple item because sys_set_flag()
updated the slot without synchronization.
Protect sys.flags updates with a mutex in free-threaded builds, and hold
the same lock across flag and interpreter int_max_str_digits state updates
so sys.get_int_max_str_digits() stays consistent with sys.flags. Add a
concurrent stress regression test in test_sys.py.
sys_set_flagwhensys.set_int_max_str_digits()is called concurrently with free-threaded build #151218