Ignore signals using notify/stop instead of ignore/reset #658
Conversation
| @@ -701,8 +701,9 @@ func runBazel(bazel string, args []string, out io.Writer, config config.Config) | |||
| // TODO(#512): We may want to treat a `bazel run` commmand differently. | |||
| // Since signal handlers are process-wide global state and bazelisk may be | |||
| // 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.
See comments in buildbuddy-io/buildbuddy#8326 -
signal.Ignore()is "permanent" soReset()has no effect.