gh-140551: Fix dict crash if clear is called at lookup stage

[efimov-mikhail]

• Issue: Crash in dict if hash __eq__ function has side effects #140551

[sergey-miryanov]

For the following repro:

d = {}

class X(object):
    def __hash__(self):
        return 1
    def __eq__(self, other):
        d.clear()

d.setdefault(X(), None)
d.setdefault(X(), None)
d.setdefault(X(), None)

assert len(d) == 1
print(d)

It crashes inside dict_setdefault_ref_lock_held -> insertion_resize -> dictresize -> unicode_get_hash

[sergey-miryanov]

For the following repro:

d = {}

class X(object):
    def __hash__(self):
        return 1
    def __eq__(self, other):
        d.clear()

d.setdefault(X(), None)
d.setdefault(X(), None)

assert len(d) == 1
print(d)

It crashes also inside unicode_get_hash but for line print(d).

Both repro tests on Windows 11 on main branch.

[sergey-miryanov]

Another one I found on main:

d = {}

class X(object):
    def __hash__(self):
        return 1
    def __eq__(self, other):
        d.clear()

d[X()] = None
d[X()] = None
for _ in range(10):
    d.pop(X(), None)

It is aborted with Assertion failed: PyUnicode_CheckExact(ep->me_key), file X:\Work\cpython\main\Objects\dictobject.c, line 1048

[efimov-mikhail]

Another one I found on main:

d[X()] = None
d[X()] = None
for _ in range(10):
    d.pop(X(), None)

Yes, it's expected that d is in incorrect state after two assignments.
There're many ways to trigger assertion at that state.
All of them have been fixed with this PR, I suppose.

[sergey-miryanov]

LGTM

[methane]

I'm not sure if this is the correct solution.

If the keys of a dictionary change during a dict lookup, can we trust the result of the lookup (e.g. ix)?

Shouldn't it raise an error if the keys of a dict change during a lookup?

[vstinner]

I like this error:

            PyErr_SetString(PyExc_RuntimeError,
                    "dict mutated during update");

[colesbury]

If the keys of a dictionary change during a dict lookup, can we trust the result of the lookup

CPython behavior has been to retry the lookup if the key changes (since Python 2.2, 453163d). I don't think we should change it.

[colesbury]

LGTM, but I will let @methane review and merge

[sergey-miryanov]

Looks good to me. I'm not expert, but code looks clear and tests seem actual.

[efimov-mikhail]

IIUC, iOS problem in CI is unrelated to this PR, see: #140190 (comment)

[vstinner]

IIUC, iOS problem in CI is unrelated to this PR, see: #140190 (comment)

I created #140702 for test_UnixDatagramServer() timeout.

[efimov-mikhail]

I created #140702 for test_UnixDatagramServer() timeout.

IIUC, problem is fixed and CI is green.

[miss-islington-app]

Thanks @efimov-mikhail for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14.
🐍🍒⛏🤖

[miss-islington-app]

Sorry, @efimov-mikhail and @kumaraditya303, I could not cleanly backport this to 3.14 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 02202c117b5702f3325e62b07ccdeaa125fc8722 3.14

[miss-islington-app]

Sorry, @efimov-mikhail and @kumaraditya303, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 02202c117b5702f3325e62b07ccdeaa125fc8722 3.13

[efimov-mikhail]

Thanks for the merge, @kumaraditya303.
I'll make backports manually.

[bedevere-app]

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

[bedevere-app]

GH-140744 is a backport of this pull request to the 3.13 branch.

[efimov-mikhail]

FYI, @kumaraditya303
The backports are ready and have green CI.