Ignore signals using notify/stop instead of ignore/reset #658
Conversation
| // used as a library, reset the signal handlers after the process exits. | ||
| signal.Ignore(syscall.SIGINT, syscall.SIGQUIT, syscall.SIGTERM) | ||
| defer signal.Reset(syscall.SIGINT, syscall.SIGQUIT, syscall.SIGTERM) | ||
| sigCh := make(chan os.Signal, 1) |
There was a problem hiding this comment.
I'm not sure how relevant this is in practice, but should we set the channel size to something reasonably large (perhaps just the number of signals?) based on https://cs.opensource.google/go/go/+/refs/tags/go1.24.0:src/os/signal/signal.go;l=110-113;drc=b28b263a91e4fd4af6900e045cd35c2ed1d6e3ff?
There was a problem hiding this comment.
If the channel is full, the notification is discarded silently: https://cs.opensource.google/go/go/+/refs/tags/go1.24.0:src/os/signal/signal.go;l=244-248;drc=b28b263a91e4fd4af6900e045cd35c2ed1d6e3ff. We are attempting to ignore signals anyway, so that's fine.
There was a problem hiding this comment.
That's my understanding as well - I actually wanted to use an unbuffered channel here to communicate that intent more clearly, but it triggered a nogo error with a message saying that this channel needs to be buffered to avoid dropping signals - which presumably helps in the majority of cases where people do want to be notified of signals.
|
@meteorcloudy @fweikert Could you review this? |
This includes our fix in bazelbuild/bazelisk#658. Part of #8326.
This includes our fix in bazelbuild/bazelisk#658. Part of #8326.
4 participants
See comments in buildbuddy-io/buildbuddy#8326 -
signal.Ignore()is "permanent" soReset()has no effect.