Skip to content

gh-151722: Fix data race on frozendict reads during construction in free-threading build#151724

Open
tonghuaroot wants to merge 2 commits into
python:mainfrom
tonghuaroot:fix-frozendict-ft-read-race
Open

gh-151722: Fix data race on frozendict reads during construction in free-threading build#151724
tonghuaroot wants to merge 2 commits into
python:mainfrom
tonghuaroot:fix-frozendict-ft-read-race

Conversation

@tonghuaroot

Copy link
Copy Markdown
Contributor

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_newdict_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

  • frozendict_length, the frozendict_repr/copy_lock_held empty-guards, and
    the frozendict_hash active-entry factor now read ma_used through
    GET_USED() (FT_ATOMIC_LOAD_SSIZE_RELAXED), matching dict_length and
    set_len.
  • frozendict_repr now runs the _PyDict_Next entry-table walk
    (anydict_repr_impl) inside Py_BEGIN_CRITICAL_SECTION, exactly like
    dict_repr and like set_repr (the tp_repr of set/frozenset).
  • frozendict_hash is split into a frozendict_hash_lock_held helper (the
    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:

  • dict_repr wraps anydict_repr_impl in Py_BEGIN_CRITICAL_SECTION(self).
    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.
  • The critical section is effective here because the construction side already
    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.
  • frozendict_hash was copied from frozenset_hash; keeping the cached-hash
    short-circuit lock-free with an atomic load keeps that parity.
  • Scalar ma_used reads going through GET_USED() matches dict_length and
    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:

  • A reader thread hammering len/repr/hash on a frozendict that is still
    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.
  • The lock-free read fast paths (__getitem__, get, in, iteration,
    __eq__) were already free-threading-safe and remain clean.
  • FrozenDictTests / FrozenDictMappingTests (21 tests), test_set and
    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.

…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant