| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
…n in free-threading build len(), repr() and hash() on a frozendict read internal state without synchronization. That is safe for a finished frozendict but not while one is being constructed from a user mapping, where the half-built object is already observable from other threads. Read ma_used through GET_USED() and run the repr/hash entry-table walk inside a critical section, matching dict_repr / set_repr and set_len; the cached hash short-circuit stays lock-free like frozenset_hash.
|
Let me take a look at better solution if possible, cc @vstinner |
Sorry, something went wrong.
|
I prefer #151740 rather than this PR. |
Sorry, something went wrong.
|
We must not protect against concurrent access to frozendict objects, since frozendict is read-only. The right fix is to only track a frozendict once it's fully initialized: that's what #151740 does. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
In the free-threading build, len(fd), repr(fd) and hash(fd) on a
frozendict (PEP 814) read the object's internal state without
synchronization. That is safe for a finished frozendict, but not while one is
being constructed from a user mapping: the half-built object is already
GC-tracked and reachable from other threads (the generic
frozendict_new → dict_merge path runs user keys()/__getitem__, which can
leak it via gc.get_objects()), so a reader races the construction-time inserts
and resizes. ThreadSanitizer reports a data race, and a reader can observe the
frozendict's length and contents change, which contradicts its immutability.
What
the frozendict_hash active-entry factor now read ma_used through
GET_USED() (FT_ATOMIC_LOAD_SSIZE_RELAXED), matching dict_length and
set_len.
(anydict_repr_impl) inside Py_BEGIN_CRITICAL_SECTION, exactly like
dict_repr and like set_repr (the tp_repr of set/frozenset).
entry-table walk) wrapped in Py_BEGIN_CRITICAL_SECTION, with the cached
ma_hash short-circuit left lock-free (atomic load), mirroring
frozenset_hash/frozenset_hash_impl.
Why this matches the existing conventions
These are not new design choices; they mirror what dictobject.c and
setobject.c already do:
set_repr wraps set_repr_lock_held the same way, and it is the tp_repr
for both PySet_Type and PyFrozenSet_Type — so a frozen type already uses a
critical-sectioned repr.
holds Py_BEGIN_CRITICAL_SECTION(a) on the target in the generic dict_merge
branch, and inserts run under it (setitem_lock_held). A reader that takes
the object's critical section therefore serializes against construction.
short-circuit lock-free with an atomic load keeps that parity.
set_len.
The construction (write) side already uses STORE_USED
(FT_ATOMIC_STORE_SSIZE_RELAXED) and a critical section, and is unchanged.
Tests / verification
On a free-threading + ThreadSanitizer build
(./configure --disable-gil --with-thread-sanitizer), HEAD
9688d252d330b0b586760a121ee8c8f7215176e8:
being constructed (leaked via gc.get_objects() from __getitem__) reports
zero ThreadSanitizer warnings with this change, where unpatched main
reports data races on all three.
__eq__) were already free-threading-safe and remain clean.
test_free_threading all pass on the same build; test_dict passes.
Note on frozendict copy
copy_lock_held was deliberately made lock-free for frozendict sources in
#145920. It reads source state the same way and so has the same latent
construction-time race, but re-adding a critical section there would revert that
performance decision, so this PR does not touch the copy table walk (only the
scalar ma_used guard, which is a pure atomicity fix). The construction-window
question for copy is discussed in the linked issue.