Capture SIGINT/SIGTERM and wait for the child process to terminate.

[bheisler]

I'm not entirely sure this is the right approach to threading the interrupted flag through to the other code, but it seems to work.

[casey]

Awesome, thank you for the PR!

It looks the travis test failures are caused by a change to the version of the dash shell shipped by homebrew and are unrelated to your changes.

I won't have the time to give this a proper review until this weekend, although I have some thoughts:

It looks like it only processes ctrl-c during non-shebang recipes, it should probably check for an interruption before running a shebang recipe too? As is, if there are multiple interdependent shebang recipes that themselve ignore ctrl-c, will ctrl-cs be ignored?

It would be nice to add a test for this. It'll likely be complex though; would we have to spin up a process and send it sigint?

[casey]

Alright, just got the time for a real review!

Since the SIGINT handler is global, I think it's okay for the interrupted flag to be global as well, which is possible with atomic bools. Check out this sketch:

https://play.rust-lang.org/?gist=90d1627a4192a2ddfe33f21ccd6f9d45&version=stable&mode=debug&edition=2015

This saves us from threading the interrupted flag through the program.

But, there are a couple more important questions first:

Does this patch give you the desired behavior with your original use-case, with the test runner?

Also I'm a little concerned about installing a ctrl-c handler for the duration of the program, when we only really want to catch ctrl-c while a subprocess is running. If we wind up going this route, I think we can install a ctrl-c handler that terminates the process if a command isn't running, but writes to the interrupted flag if not.

[bheisler]

An automated test for this would be complex, yes. I've done some manual testing with a little Python script that ignores SIGINT and it seemed to work.

I understand the concern about installing a Ctrl-C handler for the entire program, but rust-ctrlc only allows the programmer to set the handler once. We can't unset it later, and we can't set a new handler with a reference to the newest process (instead I'd have to put a reference to the process in shared global storage, and that seems ugly to me). I think setting a flag and then exiting gracefully as soon as we can is the right thing to do. That's what I would expect it to do, as a user.

[casey]

Sorry for taking absolutely forever with this, I've been swamped with work.

I started fooling around with different ways for formulate this, and I wound up writing what's in #345. I feel it's bad open-source etiquette to write an alternative implementation of a pull-request, but I just couldn't figure out or articulate what the right thing to do was without writing it myself. Sometimes my brain doesn't work as well as my hands.

I went with a guard-based approach which a destructor to try to minimize the amount of time we're ignoring interrupts, and to ensure that we abort, even if we panic for some reason.

Testing wasn't actually nearly as bad as I thought it would be. It just runs a justfile with a command that sleeps for two seconds, waits for a second, kills it, and then asserts that it exits in about 1 second, indicating that the signal was ignored for a bit and then handled, and that it returns the right status code.

I had to disable tests on windows, because rust libc on windows doesn't include a kill function, I think because windows doesn't have the same concept of signals as unix.

Would you mind testing #345 and checking that it works with the tests that weren't exiting on ctrl-C?

[bheisler]

Yeah, that seems to have done it. Thanks!