Skip to content

SIGINT and SIGTERM Handling #75

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

Closed
pmorton opened this issue Oct 25, 2017 · 11 comments
Closed

SIGINT and SIGTERM Handling #75

pmorton opened this issue Oct 25, 2017 · 11 comments
Labels
dep: mvdan/sh Issues related to the upstream interpreter used by Task. help wanted Issues that could benefit from members of the community contributing their expertise or experience.

Comments

@pmorton
Copy link

pmorton commented Oct 25, 2017

Hi - I recently discovered go-task. Overall this is pretty impressive! I am utilizing go-task as a makefile replacement on a repository that builds packer (another great go project) images. In the process I have discovered that go-task does not proxy unix signals to child processes. This presents an issue when the task that go-task is running needs to do clean up. For example, when the packer process receives SIGINT (ctrl+c) it will stop what is it doing and execute its cleanup actions so as not to leave random artifacts of half build VMs. Having go-task handle signals in the following way would help to address this issue:

  1. Trap SIGTERM and SIGINT
  2. Send the trapped signal to the child process
  3. Wait for the child process to exit
  4. Exit with the exit code of the child process

As a bonus, if go-task receives SIGINT or SIGTERM more than once, it should exit immediately.

@smyrman
Copy link
Contributor

smyrman commented Oct 25, 2017

How do you handle the case where multiple child-processes are run concurrently through dependenceies?

@pmorton
Copy link
Author

pmorton commented Oct 25, 2017

@smyrman That is an interesting question. I have not thought about this deeply nor have I had the need to deal with parallel termination. Assuming that the parallel processes are not dependent on each other (i.e. they are not doing some sort of IPC or being run as daemons of sorts for use by other processes in the parallel stack) it might be reasonable to do the following:

  1. Trap SIGTERM and SIGINT
  2. Send the trapped signal to each child process. Since processes are not interdependent it should not matter which order the the signal is sent.
  3. Wait for each child process to exit

This is where some choices have to be made

4a. Be like go, Exit with 1. When go traps either SIGINT or SIGTERM it's exit code will always be 1 to indicate abnormal termination.
4.b Be like bash, gnu parallel and the like: Exit with 128 + the signal number. SIGINT = 130, SIGTERM = 143.

IMHO 4b is probably the right choice.

@smyrman
Copy link
Contributor

smyrman commented Oct 25, 2017

I don't suppose you would be interesting in trying a PR? There might be need for some clean-up of the code to get to the stage where it's easy to add. This is a good file to start looking at:

https://github.com/go-task/task/blob/master/task.go

@andreynering
Copy link
Member

/cc @mvdan

Sorry for pinging you again. Do you have any idea if it's possible to send a SIGINT to a process run by the interp?

Today Task only cancels the context and forget about it. I think this will just kill the process.

@andreynering andreynering added enhancement help wanted Issues that could benefit from members of the community contributing their expertise or experience. labels Oct 26, 2017
@mvdan
Copy link

mvdan commented Oct 26, 2017

I haven't thought about signals at all, and nothing is done about them at the moment. But I knew it would become an issue at some point.

At the moment, interp itself just fires goroutines, it of course can't and doesn't fork any processes directly. The only child processes come from os/exec. So the first question to answer would be - what happens when a simple Go program forks a process via os/exec and receives SIGINT? Does the child also receive it? Is there any way to pass the signals on?

If it is possible in a simple program, it is also likely possible in interp somehow. Perhaps it should be configurable in some way.

@andreynering
Copy link
Member

Thanks @mvdan

Seems that the context cancelation calls os.Process.Kill which kills the process.

There's os.Process.Signal. I think it's possible to hook the Context to call that instead of passing the context to os/exec.

@smyrman
Copy link
Contributor

smyrman commented Oct 26, 2017

I don't know if it's worth the complexity, or if it's a good fit for the rest of the code base, but if you think global exit handler would be the right abstraction, you may look at:

@mvdan
Copy link

mvdan commented Oct 29, 2017

@andreynering if you would like to give that a try and submit a patch, I'd be happy to review. It would be best for the solution to not be global, although I'm not sure if that is possible given that signals are per-process and not per-goroutine.

@andreynering
Copy link
Member

mvdan/sh#170

@mvdan
Copy link

mvdan commented Nov 18, 2017

Now merged - thanks! Will release 2.1 soon, but you can vendor an updated master as you've been doing.

@andreynering
Copy link
Member

@pmorton Let us know if the recent changes improved this use case for you.

@andreynering andreynering added the dep: mvdan/sh Issues related to the upstream interpreter used by Task. label Sep 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dep: mvdan/sh Issues related to the upstream interpreter used by Task. help wanted Issues that could benefit from members of the community contributing their expertise or experience.
Projects
None yet
Development

No branches or pull requests

5 participants