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: Don't raise an error if tuple allocation fails when clearing weakrefs #118338

Merged
merged 6 commits into from Apr 29, 2024

Conversation

mpage
Copy link
Contributor

@mpage mpage 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) {
const char *err;
if (old_exc == NULL) {
err = "Deallocator of type '%s' raised an exception";
}
else if (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.
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.
@mpage mpage marked this pull request as ready for review April 27, 2024 01:55
@mpage mpage requested a review from colesbury April 27, 2024 01:55
@hugovk
Copy link
Member

hugovk commented Apr 27, 2024

Thanks, this fixes gh-118331 for me.

Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

Thanks!

Objects/weakrefobject.c Outdated Show resolved Hide resolved
@bedevere-app
Copy link

bedevere-app bot commented Apr 29, 2024

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
Contributor Author

mpage commented Apr 29, 2024

I have made the requested changes; please review again.

@bedevere-app
Copy link

bedevere-app bot commented Apr 29, 2024

Thanks for making the requested changes!

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

@colesbury colesbury merged commit 43fa766 into python:main Apr 29, 2024
34 of 37 checks passed
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