◐ Shell
reader mode source ↗
Skip to content

gh-66587: Fix deadlock from pool worker death without communication#16103

Open
applio wants to merge 6 commits into
python:mainfrom
applio:fix_multiprocessing_worker_died_indefinite_hang
Open

gh-66587: Fix deadlock from pool worker death without communication#16103
applio wants to merge 6 commits into
python:mainfrom
applio:fix_multiprocessing_worker_died_indefinite_hang

Conversation

@applio

@applio applio commented Sep 13, 2019 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

Adds tracking of which worker process in the pool takes which job from the queue.

When a worker process dies without communication, its task/job is also lost. By tracking what job that worker took off the job queue as its task, upon detecting the death, the parent process can add an item to the result queue indicating the failure of that task/job.

In case of a future regression, the supplied test uses subprocess to constrain the test with a timeout to ensure an indefinite hang does not interfere with the running of tests.

https://bugs.python.org/issue22393

@applio applio requested review from pablogsal and zooba September 13, 2019 13:38
@pierreglaser

pierreglaser commented Sep 19, 2019

Copy link
Copy Markdown
Contributor

This looks good to me, simply a few remarks:

Also pinging @tomMoral

@zooba

zooba commented Sep 19, 2019

Copy link
Copy Markdown
Member

For mine, I think this fix seems more elegant than #10441, but the tests in that PR seem to have more coverage.

I personally prefer to just have the task fail, and the pool continue. The current behaviour is that the broken worker is immediately replaced and other work continues, but if you wait on the failed task then it will never complete. Now it does complete (with a failure), which means robust code can re-queue it if appropriate. I don't see any reason to tear down the entire pool.

Few comments on the PR incoming.

tomMoral
tomMoral reviewed Sep 20, 2019
View reviewed changes

@tomMoral tomMoral 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.

Hide comment

Here is a batch of comments.

I have to say that I like this solution as it is the most robust way of handling this, (a kind of scheduler). But it also comes with more complexity and increase communication needs -> more changes for deadlocks.

One of the main argument for the fail on error design is that there is no way there is no way to know in the main process if the worker that died had a lock on one of the communication queue. In this situation, the only way to recover the system and avoid a deadlock is to kill the Pool and re-spawn one.

applio and others added 2 commits September 22, 2019 16:37
@applio @zooba
Remove spurious space. …
e1a9eb5 
Co-Authored-By: Steve Dower <steve.dower@microsoft.com>
@applio
Fix result position for killed workers, add Steve suggested change to… …
6459284 
… use dict.pop().
taleinat
taleinat reviewed Nov 17, 2019
View reviewed changes

@taleinat taleinat 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.

Hide comment

Additional tests would certainly be a good idea.

@csabella

csabella commented Jan 10, 2020

Copy link
Copy Markdown
Contributor

@applio, I'm not sure where this one is at, but I believe there are some comments that still need to be addressed. I don't know if it's waiting on anything else, but it would probably be nice to get this merged.

@ambv

ambv commented Sep 23, 2021

Copy link
Copy Markdown
Contributor

Closing and re-opening to re-trigger CI.

@ambv ambv closed this Sep 23, 2021
@ambv ambv reopened this Sep 23, 2021
@ambv ambv added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 23, 2021
@bedevere-bot

bedevere-bot commented Sep 23, 2021

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @ambv for commit 6459284 🤖

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 23, 2021
@ambv ambv added 🔨 test-with-buildbots Test PR w/ buildbots; report in status section needs backport to 3.10 only security fixes labels Sep 23, 2021
@bedevere-bot

bedevere-bot commented Sep 23, 2021

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @ambv for commit 6459284 🤖

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 23, 2021
12 hidden items Load more…
@ambv

ambv commented May 17, 2022

Copy link
Copy Markdown
Contributor

This missed the boat for inclusion in Python 3.9 which accepts security fixes only as of today.

@danoreilly danoreilly mannequin mentioned this pull request Apr 10, 2022
Open
@serhiy-storchaka serhiy-storchaka added the needs backport to 3.11 only security fixes label May 20, 2022
@serhiy-storchaka serhiy-storchaka added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes and removed needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels May 9, 2024
@Yhg1s Yhg1s removed the needs backport to 3.12 only security fixes label Apr 8, 2025
@python-cla-bot

python-cla-bot Bot commented Apr 18, 2025

Copy link
Copy Markdown

The following commit authors need to sign the Contributor License Agreement:

CLA signed

@serhiy-storchaka serhiy-storchaka added the needs backport to 3.14 bugs and security fixes label May 8, 2025
@gpshead gpshead changed the title bpo-22393: Fix deadlock from pool worker death without communication May 22, 2025
@gpshead gpshead mentioned this pull request May 22, 2025
Closed
@gpshead gpshead self-assigned this May 22, 2025
@gpshead gpshead added stdlib Standard Library Python modules in the Lib/ directory topic-multiprocessing labels May 22, 2025
@gpshead gpshead self-requested a review as a code owner May 22, 2025 18:02
@gmanthey gmanthey mentioned this pull request Jun 25, 2025
Open
@github-actions

github-actions Bot commented Apr 7, 2026

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 7, 2026
@serhiy-storchaka serhiy-storchaka added the needs backport to 3.15 pre-release feature fixes, bugs and security fixes label May 30, 2026
@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label Jun 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@taleinat taleinat taleinat left review comments

@zooba zooba zooba left review comments

@pablogsal pablogsal Awaiting requested review from pablogsal

@gpshead gpshead Awaiting requested review from gpshead gpshead is a code owner

+1 more reviewer

@tomMoral tomMoral tomMoral left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@gpshead gpshead

Labels

awaiting changes needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes needs backport to 3.15 pre-release feature fixes, bugs and security fixes stdlib topic-multiprocessing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.