fix(os/gproc): restore default signal handling when shutdown listenin… - #4868
Open
jiang-bx wants to merge 1 commit into
Open
fix(os/gproc): restore default signal handling when shutdown listenin…#4868jiang-bx wants to merge 1 commit into
jiang-bx wants to merge 1 commit into
Conversation
…g ends `listen()` is the only reader of `signalChan`, and it returns as soon as the shutdown handlers of the first shutdown signal are done. `signal.Notify` is never stopped, so from that point on every SIGINT/SIGTERM/SIGQUIT/SIGABRT is delivered to a channel nobody reads and is silently discarded. If a shutdown handler blocks, or the process fails to exit right after them, the process becomes immune to every signal but SIGKILL. Because SIGQUIT and SIGABRT are shutdown signals too, `kill -QUIT` can no longer dump the goroutine stacks either, so the hang cannot even be diagnosed. This affects every server built on `ghttp.Run` / `ghttp.Wait` / `g.Listen`, and the `gf run` CLI itself, which registers its shutdown handler the same way. Restore the default behavior for the listened signals before running the shutdown handlers. A second shutdown signal then terminates the process even when a handler blocks, which is the conventional escape hatch. `notifySignals` is guarded as well: once the listening has ended, re-arming `signal.Notify` for a handler added afterwards would swallow signals again. The flag is set under `signalHandlerMu`, the same lock `notifySignals` runs under, so a concurrent `AddSigHandler` cannot re-arm right after `signal.Stop`. The existing `Test_Signal` feeds `signalChan` directly and never exercises `signal.Notify`, so it cannot observe any of this. The new tests raise real signals in a child process, which is also the only way to assert that a process does get terminated.
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.
listen()is the only reader ofsignalChan, and it returns as soon as the shutdownhandlers of the first shutdown signal are done:
signal.Notifyis never stopped, so from that moment on every SIGINT, SIGTERM, SIGQUITand SIGABRT is delivered into a channel that nobody reads any more, and is silently
discarded.
If a shutdown handler blocks, or the process does not exit right after the handlers, the
process becomes immune to every signal but SIGKILL. And because SIGQUIT and SIGABRT are
shutdown signals as well,
kill -QUITcan no longer dump the goroutine stacks, so thehang cannot even be diagnosed.
Why this is a bug rather than a convention
The contract of the loop is "handle the shutdown signal, then stop listening". Stopping
the listening should hand the signals back to their default behavior; leaving them
captured by a channel with no reader is not a policy that any caller can observe, choose
or undo. It is simply an unreachable state.
The blast radius is wide, because nothing here is opt-in:
ghttp.Run,ghttp.Waitandg.Listenall end up in this loop, so it covers essentially every GoFrame server. ThegfCLI is affected too —gf runregisters its own shutdown handler throughgproc.AddSigHandlerShutdownand then waits up to 30s for the child process inapp.End. So when a service is started withgf run, one Ctrl+C leaves the CLI and theservice deaf at the same time, and the terminal has to be abandoned.
Reproduction
go run .— a shutdown handler that blocksSend SIGTERM three times, one second apart. (SIGTERM rather than SIGINT: a shell sets
SIGINT and SIGQUIT to
SIG_IGNfor background jobs, which would blur the result.)Before — the process outlives every signal:
After — the second one terminates it:
The change
listen()callsendSignalListening()when it takes a shutdown signal, before runningthe handlers, so the default behavior is already back in place while they run.
notifySignalsis guarded as well: once the listening has ended, a handler addedafterwards would re-arm
signal.Notifyon a channel that has no reader, and swallowsignals again. The flag is set under
signalHandlerMu, the same locknotifySignalsruns under — otherwise a concurrent
AddSigHandlercould read the flag as false and callsignal.Notifyright aftersignal.Stop, which only narrows the window instead ofclosing it.
Compatibility
Once a shutdown signal has been handled, the listened signals go back to the behavior they
had before
signal.Notify, so a second shutdown signal terminates the process instead ofbeing ignored. This is the conventional escape hatch — pressing Ctrl+C again to force quit,
or Kubernetes following SIGTERM with SIGKILL. (When a signal was already ignored at process
start, that is what gets restored instead: a shell sets SIGINT and SIGQUIT to
SIG_IGNforbackground jobs, for instance.)
signal.Stopcovers every signal registered on the channel, custom ones included, such asthe SIGUSR1
ghttpuses for graceful reload. Those handlers could not run after the loopreturned anyway, since nothing reads the channel any more; the difference is that such a
signal now takes its default behavior rather than being silently discarded.
No code can be relying on shutdown being uninterruptible by design: that state is not
reachable deliberately, it only exists because the loop leaves
signal.Notifyarmed afterits reader is gone, which also disables SIGQUIT stack dumps.
Test
Test_SignalfeedssignalChandirectly, so it never exercisessignal.Notifyandcannot observe any of this. The two new tests raise real signals, in a child process —
which is also the only way to assert that a process does get terminated:
Test_Signal_SecondShutdownSignalTerminatesProcess— a blocking shutdown handler, and asecond signal that has to terminate the process.
Test_Signal_HandlersAddedAfterListenEndedDoNotReArmNotify— the same, with the handlerregistering another handler before it blocks.
Reverting either half of the change fails the matching test, and
Test_Signalkeepspassing with the change (it is
//go:build linux, and was also run on darwin to confirm).