Skip to content

Conversation

@mpage
Copy link
Contributor

@mpagempage commented Apr 26, 2024

It's not safe to raise an exception in PyObject_ClearWeakRefs() if one is not already set. PyObject_ClearWeakRefs() may be called (transitively) by _Py_Dealloc(), which contains

cpython/Objects/object.c

Lines 2843 to 2860 in 5a90de0

// gh-89373: The tp_dealloc function must leave the current exception
// unchanged.
if (tstate!=NULL&&tstate->current_exception!=old_exc){
constchar*err;
if (old_exc==NULL){
err="Deallocator of type '%s' raised an exception";
}
elseif (tstate->current_exception==NULL){
err="Deallocator of type '%s' cleared the current exception";
}
else{
// It can happen if dealloc() normalized the current exception.
// A deallocator function must not change the current exception,
// not even normalize it.
err="Deallocator of type '%s' overrode the current exception";
}
_Py_FatalErrorFormat(__func__, err, type->tp_name);
}

Additionally, make sure we clear the weakrefs when tuple allocation fails.

This bug predates gh-111926. It exists in 3.12 and likely in earlier versions too. If it's getting tickled now, I suspect it's because we always allocate a tuple now when clearing weakrefs that have callbacks, whereas previously we only did so if there was more than one weakref with a callback, making it more likely that we fail to allocate a tuple when running test_repl.TestInteractiveInterpreter.test_no_memory.

It's not safe to raise an exception in `PyObject_ClearWeakRefs()` if one is not already set, since it may be called by `_Py_Dealloc()` and hit https://github.com/python/cpython/blob/5a90de0d4cbc151a6deea36a27eb81b192410e56/Objects/object.c#L2843-L2860. Additionally, make sure we clear the weakrefs even when tuple allocation fails. This bug predates pythongh-111926. If it's getting tickled now, I suspect it's because we always allocate a tuple when clearing weakrefs that have callbacks.
mpage added 3 commits April 26, 2024 16:37
Depending on finalization, we may encounter another allocation failure that causes the interpreter to exit with a non-zero status. That's OK, we just need to verify that we're not exiting because weakref clearing raised an exception.
@mpagempage marked this pull request as ready for review April 27, 2024 01:55
@mpagempage requested a review from colesburyApril 27, 2024 01:55
@hugovk
Copy link
Member

Thanks, this fixes gh-118331 for me.

Copy link
Contributor

@colesburycolesbury left a comment

Choose a reason for hiding this comment

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

Thanks!

@bedevere-app
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

@mpage
Copy link
ContributorAuthor

I have made the requested changes; please review again.

@bedevere-app
Copy link

Thanks for making the requested changes!

@Fidget-Spinner, @colesbury: please review the changes made to this pull request.

@colesburycolesbury merged commit 43fa766 into python:mainApr 29, 2024
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

@mpage@hugovk@colesbury@Fidget-Spinner