Skip to content

Conversation

@colesbury
Copy link
Contributor

@colesburycolesbury 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

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_memoryself.assertIn(p.returncode, (1, 120)) ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^AssertionError: -6 not found in (1, 120) ---------------------------------------------------------------------- Ran 7 tests in 0.331s FAILED (failures=1)

@colesbury
Copy link
ContributorAuthor

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

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

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

Copy link
Contributor

@DinoVDinoV 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.

@colesburycolesbury merged commit 79688b5 into python:mainApr 29, 2024
@colesburycolesbury deleted the gh-118331-unraisable branch April 29, 2024 19:49
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@colesbury@hugovk@mpage@DinoV