Skip to content

gh-158570: Fix ProcessPoolExecutor shutdown deadlock when the result pipe is full - #158571

Open
tfzee wants to merge 6 commits into
python:mainfrom
tfzee:fix-ppe-shutdown-result-pipe-deadlock
Open

tfzee wants to merge 6 commits into
python:mainfrom
tfzee:fix-ppe-shutdown-result-pipe-deadlock

Conversation

@tfzee

@tfzee tfzee commented Oct 1, 2026 •

Copy link
Copy Markdown

Fixes a deadlock in ProcessPoolExecutor.shutdown().
When shutting down, each worker receiving the None sentinel wrote its PID to the result queue. While joining workers in _join_executor_internals(), the manager thread no longer reads that queue. If the result pipe is full, for example with many workers or a small pipe buffer, a worker will block writing its PID and never exits. p.join() then hangs.
This PR removes the shutdown-only PID write from _process_worker(). Since nothing consumes the PID.

The new test makes the hang deterministic.
Right before the workers are told to exit, a thread fills up the result queue.
This is to emulate the a lot of workers exiting and writing their results back.
Any worker that still writes to the result queue on exit would block behind this.

Prepared with help from an AI assistant (Claude). I reviewed and verified the change.

@python-cla-bot

python-cla-bot Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

CLA signed

@ngoldbaum ngoldbaum left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not very familiar with this code but I got pinged to review this. I used an AI model, it found an alternate fix. See below but please take it worth a grain of salt and try for yourself, along with looking at the 2023 change my comment links to.

@gpshead does have more recent context about this code and might have better suggestions.

Comment thread Lib/concurrent/futures/process.py Outdated
Comment thread Lib/test/test_concurrent_futures/test_deadlock.py

@ngoldbaum ngoldbaum left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@tfzee, thanks! Looks good to me besides one issue in your test. One last thing: can you trim down the PR description a bit and update it to reflect the current approach? Your PR description will be the commit message for the squashed merge commit.

I don't feel comfortable approving and merging this because I've never worked in multiprocessing and my review so far was heavily AI-assisted.

@vstinner any chance you can take a look at this? For context, you removed some other obsolete code for handling PIDs in multiprocessing back in 2023 in #109906. Maybe you'd feel a bit more comfortable hitting the merge button than me?

A Quansight client hit the deadlock fixed by this PR which is why I'm looking at this PR. I think it's not the same organization hitting this that also led to @tfzee to working on this, so it seems like this is being hit by two different organizations using Python in production. Apparently some new CPUs came out this week with lots and lots of cores so these rare timing bugs are suddenly getting hit with a high likelihood.

Comment thread Lib/test/test_concurrent_futures/test_deadlock.py Outdated
@nascheme

nascheme commented Oct 9, 2026

Copy link
Copy Markdown
Member

Could we remove the shutdown-only PID notification instead of adding a drain loop?

I think that's the better solution and it looks correct to me. However, I'm not familiar with multiprocessing internals either.

Co-authored-by: ngoldbaum <nathan.goldbaum@gmail.com>"
@gpshead gpshead self-assigned this Oct 12, 2026
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.

4 participants