-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
gh-142495: make defaultdict keep existed value when racing with __missing__
#142668
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
base: main
Are you sure you want to change the base?
Conversation
b57dd36 to
57064dc
Compare
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it need threads to reproduce? Would not the following code be enough to reproduce the issue?
count = 0
def default_factory():
nonlocal count
count += 1
if count == 1:
test_dict[key]
return count
Hi @serhiy-storchaka , Thanks very much for your suggestion! ❤ Yes, this case could be designed simply without threads. I have updated the test case as suggested. Best Regards, |
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! But similar tests usually use a local function with a nonlocal variable.
Hi @serhiy-storchaka , Thanks very much for your suggestion! 😊 Best Regards, |
| } | ||
| return value; | ||
| PyObject *result = NULL; | ||
| (void)PyDict_SetDefaultRef(op, key, value, &result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't need the return value, could we use PyDict_SetDefault?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PyDict_SetDefault returns a borrowed reference and I find this easier to read (at least the result is know to be a strong reference and we can directly return it).
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the new test reproduce the issue? It seems that __missing__() is not called recursively in your current code, unlike to the code in my suggestion.
Issue
#142495
Proposed Changes
PyDict_SetDefaultRefto guard the object update and keep existed value.Comment
The
PyDict_SetDefaultRefwould increase the reference count of the setted item, so we must decrement the refrence count on__missing__created object to avoid memory leak.