Skip to content
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

Closed
CGamesPlay opened this issue May 12, 2022 · 2 comments · Fixed by #58
Closed

Robo abandons children #55

CGamesPlay opened this issue May 12, 2022 · 2 comments · Fixed by #58

Comments

@CGamesPlay
Copy link
Contributor

(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 is cli.Run, since it's a global effect, and putting it in Task.Run feels like a violation of the Task encapsulation.

Would a PR implementing this behavior be accepted?

@samsarkleio
Copy link

+1, this would be quite useful. I think it makes sense in the cli.Run method too.

@yields
Copy link
Collaborator

yields commented Jul 14, 2022

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
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants