Skip to content

Conversation

yihong0618
Copy link
Contributor

@yihong0618 yihong0618 commented Oct 14, 2025

fix the error that's forget the cleanup

cc @ZeroIntensity @picnixz and it also turns out its not the _testcapi_setnomemroy(0) issue.

seems the news can skip?

after this patch no hang anymore

image

@yihong0618 yihong0618 changed the title fix: atexit_cleanup the state gh-140080: atexit_cleanup the state Oct 14, 2025
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

Since the 3.15 alpha is today, please add a news entry so we can know what was fixed in the next release.

Also, per usual, this needs a test case. At a glance, this should be reproducible by calling set_nomemory after registering an atexit callback.

@yihong0618
Copy link
Contributor Author

Since the 3.15 alpha is today, please add a news entry so we can know what was fixed in the next release.

Also, per usual, this needs a test case. At a glance, this should be reproducible by calling set_nomemory after registering an atexit callback.

of course will try, but the test things maybe need some help( - -)
will try it.

and also bring the 3.13(no_memory) tests is that under plan need to add in this pull request?

@ZeroIntensity
Copy link
Member

and also bring the 3.13(no_memory) tests is that under plan need to add in this pull request?

Let's do that in a different PR, because we'll want that in 3.14 as well.

@yihong0618
Copy link
Contributor Author

and also bring the 3.13(no_memory) tests is that under plan need to add in this pull request?

Let's do that in a different PR, because we'll want that in 3.14 as well.

ok will do it later, thank you for the help~

Signed-off-by: yihong0618 <[email protected]>
@yihong0618
Copy link
Contributor Author

Since the 3.15 alpha is today, please add a news entry so we can know what was fixed in the next release.

Also, per usual, this needs a test case. At a glance, this should be reproducible by calling set_nomemory after registering an atexit callback.

Done, before the patch the tests hangs
after the patch the tests pas

tests from #138491

yihong0618 and others added 2 commits October 14, 2025 22:17
Signed-off-by: yihong0618 <[email protected]>
Signed-off-by: yihong0618 <[email protected]>
yihong0618 and others added 2 commits October 15, 2025 07:48
Co-authored-by: Victor Stinner <[email protected]>
Signed-off-by: yihong0618 <[email protected]>
@yihong0618 yihong0618 requested a review from vstinner October 15, 2025 05:51
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM

@vstinner vstinner added the needs backport to 3.14 bugs and security fixes label Oct 15, 2025
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

Python 3.13 is not affected, atexit doesn't use PyObject *copy = PyList_GetSlice(state->callbacks, 0, PyList_GET_SIZE(state->callbacks)); in this version.

@yihong0618
Copy link
Contributor Author

Python 3.13 is not affected, atexit doesn't use PyObject *copy = PyList_GetSlice(state->callbacks, 0, PyList_GET_SIZE(state->callbacks)); in this version.

FYI: 3.14 not hang
and #136004 this patch seems did not backport to it.

@yihong0618
Copy link
Contributor Author

LGTM

Thank you!

@vstinner vstinner changed the title gh-140080: atexit_cleanup the state gh-140080: Fix atexit with low memory Oct 15, 2025
@ZeroIntensity ZeroIntensity removed the needs backport to 3.14 bugs and security fixes label Oct 15, 2025
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

Yeah, this doesn't affect 3.14, so we shouldn't backport it there.

@yihong0618 After this is merged, would you mind creating a PR to 3.14 with the test from this PR?

@yihong0618
Copy link
Contributor Author

Yeah, this doesn't affect 3.14, so we shouldn't backport it there.

@yihong0618 After this is merged, would you mind creating a PR to 3.14 with the test from this PR?

of course with pleasure to do it thank you

@ZeroIntensity ZeroIntensity merged commit a05aece into python:main Oct 15, 2025
53 checks passed
@vstinner
Copy link
Member

Why not backporting this change to 3.14?

@ZeroIntensity
Copy link
Member

Why not backporting this change to 3.14?

On 3.14, we don't execute atexit callbacks in a loop, so if PyList_GetSlice fails, it will correctly bail out and continue with finalization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants