-
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
Fix potential core dump during worker process shutdown #1883
Conversation
I'm a big fan of simplifying code, but i'm also worried about losing important functionality. This PR needs to be double checked. |
452d039
to
d85e4b2
Compare
This PR needs to be rebased. |
Thanks, I will just close it as nobody seems interested in reviewing. |
That's the reason i was asking for it to be rebased, if tests don't work nobody will review. 🙄 |
All tests pass and there are no conflicts so I just reopened 🤷 |
The PR would leave the documentation incorrect at least, since the worker does not go into graceful shutdown mode anymore. |
So, since the question has come up on IRC: The upside of this change is that it eliminates a possible signal handler race condition during worker shutdown that kills the worker in an unfortunate way. The downside is that it leaves the possibility of a worker getting stuck not handling any new requests forever, without the manager process ever knowing, if the app developer made a small mistake that keeps the event loop running. Actually not an easy decision to make. |
Looks like I'm new to this discussion, but what's the rationale for breaking the cycle at (1) instead of at (3)? That's the method we ended up with in #2046. Seems like it would be useful to notify the managing thread about attempted graceful shutdown, so it can still timeout and SIGKILL us if we get stuck. |
It sounds to me like we wouldn't want this change if it can end up with impossible-to-debug zombie processes. |
I didn't consider that a manual Please consider this skeleton app:
It seems wrong to close all requests here just because But for |
I am not going to update this PR as I think the fix by @brsakai-csco is better and my concern above is a separate issue. |
Closing in favor of #2073. |
Summary
Remove redundant
stop_gracefully
afterfinish
.Motivation
When reaching
max_accepts
,IOLoop
willfinish
callback on thePrefork
worker which willfinished=1
heartbeat on a socket which willPrefork
manager toSIGQUIT
the worker which willstop_gracefully
on theIOLoop
This seems redundant (and slightly circular) as the
IOLoop
is already in the process of stopping. It is also a small bug because the worker process could have uninstalled itslocal $SIG{QUIT}
handler between step 2 and step 3 and the signal will then cause a core dump (#1449).I am a bit wary of suggesting this but I could not find anything else happening due to that
finish
callback so I propose to break the cycle already at the first step.(Most of the PR is cleaning up the heartbeat protocol now that
finished
is no longer used.)References
This fixes #1449 which is a rare but real problem.