gh-136003: Execute pre-finalization callbacks in a loop#136004
Uh oh!
There was an error while loading. Please reload this page.
Conversation
| /* Each of these functions can start one another, e.g. a pending call | ||
| * could start a thread or vice versa. To ensure that we properly clean | ||
| * call everything, we run these in a loop until none of them run anything. */ | ||
| for (;){ |
There was a problem hiding this comment.
Would it make sense to add an arbitrary limit to detect infinite loop? For example, log an error after 16 attemps.
There was a problem hiding this comment.
Well, it would prevent deadlocks in rare cases, but it would cause crashes in other equally rare cases. Maybe it would be better to emit a fatal error when there are too many iterations?
Uh oh!
There was an error while loading. Please reload this page.
ericsnowcurrently left a comment
ericsnowcurrently left a comment There was a problem hiding this comment.
Overall, good work. 😄
I'm pretty sure there's a race we need to deal with which will require a variety of changes to this PR.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
When you're done making the requested changes, leave the comment: |
…cular-finalization
This solution is much simpler, but will perform a little bit worse. Instead of using a dedicated lock on the interpreter state, we can simply use stop-the-world (and the GIL on the default build) to ensure that no other threads can create pre-finalization callbacks concurrently.
The failing CI jobs are due to Ubuntu being weird, and should disappear when I rerun the job. But I'll take that as a sign that I should run the buildbots before merging this. |
ZeroIntensity added the 🔨 test-with-buildbots 
This comment was marked as outdated.
This comment was marked as outdated.
bedevere-bot removed the 🔨 test-with-buildbots ZeroIntensity commented Sep 17, 2025
Nevermind, I broke FT when I fixed the merge conflicts. Ugh. |
ZeroIntensity added the 🔨 test-with-buildbots bedevere-bot commented Sep 17, 2025
🤖 New build scheduled with the buildbot fleet by @ZeroIntensity for commit e202848 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F136004%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
bedevere-bot removed the 🔨 test-with-buildbots 
ZeroIntensity commented Sep 17, 2025
@ericsnowcurrently I'll hold off on merging until we talk tomorrow. We might want to extend the check to subinterpreters, and then disallow creating new interpreters during finalization. |
…cular-finalization
Apparently, Py_AddPendingCall() schedules for the main interpreter.
ZeroIntensity merged commit 2191497 into python:mainUh oh!
There was an error while loading. Please reload this page.
…ng finalization (GH-140103) This fixes a regression introduced by GH-136004, in which finalization would hang while executing atexit handlers if the system was out of memory. --------- Signed-off-by: yihong0618 <[email protected]> Co-authored-by: Peter Bierma <[email protected]> Co-authored-by: Victor Stinner <[email protected]>
…s during finalization (pythonGH-140103) This fixes a regression introduced by pythonGH-136004, in which finalization would hang while executing atexit handlers if the system was out of memory. --------- Signed-off-by: yihong0618 <[email protected]> Co-authored-by: Peter Bierma <[email protected]> Co-authored-by: Victor Stinner <[email protected]>
5 participants
Uh oh!
There was an error while loading. Please reload this page.