Skip to content

Commit 7569c3b

Browse files
pythongh-157217: Fix dir() race when merging from a mappingproxy
Unwrap mappingproxy when it wraps a dict so dict_merge uses the locked dict-to-dict path. Add mappingproxy merge tests and a free-threading dir() race regression test.
1 parent 23180c5 commit 7569c3b

4 files changed

Lines changed: 99 additions & 5 deletions

File tree

‎Lib/test/test_dict_mappingproxy.py‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
"""Tests for dict() / update / unpacking of types.MappingProxyType.
2+
3+
gh-157217: merging from a mappingproxy wrapping a dict should take the
4+
locked dict-to-dict path instead of iterating the proxy unlocked.
5+
"""
6+
7+
import collections
8+
import types
9+
import unittest
10+
11+
12+
class MappingProxyDictMergeTests(unittest.TestCase):
13+
def test_update_from_mappingproxy_dict(self):
14+
d = {}
15+
d.update(types.MappingProxyType({1: 1, 2: 2, 3: 3}))
16+
self.assertEqual(d, {1: 1, 2: 2, 3: 3})
17+
18+
def test_dict_constructor_from_mappingproxy(self):
19+
view = types.MappingProxyType({'a': 1, 'b': 2})
20+
self.assertEqual(dict(view), {'a': 1, 'b': 2})
21+
self.assertEqual({**view}, {'a': 1, 'b': 2})
22+
dest = {'z': 0}
23+
dest.update(view)
24+
self.assertEqual(dest, {'z': 0, 'a': 1, 'b': 2})
25+
26+
def test_dict_constructor_from_mappingproxy_userdict(self):
27+
self.assertEqual(
28+
dict(types.MappingProxyType(collections.UserDict(x=1))),
29+
{'x': 1},
30+
)
31+
32+
33+
if __name__ == '__main__':
34+
unittest.main()

‎Lib/test/test_free_threading/test_type.py‎

Lines changed: 44 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
from threading import Barrier, Thread
66
from unittest import TestCase
77
import sys
8-
from test.support import import_helper, threading_helper
8+
from test.support import import_helper, threading_helper, Py_GIL_DISABLED
99

1010
_testinternalcapi = import_helper.import_module("_testinternalcapi")
1111

@@ -145,6 +145,7 @@ class ClassB(Base):
145145
# keep reference to __dict__
146146
d = obj.__dict__
147147
obj.__class__ = ClassB
148+
self.assertEqual(d['attr'], 123)
148149

149150

150151
def test_name_change(self):
@@ -359,7 +360,8 @@ class Base:
359360
pass
360361

361362
def setter():
362-
func = lambda self: "x"
363+
def func(self):
364+
return "x"
363365
barrier.wait()
364366
while not done:
365367
Base.__repr__ = func
@@ -401,6 +403,46 @@ class B(A):
401403
with threading_helper.start_threads(threads):
402404
pass
403405

406+
@unittest.skipUnless(Py_GIL_DISABLED,
407+
"race only occurs on the free-threaded build")
408+
def test_dir_racing_class_dict_insert(self):
409+
# gh-157217: dir() iterated a mappingproxy of the class dict without
410+
# holding that dict's critical section. A concurrent insert into the
411+
# class dict (for example a lazy __annotations_cache__) then raised
412+
# RuntimeError: dictionary changed size during iteration.
413+
errors = []
414+
415+
class C:
416+
x: int
417+
418+
for i in range(200):
419+
setattr(C, f'attr_{i}', i)
420+
421+
def reader():
422+
barrier.wait()
423+
for _ in range(400):
424+
try:
425+
dir(C)
426+
dict(vars(C))
427+
{**vars(C)}
428+
except RuntimeError as exc:
429+
errors.append(exc)
430+
431+
def writer():
432+
barrier.wait()
433+
# First access stores __annotations_cache__ on the class.
434+
C.__annotations__
435+
for i in range(200):
436+
setattr(C, f'extra_{i}', i)
437+
438+
n_readers = 4
439+
barrier = threading.Barrier(n_readers + 1)
440+
threads = [Thread(target=reader) for _ in range(n_readers)]
441+
threads.append(Thread(target=writer))
442+
with threading_helper.start_threads(threads):
443+
pass
444+
self.assertEqual(errors, [])
445+
404446

405447
if __name__ == "__main__":
406448
unittest.main()
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
Fix :func:`dir` raising :exc:`RuntimeError` on the free-threaded build
2+
when another thread concurrently inserts into a class ``__dict__``.
3+
``dict.update()`` and related merges now take the locked dict-to-dict path
4+
when the source is a :class:`types.MappingProxyType` wrapping a dict.

‎Objects/dictobject.c‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4305,11 +4305,25 @@ dict_merge(PyObject *a, PyObject *b, int override, PyObject **dupkey)
43054305

43064306
PyDictObject *mp = _PyAnyDict_CAST(a);
43074307

4308+
/* Mapping proxies (including type.__dict__) wrap a real dict. Unwrap
4309+
* so we take the locked dict-to-dict path instead of iterating the
4310+
* proxy without holding the underlying dict's critical section.
4311+
* Layout must match mappingproxyobject in descrobject.c. See gh-157217.
4312+
*/
4313+
typedef struct {
4314+
PyObject_HEAD
4315+
PyObject *mapping;
4316+
} mappingproxyobject;
4317+
PyObject *source = b;
4318+
if (Py_IS_TYPE(b, &PyDictProxy_Type)) {
4319+
source = ((mappingproxyobject *)b)->mapping;
4320+
}
4321+
43084322
int res = 0;
4309-
if (PyAnyDict_Check(b) && (Py_TYPE(b)->tp_iter == dict_iter)) {
4310-
PyDictObject *other = (PyDictObject*)b;
4323+
if (PyAnyDict_Check(source) && (Py_TYPE(source)->tp_iter == dict_iter)) {
4324+
PyDictObject *other = (PyDictObject*)source;
43114325
int res;
4312-
Py_BEGIN_CRITICAL_SECTION2(a, b);
4326+
Py_BEGIN_CRITICAL_SECTION2(a, source);
43134327
assert(can_modify_dict(mp));
43144328
res = dict_dict_merge((PyDictObject *)a, other, override, dupkey);
43154329
ASSERT_CONSISTENT(a);

0 commit comments

Comments
 (0)