Skip to content

Commit a89b241

Browse files
[3.14] gh-148393: Use atomic ops on _ma_watcher_tag in free threading build (gh-148397) (#148451)
Fixes data races between dict mutation and watch/unwatch on the same dict. (cherry picked from commit 3ab94d6) Co-authored-by: Sam Gross <colesbury@gmail.com>
1 parent a9d122f commit a89b241

File tree

6 files changed

+44
-6
lines changed

6 files changed

+44
-6
lines changed

Include/internal/pycore_dict.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -286,7 +286,7 @@ _PyDict_NotifyEvent(PyInterpreterState *interp,
286286
PyObject *value)
287287
{
288288
assert(Py_REFCNT((PyObject*)mp) > 0);
289-
int watcher_bits = mp->_ma_watcher_tag & DICT_WATCHER_MASK;
289+
int watcher_bits = FT_ATOMIC_LOAD_UINT64_RELAXED(mp->_ma_watcher_tag) & DICT_WATCHER_MASK;
290290
if (watcher_bits) {
291291
RARE_EVENT_STAT_INC(watched_dict_modification);
292292
_PyDict_SendEvent(watcher_bits, event, mp, key, value);
@@ -362,7 +362,7 @@ PyDictObject *_PyObject_MaterializeManagedDict_LockHeld(PyObject *);
362362
static inline Py_ssize_t
363363
_PyDict_UniqueId(PyDictObject *mp)
364364
{
365-
return (Py_ssize_t)(mp->_ma_watcher_tag >> DICT_UNIQUE_ID_SHIFT);
365+
return (Py_ssize_t)(FT_ATOMIC_LOAD_UINT64_RELAXED(mp->_ma_watcher_tag) >> DICT_UNIQUE_ID_SHIFT);
366366
}
367367

368368
static inline void

Include/internal/pycore_pyatomic_ft_wrappers.h

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,8 @@ extern "C" {
4747
_Py_atomic_load_uint16_relaxed(&value)
4848
#define FT_ATOMIC_LOAD_UINT32_RELAXED(value) \
4949
_Py_atomic_load_uint32_relaxed(&value)
50+
#define FT_ATOMIC_LOAD_UINT64_RELAXED(value) \
51+
_Py_atomic_load_uint64_relaxed(&value)
5052
#define FT_ATOMIC_LOAD_ULONG_RELAXED(value) \
5153
_Py_atomic_load_ulong_relaxed(&value)
5254
#define FT_ATOMIC_STORE_PTR_RELAXED(value, new_value) \
@@ -63,6 +65,12 @@ extern "C" {
6365
_Py_atomic_store_uint16_relaxed(&value, new_value)
6466
#define FT_ATOMIC_STORE_UINT32_RELAXED(value, new_value) \
6567
_Py_atomic_store_uint32_relaxed(&value, new_value)
68+
#define FT_ATOMIC_AND_UINT64(value, new_value) \
69+
(void)_Py_atomic_and_uint64(&value, new_value)
70+
#define FT_ATOMIC_OR_UINT64(value, new_value) \
71+
(void)_Py_atomic_or_uint64(&value, new_value)
72+
#define FT_ATOMIC_ADD_UINT64(value, new_value) \
73+
(void)_Py_atomic_add_uint64(&value, new_value)
6674
#define FT_ATOMIC_STORE_CHAR_RELAXED(value, new_value) \
6775
_Py_atomic_store_char_relaxed(&value, new_value)
6876
#define FT_ATOMIC_LOAD_CHAR_RELAXED(value) \
@@ -139,6 +147,7 @@ extern "C" {
139147
#define FT_ATOMIC_LOAD_UINT8_RELAXED(value) value
140148
#define FT_ATOMIC_LOAD_UINT16_RELAXED(value) value
141149
#define FT_ATOMIC_LOAD_UINT32_RELAXED(value) value
150+
#define FT_ATOMIC_LOAD_UINT64_RELAXED(value) value
142151
#define FT_ATOMIC_LOAD_ULONG_RELAXED(value) value
143152
#define FT_ATOMIC_STORE_PTR_RELAXED(value, new_value) value = new_value
144153
#define FT_ATOMIC_STORE_PTR_RELEASE(value, new_value) value = new_value
@@ -148,6 +157,9 @@ extern "C" {
148157
#define FT_ATOMIC_STORE_UINT8_RELAXED(value, new_value) value = new_value
149158
#define FT_ATOMIC_STORE_UINT16_RELAXED(value, new_value) value = new_value
150159
#define FT_ATOMIC_STORE_UINT32_RELAXED(value, new_value) value = new_value
160+
#define FT_ATOMIC_AND_UINT64(value, new_value) (void)(value &= new_value)
161+
#define FT_ATOMIC_OR_UINT64(value, new_value) (void)(value |= new_value)
162+
#define FT_ATOMIC_ADD_UINT64(value, new_value) (void)(value += new_value)
151163
#define FT_ATOMIC_LOAD_CHAR_RELAXED(value) value
152164
#define FT_ATOMIC_STORE_CHAR_RELAXED(value, new_value) value = new_value
153165
#define FT_ATOMIC_LOAD_UCHAR_RELAXED(value) value

Lib/test/test_free_threading/test_dict.py

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,29 @@ def reader():
245245
with threading_helper.start_threads([t1, t2]):
246246
pass
247247

248+
@unittest.skipIf(_testcapi is None, "requires _testcapi")
249+
def test_racing_watch_unwatch_dict(self):
250+
# gh-148393: race between PyDict_Watch / PyDict_Unwatch
251+
# and concurrent dict mutation reading _ma_watcher_tag.
252+
wid = _testcapi.add_dict_watcher(0)
253+
try:
254+
d = {}
255+
ITERS = 1000
256+
257+
def writer():
258+
for i in range(ITERS):
259+
d[i] = i
260+
del d[i]
261+
262+
def watcher():
263+
for _ in range(ITERS):
264+
_testcapi.watch_dict(wid, d)
265+
_testcapi.unwatch_dict(wid, d)
266+
267+
threading_helper.run_concurrently([writer, watcher])
268+
finally:
269+
_testcapi.clear_dict_watcher(wid)
270+
248271
def test_racing_dict_update_and_method_lookup(self):
249272
# gh-144295: test race between dict modifications and method lookups.
250273
# Uses BytesIO because the race requires a type without Py_TPFLAGS_INLINE_VALUES
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
Fix data races between :c:func:`PyDict_Watch` / :c:func:`PyDict_Unwatch`
2+
and concurrent dict mutation in the :term:`free-threaded build`.

Objects/dictobject.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7718,7 +7718,7 @@ PyDict_Watch(int watcher_id, PyObject* dict)
77187718
if (validate_watcher_id(interp, watcher_id)) {
77197719
return -1;
77207720
}
7721-
((PyDictObject*)dict)->_ma_watcher_tag |= (1LL << watcher_id);
7721+
FT_ATOMIC_OR_UINT64(((PyDictObject*)dict)->_ma_watcher_tag, (1LL << watcher_id));
77227722
return 0;
77237723
}
77247724

@@ -7733,7 +7733,7 @@ PyDict_Unwatch(int watcher_id, PyObject* dict)
77337733
if (validate_watcher_id(interp, watcher_id)) {
77347734
return -1;
77357735
}
7736-
((PyDictObject*)dict)->_ma_watcher_tag &= ~(1LL << watcher_id);
7736+
FT_ATOMIC_AND_UINT64(((PyDictObject*)dict)->_ma_watcher_tag, ~(1LL << watcher_id));
77377737
return 0;
77387738
}
77397739

Python/optimizer_analysis.c

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,14 +54,15 @@ static int
5454
get_mutations(PyObject* dict) {
5555
assert(PyDict_CheckExact(dict));
5656
PyDictObject *d = (PyDictObject *)dict;
57-
return (d->_ma_watcher_tag >> DICT_MAX_WATCHERS) & ((1 << DICT_WATCHED_MUTATION_BITS)-1);
57+
uint64_t tag = FT_ATOMIC_LOAD_UINT64_RELAXED(d->_ma_watcher_tag);
58+
return (tag >> DICT_MAX_WATCHERS) & ((1 << DICT_WATCHED_MUTATION_BITS) - 1);
5859
}
5960

6061
static void
6162
increment_mutations(PyObject* dict) {
6263
assert(PyDict_CheckExact(dict));
6364
PyDictObject *d = (PyDictObject *)dict;
64-
d->_ma_watcher_tag += (1 << DICT_MAX_WATCHERS);
65+
FT_ATOMIC_ADD_UINT64(d->_ma_watcher_tag, (1 << DICT_MAX_WATCHERS));
6566
}
6667

6768
/* The first two dict watcher IDs are reserved for CPython,

0 commit comments

Comments
 (0)