gh-142495: make `defaultdict` keep existed value when racing with `missing` #142668

LindaSummer · 2025-12-13T16:04:15Z

Issue

Proposed Changes

Use PyDict_SetDefaultRef to guard the object update and keep existed value.
Add a test case for the racing case

Comment

The PyDict_SetDefaultRef would increase the reference count of the setted item, so we must decrement the refrence count on __missing__ created object to avoid memory leak.

Issue: defaultdict.__missing__ has data races #142495

Modules/_collectionsmodule.c

serhiy-storchaka

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

LindaSummer · 2025-12-15T14:50:16Z

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,
Edward

serhiy-storchaka

Great! But similar tests usually use a local function with a nonlocal variable.

LindaSummer · 2025-12-15T15:01:58Z

Great! But similar tests usually use a local function with a nonlocal variable.

Hi @serhiy-storchaka ,

Thanks very much for your suggestion! 😊
Got it. I will refactor the test case with nonlocal variables for simplicity.

Best Regards,
Edward

chaope · 2025-12-15T15:57:25Z

Modules/_collectionsmodule.c

-    }
-    return value;
+    PyObject *result = NULL;
+    (void)PyDict_SetDefaultRef(op, key, value, &result);


If we don't need the return value, could we use PyDict_SetDefault?

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

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.

LindaSummer · 2025-12-16T13:48:48Z

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.

Hi @serhiy-storchaka ,

Thanks very much for your correction!❤

Sorry for misunderstanding the test case comment.
I have updated the test case.
I use a local copy of count to verify that the first time return value 1 of __missing__ does not overwrite the second time assigned value 2.

Please correct me if I misunderstand.

Best Regards,
Edward

serhiy-storchaka · 2025-12-16T14:08:53Z

Lib/test/test_defaultdict.py

+    def test_factory_conflict_with_set_value(self):
+        key = "conflict_test"
+        count = 0
+        test_dict = None


Is this needed? test_dict is defined at line 203.

Hi @serhiy-storchaka ,

Thanks for your correction! 😊
I have removed the redundant declaration of test_dict.

Best Regards,
Edward

serhiy-storchaka · 2025-12-16T14:09:31Z

Lib/test/test_defaultdict.py

+
+        def default_factory():
+            nonlocal count
+            nonlocal test_dict


This is not needed, as the variable is not modified in the function (the variable is bound to the same object, even if its value is changed).

Hi @serhiy-storchaka ,

Got it! 😊
I have removed this line.

Best Regards,
Edward

serhiy-storchaka

LGTM. 👍

Thank you for your contribution. In the future, try to use chatbots less (could you use a simple online translator instead?). They create an unpleasant, creepy impression.

miss-islington-app · 2025-12-16T15:05:24Z

Thanks @LindaSummer for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington-app · 2025-12-16T15:05:24Z

Thanks @LindaSummer for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

miss-islington-app · 2025-12-16T15:05:27Z

Sorry, @LindaSummer and @serhiy-storchaka, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker a0434075108efe6acdfba34f42545f4d80ac9a5e 3.13

…th `__missing__` (pythonGH-142668) (cherry picked from commit a043407) Co-authored-by: Edward Xu <[email protected]>

bedevere-app · 2025-12-16T15:05:41Z

GH-142832 is a backport of this pull request to the 3.14 branch.

LindaSummer · 2025-12-16T15:11:49Z

LGTM. 👍

Thank you for your contribution. In the future, try to use chatbots less (could you use a simple online translator instead?). They create an unpleasant, creepy impression.

Got it. Thanks for your review and help!
Haha, in fact I didn't use a chatbot and I try to write in email style because I thought it was polite and friendly.
I will improve my wording and avoid fixed patterns, which looks like a bot.

bedevere-bot · 2025-12-16T15:24:22Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Debian root 3.x (tier-1) has failed when building commit a043407.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/345/builds/12953) and take a look at the build logs.
Check if the failure is related to this commit (a043407) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/345/builds/12953

Summary of the results of the build (if available):

==

Click to see traceback logs

Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/test/support/__init__.py", line 935, in gc_collect
    gc.collect()
    ~~~~~~~~~~^^
ResourceWarning: unclosed file <_io.FileIO name=13 mode='wb' closefd=True>

…ith `__missing__` (GH-142668) (GH-142832) (cherry picked from commit a043407) Co-authored-by: Edward Xu <[email protected]>

LindaSummer requested a review from rhettinger as a code owner December 13, 2025 16:04

bedevere-app bot mentioned this pull request Dec 13, 2025

defaultdict.__missing__ has data races #142495

Open

bedevere-app bot added the awaiting review label Dec 13, 2025

picnixz reviewed Dec 13, 2025

View reviewed changes

Modules/_collectionsmodule.c Outdated Show resolved Hide resolved

LindaSummer added 7 commits December 14, 2025 10:34

add code and simple test case.

b50659a

add unit test.

c81a1d4

refactor logic and make it cleaner

4a2bc6c

fix lint issue

3496ffe

fix reference count leak

b9e5a8c

add news entry.

002b3a6

refactor redundant comment

57064dc

LindaSummer force-pushed the defaultdict branch from b57dd36 to 57064dc Compare December 14, 2025 02:43

LindaSummer requested a review from picnixz December 15, 2025 07:32

serhiy-storchaka reviewed Dec 15, 2025

View reviewed changes

LindaSummer and others added 3 commits December 15, 2025 22:10

remove thread from test case

6c5bbf7

Merge branch 'main' into defaultdict-pr-test

53a419b

refactor test case.

5a1783b

serhiy-storchaka reviewed Dec 15, 2025

View reviewed changes

LindaSummer added 2 commits December 15, 2025 23:05

simplify test case with nonlocal

78d0180

Merge branch 'main' of github.com:python/cpython into defaultdict

4536222

chaope reviewed Dec 15, 2025

View reviewed changes

serhiy-storchaka reviewed Dec 15, 2025

View reviewed changes

LindaSummer added 2 commits December 16, 2025 21:11

Merge remote-tracking branch 'upstream/main' into defaultdict

e17b229

refactor test case with recursive __missing__ invoked

2bc82af

serhiy-storchaka reviewed Dec 16, 2025

View reviewed changes

remove redundant decalaration of test_dict.

149c88b

serhiy-storchaka approved these changes Dec 16, 2025

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting review labels Dec 16, 2025

serhiy-storchaka merged commit a043407 into python:main Dec 16, 2025
50 checks passed

bedevere-app bot removed the awaiting merge label Dec 16, 2025

serhiy-storchaka added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels Dec 16, 2025

miss-islington-app bot assigned serhiy-storchaka Dec 16, 2025

bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Dec 16, 2025

serhiy-storchaka pushed a commit that referenced this pull request Dec 16, 2025

[3.14] gh-142495: Make defaultdict keep existed value when racing w…

d089cad

…ith `__missing__` (GH-142668) (GH-142832) (cherry picked from commit a043407) Co-authored-by: Edward Xu <[email protected]>

Uh oh!

gh-142495: make defaultdict keep existed value when racing with __missing__ #142668

gh-142495: make defaultdict keep existed value when racing with __missing__ #142668

Conversation

LindaSummer commented Dec 13, 2025 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Proposed Changes

Comment

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

LindaSummer commented Dec 15, 2025

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

LindaSummer commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chaope Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

picnixz Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

LindaSummer commented Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

LindaSummer Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

LindaSummer Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

miss-islington-app bot commented Dec 16, 2025

Uh oh!

miss-islington-app bot commented Dec 16, 2025

Uh oh!

miss-islington-app bot commented Dec 16, 2025

Uh oh!

bedevere-app bot commented Dec 16, 2025

Uh oh!

LindaSummer commented Dec 16, 2025

Uh oh!

bedevere-bot commented Dec 16, 2025

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gh-142495: make `defaultdict` keep existed value when racing with `missing` #142668

gh-142495: make `defaultdict` keep existed value when racing with `missing` #142668

LindaSummer commented Dec 13, 2025 •

edited by bedevere-app bot

Loading

LindaSummer commented Dec 15, 2025 •

edited

Loading

LindaSummer commented Dec 16, 2025 •

edited

Loading