add a global signal handler for interrupts #1118
Conversation
|
@Naatan This is how I think it can be done. As mentioned earlier it is not pretty, as it relies on some globally created channels right now. There is still an open question on how to implement the |
| @@ -177,33 +175,13 @@ func binaryPathCmd(env []string, name string) (string, error) { | |||
| return split[0], nil | |||
| } | |||
|
|
|||
| func ignoreInterrupts(ctx context.Context) { | |||
There was a problem hiding this comment.
I don't think we should be removing this. IIRC it is required so as not to bubble stuff up from the subshell context. For this story we're concerned with interrupts happening outside of subshell.
There was a problem hiding this comment.
Yes, this is now replaced by this sighandler.IgnoreInterrupts() function, as we are now listening for interrupts on the captain.Command-level.
internal/sighandler/sighandler.go
Outdated
| var ignoreCh chan bool | ||
|
|
||
| // Init initializes the global signal interrupt handler | ||
| func Init(subCommand string) { |
There was a problem hiding this comment.
Can we use New and return a struct that can be closed. We don't want to still be capturing this after the captain runner has finished.
internal/sighandler/sighandler.go
Outdated
| logging.Debug("captured ctrl-c event") | ||
| analytics.EventWithLabel(analytics.CatCommandExit, subCommand, "interrupt") | ||
| analytics.WaitForAllEvents(time.Second) | ||
| os.Exit(1) |
There was a problem hiding this comment.
Why are you calling os.Exit? Big red flag. We want to bubble up interrupts as errors.
There was a problem hiding this comment.
I did it this way, because that is the default behavior of ctrl-c. It exits the programme with a non-zero exit code. Here we just inject our analytics code before.
I could add another channel, and run the c.execute() function in a go function, such that I could listen for what returns first: the main path (c.execute or the signal handler). That should be a little nicer.
internal/sighandler/sighandler.go
Outdated
| logging.Debug("captured ctrl-c event") | ||
| analytics.EventWithLabel(analytics.CatCommandExit, subCommand, "interrupt") | ||
| analytics.WaitForAllEvents(time.Second) | ||
| os.Exit(1) |
There was a problem hiding this comment.
Can we handle this outside this package.
Something like:
sh := sighandler.New()
defer sh.Close()
sh.OnInterrupt(func () { ... })
- rename Stop() -> Pause() - add WaitGroup - add mutex
https://www.pivotaltracker.com/story/show/175721061