GH-102126: fix deadlock at shutdown when clearing thread states by kumaraditya303 · Pull Request #102222 · python/cpython · GitHub

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

🤖 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.

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.

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.

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

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

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

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

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

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

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

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

…states (pythonGH-102222). (cherry picked from commit 5f11478) Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>

… states (pythonGH-102222). (cherry picked from commit 5f11478) Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>

Regardless I'll address that separately in a new issue

#102233

#102234) [3.11] GH-102126: fix deadlock at shutdown when clearing thread states (GH-102222) (cherry picked from commit 5f11478)

#102235) [3.10] GH-102126: fix deadlock at shutdown when clearing thread states (GH-102222). (cherry picked from commit 5f11478)

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

…GH-102222) (#102236) (cherry picked from commit 5f11478)

… state… (python#102235) [3.10] pythonGH-102126: fix deadlock at shutdown when clearing thread states (pythonGH-102222). (cherry picked from commit 5f11478)

fix deadlock with reentrant calls to get head lock

9ade043

bedevere-bot added the awaiting core review label Feb 24, 2023

bedevere-bot mentioned this pull request Feb 24, 2023

Possible deadlock at shutdown while recursively acquiring head lock #102126

Closed

kumaraditya303 and others added 2 commits February 24, 2023 18:15

improve comment

b3cac6d

📜🤖 Added by blurb_it.

1e6a87b

kumaraditya303 force-pushed the deadlock branch from dd672a9 to 1e6a87b Compare February 24, 2023 18:15

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Feb 24, 2023

kumaraditya303 requested review from ericsnowcurrently and pablogsal February 24, 2023 18:36

ericsnowcurrently approved these changes Feb 24, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Feb 24, 2023

kumaraditya303 merged commit 5f11478 into python:main Feb 25, 2023

bedevere-bot removed the awaiting merge label Feb 25, 2023

kumaraditya303 deleted the deadlock branch February 25, 2023 06:51

miss-islington assigned kumaraditya303 Feb 25, 2023

kumaraditya303 mentioned this pull request Feb 25, 2023

[3.11] GH-102126: fix deadlock at shutdown when clearing thread state… #102234

Merged

bedevere-bot removed the needs backport to 3.11 only security fixes label Feb 25, 2023

kumaraditya303 mentioned this pull request Feb 25, 2023

[3.10] GH-102126: fix deadlock at shutdown when clearing thread state… #102235

Merged

bedevere-bot removed the needs backport to 3.10 only security fixes label Feb 25, 2023

kumaraditya303 mentioned this pull request Feb 25, 2023

[3.9] GH-102126: fix deadlock at shutdown when clearing thread states (GH-102222) #102236

Merged

bedevere-bot removed the needs backport to 3.9 label Feb 25, 2023

kumaraditya303 added a commit that referenced this pull request Feb 25, 2023

[3.11] GH-102126 : fix deadlock at shutdown when clearing thread state… (

026faf2

#102234) [3.11] GH-102126: fix deadlock at shutdown when clearing thread states (GH-102222) (cherry picked from commit 5f11478)

ericsnowcurrently mentioned this pull request Feb 25, 2023

Disallow calling PyThreadState_Delete* at runtime shutdown #102233

Open

kumaraditya303 added a commit that referenced this pull request Mar 3, 2023

[3.10] GH-102126 : fix deadlock at shutdown when clearing thread state… (

6c2e052

#102235) [3.10] GH-102126: fix deadlock at shutdown when clearing thread states (GH-102222). (cherry picked from commit 5f11478)

kumaraditya303 mentioned this pull request Mar 18, 2023

Add experts for runtime lifecycle management python/devguide#1068

Merged

ambv pushed a commit that referenced this pull request Mar 28, 2023

[3.9] GH-102126 : fix deadlock at shutdown when clearing thread states (…

7cb3a44

…GH-102222) (#102236) (cherry picked from commit 5f11478)

GH-102126: fix deadlock at shutdown when clearing thread states by kumaraditya303 · Pull Request #102222 · python/cpython · GitHub

Uh oh!

Uh oh!

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

GH-102126: fix deadlock at shutdown when clearing thread states#102222kumaraditya303 merged 3 commits intopython:mainpython/cpython:mainfrom kumaraditya303:deadlockkumaraditya303/cpython:deadlockCopy head branch name to clipboard

Conversation

kumaraditya303 commented Feb 24, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-bot commented Feb 24, 2023

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

kumaraditya303 commented Feb 25, 2023

Uh oh!

kumaraditya303 commented Feb 25, 2023

Uh oh!

miss-islington commented Feb 25, 2023

Uh oh!

miss-islington commented Feb 25, 2023

Uh oh!

miss-islington commented Feb 25, 2023

Uh oh!

miss-islington commented Feb 25, 2023

Uh oh!

bedevere-bot commented Feb 25, 2023

Uh oh!

bedevere-bot commented Feb 25, 2023

Uh oh!

bedevere-bot commented Feb 25, 2023

Uh oh!

kumaraditya303 commented Feb 25, 2023

Uh oh!

bedevere-bot commented Mar 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GH-102126: fix deadlock at shutdown when clearing thread states#102222
kumaraditya303 merged 3 commits into
python:mainfrom
kumaraditya303:deadlock

kumaraditya303 commented Feb 24, 2023 •
edited by bedevere-bot
Loading