gh-144981: Make PyUnstable_Code_SetExtra/GetExtra thread-safe#144980
gh-144981: Make PyUnstable_Code_SetExtra/GetExtra thread-safe#144980yoney wants to merge 2 commits intopython:mainfrom
Conversation
| o->co_extra = co_extra; | ||
| Py_BEGIN_CRITICAL_SECTION(o); | ||
|
|
||
| _PyCodeObjectExtra *old_extra = (_PyCodeObjectExtra *) o->co_extra; |
There was a problem hiding this comment.
This probably still needs to be relaxed as it races w/ the get
| co_extra->ce_extras[i] = NULL; | ||
| } | ||
|
|
||
| if (old_extra != NULL && index < old_size && |
There was a problem hiding this comment.
Maybe we want to grab this value and move the free outside of the critical section?
| // Lock-free read; pairs with release store in SetExtra. | ||
| _PyCodeObjectExtra *co_extra = FT_ATOMIC_LOAD_PTR_ACQUIRE(o->co_extra); | ||
| if (co_extra != NULL && index < co_extra->ce_size) { | ||
| *extra = co_extra->ce_extras[index]; |
There was a problem hiding this comment.
We probably want a relaxed read here too as it's racing with the writer
| Py_ssize_t new_size = old_size > index ? old_size : user_count; | ||
| assert(new_size > 0 && new_size > index); | ||
|
|
||
| // Free-threaded builds require copy-on-write to avoid mutating |
There was a problem hiding this comment.
Should this be run a if (new_size != old_size)? I don't think we need to copy the whole thing for this to work lock free if we're using the right atomics when reading the values.
| } | ||
| } | ||
|
|
||
| co_extra->ce_extras[index] = extra; |
There was a problem hiding this comment.
And this should probably be a relaxed store
|
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 |
PyUnstable_Code_GetExtraandPyUnstable_Code_SetExtrahad no thread-safety. Concurrent calls could cause data races.GetExtrais performance-sensitive for the use cases like JIT . It now uses an acquire load ofco_extraand plain reads of the immutable published buffer.SetExtrauses a per-object critical section and copy-on-write: it always allocates a new buffer, copies existing slots, and publishes with a release store. Old buffers are freed via_PyMem_FreeDelayedin free-threaded builds.PyUnstable_Eval_RequestCodeExtraIndexis now protected byinterp->code_state.mutex.RequestCodeExtraIndex,SetExtra, andGetExtra.The new tests exercise the concurrent paths and a free-threaded TSAN build without the fix crashes/emits warnings.
cc: @DinoV @colesbury