gh-117657: Fix data races in the method cache in free-threaded builds by mpage · Pull Request #117954 · python/cpython · GitHub

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:

Issue: Make TSAN tests pass with the GIL disabled in free-threaded builds #117657

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

LGTM!

bedevere-app Bot mentioned this pull request Apr 17, 2024

Make TSAN tests pass with the GIL disabled in free-threaded builds #117657

Closed

mpage added the skip news label Apr 17, 2024

mpage requested review from DinoV and colesbury April 17, 2024 03:55

mpage marked this pull request as ready for review April 17, 2024 03:55

mpage requested a review from markshannon as a code owner April 17, 2024 03:55

bedevere-app Bot added the awaiting review label Apr 17, 2024

DinoV reviewed Apr 17, 2024

View reviewed changes

DinoV merged commit b6c62c7 into python:main Apr 17, 2024

bedevere-app Bot removed the awaiting review label Apr 17, 2024

gh-117657: Fix data races in the method cache in free-threaded builds by mpage · Pull Request #117954 · python/cpython · GitHub

Uh oh!

Uh oh!

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

gh-117657: Fix data races in the method cache in free-threaded builds#117954
DinoV merged 1 commit into
python:mainfrom
mpage:gh-117657-update_cache

mpage commented Apr 17, 2024 •
edited
Loading

Uh oh!

Uh oh!

DinoV left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	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));
	}

	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);

gh-117657: Fix data races in the method cache in free-threaded builds by mpage · Pull Request #117954 · python/cpython · GitHub

Uh oh!

Uh oh!

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

gh-117657: Fix data races in the method cache in free-threaded builds#117954DinoV merged 1 commit intopython:mainpython/cpython:mainfrom mpage:gh-117657-update_cachempage/cpython:gh-117657-update_cacheCopy head branch name to clipboard

Conversation

mpage commented Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DinoV left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gh-117657: Fix data races in the method cache in free-threaded builds#117954
DinoV merged 1 commit into
python:mainfrom
mpage:gh-117657-update_cache

mpage commented Apr 17, 2024 •
edited
Loading