Skip to content

bpo-33997: Fix racing condition in termination of pool result_handler #8009

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

3mb3dw0rk5
Copy link

@3mb3dw0rk5 3mb3dw0rk5 commented Jun 29, 2018

This patch fixes a racing condition when using Pool.terminate()
My application is using imap() but aborting further processing after some iterations. Therefore I used terminate() to end the workers pool but the called join() of the result_handler was hanging sporadically.
I traced the issue down to the problem that somehow poll() signals new data in the outqueue but the preceding get() never returned due to ERROR_IO_PENDING in winapi of the queue hand endless hanging WaitForMultipleObjects().

Waiting at most for the two sentinels issued by the pool cleanup fixes the issue. Also the
while sentinel_count < 2 seems to me more reasonable and plattform-independent than the old for i in range(10).

https://bugs.python.org/issue33997

Fixes a racing condition when using Pool.terminate(). Sporadically the poll() function signals new data in the queue but get() never returns.
This fix just consumes the two issued sentinels by the cleanup process.
@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

When your account is ready, please add a comment in this pull request
and a Python core developer will remove the CLA not signed label
to make the bot check again.

Thanks again for your contribution, we look forward to reviewing it!

@3mb3dw0rk5
Copy link
Author

3mb3dw0rk5 commented Jun 29, 2018

Added missing "GitHub Name" entry to successful verify CLA.

Copy link
Contributor

@remilapeyre remilapeyre left a comment

Choose a reason for hiding this comment

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

Thanks @3mb3dw0rk5

@@ -497,6 +499,7 @@ def _handle_results(outqueue, get, cache):

if task is None:
util.debug('result handler ignoring extra sentinel')
sentinel_counter += 1
Copy link

Choose a reason for hiding this comment

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

This extra sentinel is no longer ignored, so the debug is wrong.

Copy link
Author

Choose a reason for hiding this comment

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

Obviously right. Fixed that.

Fixed debug message.
Copy link

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 Sep 21, 2024
@python-cla-bot
Copy link

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

CLA signed

@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Apr 18, 2025
Copy link

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 May 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting core review stale Stale PR or inactive for long period of time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants