Skip to content
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

gh-118331: Handle errors in _PyObject_SetManagedDict #118334

Merged
merged 2 commits into from Apr 29, 2024

Conversation

colesbury
Copy link
Contributor

@colesbury colesbury commented Apr 26, 2024

When detaching a dict, the copy_values call may fail due to out-of-memory errors. This can be triggered by test_no_memory in test_repl.

When detaching a dict, the copy_values call may fail due to
out-of-memory errors. This can be triggered by test_no_memory in
test_repl.
@hugovk
Copy link
Member

hugovk commented Apr 26, 2024

Running this on this branch:

make clean
git clean -fdx .
./configure --with-pydebug && make -j10
./python.exe Lib/test/test_repl.py

I still get the error:

.....F.
======================================================================
FAIL: test_no_memory (__main__.TestInteractiveInterpreter.test_no_memory)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/private/tmp/cpython/Lib/test/test_repl.py", line 86, in test_no_memory
    self.assertIn(p.returncode, (1, 120))
    ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: -6 not found in (1, 120)

----------------------------------------------------------------------
Ran 7 tests in 0.331s

FAILED (failures=1)

@colesbury
Copy link
Contributor Author

Hmmm... maybe there are multiple unhandled exceptions?

I haven't yet been able to reproduce additional failures with this PR.

@mpage
Copy link
Contributor

mpage commented Apr 26, 2024

My experience is the same as @colesbury:

I tested on both fedora and MacOS.

@hugovk
Copy link
Member

hugovk commented Apr 26, 2024

It's really strange, I can repro on main with two different Macs: my work one (which is at work until Monday) and this one.

I can also repro with this branch and #118336.

@hugovk
Copy link
Member

hugovk commented Apr 27, 2024

gh-118331 fixes it for me, and also with that merged into this. Thanks!

Copy link
Contributor

@DinoV DinoV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!


err = _PyDict_DetachFromObject(dict, obj);
if (err == 0) {
FT_ATOMIC_STORE_PTR(_PyObject_ManagedDictPointer(obj)->dict,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're no longer assigning this before calling _PyDict_DetachFromObject we can bring back some of the assertions in _PyDict_DetachFromObject, specifically:

    assert(_PyObject_ManagedDictPointer(obj)->dict == mp);
    assert(_PyObject_InlineValuesConsistencyCheck(obj));

I think assigning after should be fine after all, because the object and the dict are locked, so we'd only see reads which are proceeding against the values which are being copied. Once the values are invalidated we'd go to the dict which is being detached, and then we'd finally start seeing the new dict.

@colesbury colesbury merged commit 79688b5 into python:main Apr 29, 2024
34 checks passed
@colesbury colesbury deleted the gh-118331-unraisable branch April 29, 2024 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants