Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-117657: Fix data races in the method cache in free-threaded builds #117954

Merged
merged 1 commit into from
Apr 17, 2024

Conversation

mpage
Copy link
Contributor

@mpage mpage commented Apr 17, 2024

These are technically data races, but I think they're benign (to the extent that that is actually possible). We update cache entries non-atomically:

cpython/Objects/typeobject.c

Lines 4974 to 4984 in a23fa33

static void
update_cache(struct type_cache_entry *entry, PyObject *name, unsigned int version_tag, PyObject *value)
{
entry->version = version_tag;
entry->value = value; /* borrowed */
assert(_PyASCIIObject_CAST(name)->hash != -1);
OBJECT_STAT_INC_COND(type_cache_collisions, entry->name != Py_None && entry->name != name);
// We're releasing this under the lock for simplicity sake because it's always a
// exact unicode object or Py_None so it's safe to do so.
Py_SETREF(entry->name, Py_NewRef(name));
}

but read them atomically from another thread:

cpython/Objects/typeobject.c

Lines 5046 to 5051 in a23fa33

if (_Py_atomic_load_uint32_relaxed(&entry->version) == type->tp_version_tag &&
_Py_atomic_load_ptr_relaxed(&entry->name) == name) {
assert(_PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG));
OBJECT_STAT_INC_COND(type_cache_hits, !is_dunder_name(name));
OBJECT_STAT_INC_COND(type_cache_dunder_hits, is_dunder_name(name));
PyObject *value = _Py_atomic_load_ptr_relaxed(&entry->value);

and there's nothing that establishes a happens-before relationship between the reads and writes that I can see.

In practice I don't think this matters much. We care about always reading the entire entry atomically, and the sequence lock enforces that.

Sample races reported by TSAN:

These are technically data races, but I think they're benign (to
the extent that that is actually possible). We update cache entries
non-atomically but read them atomically from another thread, and there's
nothing that establishes a happens-before relationship between the
reads and writes that I can see.

Sample races reported by TSAN:

- https://gist.github.com/mpage/8633aacbf11303bdfdbda2c0e644d43d
- https://gist.github.com/mpage/70d5758968c0225384a2ace448122d09
- https://gist.github.com/mpage/33436b62af0d2e688f36c4ecb9171dee
Copy link
Contributor

@DinoV DinoV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@DinoV DinoV merged commit b6c62c7 into python:main Apr 17, 2024
36 checks passed
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.

2 participants