-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Robo abandons children #55
Comments
+1, this would be quite useful. I think it makes sense in the |
Hey @CGamesPlay a PR would be awesome, I can include it in the upcoming release as well |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
(Sorry, I couldn't resist that issue name.) Hi, I'm a big fan of robo, and I've been using it increasingly over the past year or so. I've recently noticed that robo will return control to the shell immediately when it receives a SIGINT. This is a problem, because child processes of robo may still be running cleanup behavior. The upshot of this is that any program which has cleanup behavior on exit will still be running in the background, and if it produces any output (or changes terminal modes), it will be printed in the middle of the user's shell prompt.
For a quick demo showing this behavior, see this gist. I added a Makefile to compare the behavior against make, which I would argue is the correct behavior: it catches the signal, stops running new commands, waits for all child processes to exit, and only then exits itself.
In order to bring robo inline with make, I think Robo just needs to ignore the relevant signals. The child processes are already delivered the signal as well, so the
cmd.Run()
call will terminate naturally. I think the proper signals to catch are SIGINT and SIGTERM. There's a stylistic question about where this behavior should live. My suggestion iscli.Run
, since it's a global effect, and putting it inTask.Run
feels like a violation of theTask
encapsulation.Would a PR implementing this behavior be accepted?
The text was updated successfully, but these errors were encountered: