client: restore signal dispositions on teardown and escalate repeated signals#1560
Merged
Conversation
signals Three related defects left nighthawk unresponsive to Ctrl-C exactly when shutdown hangs: - ~SignalHandler() left the process-wide handler installed, delegating to a destroyed instance (undefined behavior; in practice a silent no-op). - client.cc destroyed the SignalHandler before process->shutdown(), so the phase most likely to hang had no live signal handling. - After the first handled signal the notification pipe is closed, so a second Ctrl-C was silently swallowed, leaving no force-quit path. SignalHandler now saves the dispositions it replaces and restores them on destruction (then clears the process-wide delegate), the handler's scope in client.cc covers process->shutdown(), and the saved dispositions are restored as soon as the first signal is consumed - before the cancellation callback runs, since that callback may itself block on the hung shutdown - so any further signal gets the OS default action and terminates the process. Note: nighthawk_service uses the same SignalHandler and inherits the escalation; a second SIGTERM/SIGINT now terminates it after the first has initiated graceful shutdown. Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
aab0956 to
4804e77
Compare
eric846
approved these changes
Jul 9, 2026
Contributor
|
Thank you Otto! This will be a huge improvement to have a fast and clean return even when the server was slow. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part 3/3 of fixing nighthawk failing to shut down when a target can't keep up.
Three related defects left nighthawk unresponsive to Ctrl-C exactly when shutdown hangs:
~SignalHandler()left the process-wide handler installed, delegating to a destroyed instance (undefined behavior; in practice a silent no-op).client.ccdestroyed the SignalHandler beforeprocess->shutdown(), so the phase most likely to hang had no live signal handling.Fix: SignalHandler saves the dispositions it replaces and restores them on destruction (then clears the process-wide delegate); the handler's scope in
client.ccnow coversprocess->shutdown(); and the saved dispositions are restored as soon as the first signal is consumed — deliberately before the cancellation callback runs, since that callback may itself block on the hung shutdown (it contends on the lock held across the worker drain loop). A restore placed after the callback would never execute in exactly the flagship hang scenario. Destructor ordering (set flag, restore dispositions, join, clear delegate) is documented in-code.Behavior change: a second Ctrl-C now hard-kills the process (no output written) — intended, it's the escape hatch.
nighthawk_serviceuses the same SignalHandler and inherits the escalation.Testing: disposition-restore test, a threadsafe-style death test (post-destruction SIGTERM must kill the process; previously "failed to die"), and an escalation test that observes the disposition fall back to
SIG_DFLvia read-onlysigactionprobing (astd::signalround-trip probe could race and clobber the restore). All three fail against the previous implementation.