Uh oh!
There was an error while loading. Please reload this page.
gh-151722: Defer GC tracking of frozendict to end of construction - #151740
Conversation
TSAN scriptimportthreading, gcN=4000stop=threading.Event()
box= []
defreader():
whilenotstop.is_set():
ifbox:
o=box[0]
try:
len(o); hash(o); repr(o)
exceptException:
passclassEvil:
def__init__(self):
self.n=0defkeys(self):
return [f"k{i}"foriinrange(N)]
def__getitem__(self, k):
self.n+=1ifself.n==10andnotbox: # one scan/construction; k0..k8 presentforoingc.get_objects():
iftype(o) isfrozendictand"k0"inoandlen(o) <N:
box.append(o)
breakreturn1t=threading.Thread(target=reader, daemon=True)
t.start()
for_inrange(40):
box.clear()
frozendict(Evil())
stop.set()
t.join(timeout=2)
print("observed half-built:", bool(box))AS-ISTO-BE |
corona10
commented
Jun 19, 2026
cc @tonghuaroot |
tonghuaroot
commented
Jun 20, 2026
Thanks for the cc. Built the PR ( Two sibling creation paths the defer-track doesn't reach yet, both pre-existing + TSan-confirmed on this branch:
Also no committed test yet — the fromkeys one above doubles as a |
| if (self == NULL) { | ||
| return NULL; | ||
| } | ||
| if (!_PyObject_GC_IS_TRACKED(self)) { |
corona10
commented
Jun 20, 2026
|
| } | ||
| static PyObject * | ||
| dict_new(PyTypeObject *type, PyObject *args, PyObject *kwds) |
There was a problem hiding this comment.
This function is strange. Why does it ignore its parameters?
Creating a dict with arguments should create a dict from those arguments.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Lines 5245 to 5269 in aa5b164
The function already ignore those params, and it was done by dict_init
There was a problem hiding this comment.
Indeed:
>>>dict.__new__(dict, (), a=1,b=2)
{}but frozendict is inconsistent:
>>>frozendict.__new__(frozendict, (), a=1,b=2)
frozendict({'a': 1, 'b': 2})There was a problem hiding this comment.
I think that than we need to modify frozenset also
>>> set.__new__(set, [1,2,3])
set()
>>> frozenset.__new__(frozenset, [1,2,3])
frozenset({1, 2, 3})
Let's handle this at separate issue.
There was a problem hiding this comment.
I don't see what's wrong here. For me, it works as expected:
>>> dict((), a=1,b=2)
{'a': 1, 'b': 2}
>>> frozendict((), a=1,b=2)
frozendict({'a': 1, 'b': 2})
The fact that arguments are used by __new__() or __init__() is just an implementation detail. I don't think that it matters for the "constructor" API.
markshannon
commented
Jun 20, 2026
See #151722 (comment) for the proper fix for the issue. Having said that, deferring tracking of any object until the object is complete is a good idea. |
corona10
commented
Jun 20, 2026
@markshannon@methane I've updated the PR. PTAL :) |
| STORE_USED(mp, other->ma_used); | ||
| ASSERT_CONSISTENT(mp); | ||
| if (_PyObject_GC_IS_TRACKED(other) && !_PyObject_GC_IS_TRACKED(mp)) { |
There was a problem hiding this comment.
No more lazy track from here, we can remove this logic safely.
There was a problem hiding this comment.
This removal makes me a little bit nervous. You may add the following assertion at the top of dict_dict_merge() (after other assertions).
if (PyDict_Check(mp)) {
assert(_PyObject_GC_IS_TRACKED(mp));
}
| PyObject *self = frozendict_new(_PyType_CAST(type), NULL, NULL); | ||
| /* Keep the frozendict untracked until it is fully built, so a half-built | ||
| object is never reachable from another thread. */ |
There was a problem hiding this comment.
| objectisneverreachablefromanotherthread. */ | |
| objectisneverreachablefromanotherthread (usingthegcmodule). */ |
You may add a reference to the issue gh-151722.
| static PyObject * | ||
| dict_new(PyTypeObject *type, PyObject *Py_UNUSED(args), PyObject *Py_UNUSED(kwds)) | ||
| { | ||
| /* tp_new only allocates; args/kwds are consumed by dict_init (tp_init). */ |
There was a problem hiding this comment.
| /* tp_new only allocates; args/kwds are consumed by dict_init (tp_init). */ | |
| /* tp_new ignores args/kwds; args/kwds are consumed by dict_init (tp_init). */ |
| STORE_USED(mp, other->ma_used); | ||
| ASSERT_CONSISTENT(mp); | ||
| if (_PyObject_GC_IS_TRACKED(other) && !_PyObject_GC_IS_TRACKED(mp)) { |
There was a problem hiding this comment.
This removal makes me a little bit nervous. You may add the following assertion at the top of dict_dict_merge() (after other assertions).
if (PyDict_Check(mp)) {
assert(_PyObject_GC_IS_TRACKED(mp));
}
Uh oh!
There was an error while loading. Please reload this page.
Thanks @corona10 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.15. |
GH-151954 is a backport of this pull request to the 3.15 branch. |
Uh oh!
There was an error while loading. Please reload this page.