fix: data race on last_storage_ptr_ cache in gil_safe_call_once_and_store#6087
Draft
henryiii wants to merge 1 commit into
Draft
fix: data race on last_storage_ptr_ cache in gil_safe_call_once_and_store#6087henryiii wants to merge 1 commit into
henryiii wants to merge 1 commit into
Conversation
…tore Under free-threaded CPython (Py_GIL_DISABLED) the GIL provides no mutual exclusion, so the plain pointer last_storage_ptr_ was read and written concurrently without synchronization, a C++ data race. Make it a std::atomic<T *>. Also reorder get_stored() to check is_last_storage_valid() before loading the cached pointer. The writer publishes the pointer before setting the validity flag, so the flag must be observed first for correct acquire/release ordering. Assisted-by: ClaudeCode:claude-fable-5
5 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 AI text below 🤖
Problem
In
include/pybind11/gil_safe_call_once.h, thePYBIND11_HAS_SUBINTERPRETER_SUPPORTbranch ofgil_safe_call_once_and_storecaches the per-interpreter storage pointer inT *last_storage_ptr_as a fast path for the single-interpreter case.This plain pointer is:
call_once_and_store_result()(inside thestd::call_oncelambda) and inget_stored(), andget_stored().Under free-threaded CPython (
Py_GIL_DISABLED),gil_scoped_acquireprovides no mutual exclusion, so these concurrent unsynchronized reads/writes of a plain pointer are a C++ data race (undefined behavior).Additionally,
get_stored()loaded the cached pointer before callingis_last_storage_valid(). The writer publishes the pointer (last_storage_ptr_) before setting the validity flag (is_initialized_by_at_least_one_interpreter_), so a reader must observe the flag first and only then load the pointer to get correct acquire/release ordering.Fix
last_storage_ptr_astd::atomic<T *>(<atomic>is already included in this branch). Default seq_cst operations pair with the existing atomic validity flag.get_stored()to checkis_last_storage_valid()first, and only loadlast_storage_ptr_on the fast path; on the slow path, look up the per-interpreter storage and update the cache as before.The change is minimal and behavior-preserving on the non-free-threaded build.
Not addressed here
The separate embedded finalize / re-init staleness concern (a stale
last_storage_ptr_surviving interpreter finalize + re-init within the same process) is not fixed by this PR. It is tracked separately in the issue.Verification
Compiled a scratch translation unit including
<pybind11/gil_safe_call_once.h>and instantiatingpybind11::gil_safe_call_once_and_store<int>withc++ -std=c++17 -fsyntax-onlyagainst Python 3.14 (the subinterpreter branch is active by default for Python >= 3.12). Compiles cleanly.clang-formatapplied.Part of #6084