-
Notifications
You must be signed in to change notification settings - Fork 576
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
Do not send redundant SIGQUITs #2073
Conversation
I like this fix better, except for the if duplication/perltidy. |
Please squash your commits. |
@kraih Done, I think. Let me know if that doesn't look how you expect |
Is |
If the worker reports itself as shutting down, do not send a SIGQUIT to the worker. It already knows it is shutting down, and this can cause a race with signal handlers that can trigger core dumps.
I didn't even see that. Must've switched the words in my head without realizing. Something like Updated the commit message 👍 |
I like this 👍 For completeness, just a note that this does not remove the race condition. It is still possible to hit it by sending two To fully fix the race, |
If the worker reports itself as shutting down, do not second another SIGQUIT to the worker. It already knows it is shutting down, and this can cause a race with signal handlers that triggers coredumps.
Summary
Full writeup in #2046
The short version:
The patch here increments the worker's
quit
state in the managing thread so that it does not send a second SIGQUIT to the worker if the manager finds out about the graceful shutdown from the workerMotivation
This behavior was causing coredumps in our application, since we were using SIGQUIT to gracefully end workers.
I understand from #1883/#1449 that there are also other ways for the worker to initiate graceful shutdown, which can trigger the same race condition
References
Tested via the test app uploaded to #2046
Attempts to fix the same issue seen in #1883