GH-102126: fix deadlock at shutdown when clearing thread states

[kumaraditya303]

• Issue: Possible deadlock at shutdown while recursively acquiring head lock #102126

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @kumaraditya303 for commit 1e6a87b 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

[ericsnowcurrently]

LGTM

There's one case that could be problematic but (ISTM) is highly unlikely: some other thread might call PyThreadState_Delete*() on one of the existing thread states, which would modify the linked list while we're walking it here. We could mitigate that (very small) risk by checking in those functions if the interpreter is finalizing and returning immediately (or blocking) if it is. FWIW, the status quo is already deficient in that case, and this PR only slightly adds to the problem. Regardless, the risk is fairly minimal so we can address that separately.

(Before merging, would you mind just double-checking that the risk actually is minimal? 😄)

---

One observation: the lock isn't strictly necessary here. It's needed for one of the following:

• in case a new thread state gets created for the interpreter
• in the case described above (PyThreadState_Delete*() was called in another thread)

At this point in interpreter finalization, neither of those things should be happening. So it may make sense to disallow them (or ignore/block, in the case of the second one). Alternately we could give each interpreter it's own lock (e.g. PyInterpreterState.threads.mutex) to use when managing its thread states, as opposed to reusing the global "HEAD" lock like we currently do.

[kumaraditya303]

There's one case that could be problematic but (ISTM) is highly unlikely: some other thread might call PyThreadState_Delete*() on one of the existing thread states, which would modify the linked list while we're walking it here. We could mitigate that (very small) risk by checking in those functions if the interpreter is finalizing and returning immediately (or blocking) if it is. FWIW, the status quo is already deficient in that case, and this PR only slightly adds to the problem. Regardless, the risk is fairly minimal so we can address that separately.

Yeah, I too noticed that but I am not concerned about that as at runtime shutdown such code is already unsafe and can crash or result in hang etc. Regardless I'll address that separately in a new issue and get this fixed first.

Thanks for the review.

[kumaraditya303]

The buildbots failures are unrelated so merging. https://buildbot.python.org/all/#/builders/259/builds/681/steps/5/logs/stdio

[miss-islington]

Thanks @kumaraditya303 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10, 3.11.
🐍🍒⛏🤖

[miss-islington]

Sorry, @kumaraditya303, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 5f11478ce7fda826d399530af4c5ca96c592f144 3.11

[miss-islington]

Sorry @kumaraditya303, I had trouble checking out the 3.10 backport branch.
Please retry by removing and re-adding the "needs backport to 3.10" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker 5f11478ce7fda826d399530af4c5ca96c592f144 3.10

[miss-islington]

Sorry, @kumaraditya303, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 5f11478ce7fda826d399530af4c5ca96c592f144 3.9

[bedevere-bot]

GH-102234 is a backport of this pull request to the 3.11 branch.

[bedevere-bot]

GH-102235 is a backport of this pull request to the 3.10 branch.

[bedevere-bot]

GH-102236 is a backport of this pull request to the 3.9 branch.

[kumaraditya303]

Regardless I'll address that separately in a new issue

#102233

[bedevere-bot]

GH-102236 is a backport of this pull request to the 3.9 branch.