proposal: os/exec: allow user of CommandContext to specify the kill signal when context is done

[dqminh]

What version of Go are you using (go version)?

go version go1.8.3 linux/amd64

Currently, process started from exec.CommandContext will be killed by SIGKILL when the context is done before the process finished. But in some cases, the process should be allow a chance to clean up then exit cleanly ( by received an expected signal like SIGTERM for example). We received a similar request in containerd/go-runc#21

[odeke-em]

Sounds like a feature request, I'll triage this for Go1.10.
/cc @bradfitz @ianlancetaylor.

[odeke-em]

In regards to the design, I can see perhaps adding a field to *exec.Cmd

Cmd struct {
   KillSignal os.Signal
}

or better, making a method

func (c *Cmd) SetKillSignal(signal os.Signal) {
     c.sigmu.Lock()
     c.killSignal = &signal
     c.sigmu.Unlock()
}

and then for implementation we can do something altogether like this

diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go
index 893d8ee..9c541d2 100644
--- a/src/os/exec/exec.go
+++ b/src/os/exec/exec.go
@@ -113,6 +113,9 @@ type Cmd struct {
 	// available after a call to Wait or Run.
 	ProcessState *os.ProcessState
 
+	sigMu      sync.RWMutex // sigMu protects killSignal
+	killSignal *os.Signal
+
 	ctx             context.Context // nil means none
 	lookPathErr     error           // LookPath error, if any.
 	finished        bool            // when Wait was called
@@ -386,7 +389,14 @@ func (c *Cmd) Start() error {
 		go func() {
 			select {
 			case <-c.ctx.Done():
-				c.Process.Kill()
+				c.sigMu.RLock()
+				sig := c.killSignal
+				c.sigMu.RUnlock()
+				if sig == nil {
+					c.Process.Kill()
+				} else {
+					c.Process.Signal(*sig)
+				}
 			case <-c.waitDone:
 			}
 		}()
@@ -395,6 +405,14 @@ func (c *Cmd) Start() error {
 	return nil
 }
 
+func (c *Cmd) SetKillSignal(sig os.Signal) {
+	c.sigMu.Lock()
+	cpSig := new(os.Signal)
+	*cpSig = sig
+	c.killSignal = cpSig
+	c.sigMu.Unlock()
+}
+
 // An ExitError reports an unsuccessful exit by a command.
 type ExitError struct {
 	*os.ProcessState

However, am not so sure if this design will be cross platform. Some signals differ between Windows and the *NIXes, so I'll defer to @ianlancetaylor to advise me on that.

[gopherbot]

CL https://golang.org/cl/50830 mentions this issue.

[rsc]

/cc @bradfitz

It's not clear to me that CommandContext needed to be in os/exec to begin with. You can certainly build other signaling semantics on top of os/exec without it. It's unclear we should embellish this one instead of just encouraging people to do other things (like SIGTERM, sleep 5 seconds, then SIGKILL) with code in other packages.

[bradfitz]

Maybe not but I sure do appreciate it now that we have it. Recent example: https://twitter.com/bradfitz/status/883357040919232512

I would've risked leaking processes otherwise, or it would've been tediously long.

[dqminh]

yah, having os/exec handled context for you is certainly very convenient, but I think @rsc is also quite right that most of the time if user want to handle terminating the process gracefully, adding the custom signal alone probably wont be enough, you probably want to SIGTERM, timeout, then SIGKILL.

1. Handle this behaviour out of os/exec, probably with some pseudo-code like this:

cmd := exec.Command("sleep", "10")
cmd.Start()
waitDone := make(chan struct{})
go func() {
	select {
	case <-ctx.Done():
		cmd.Process.Signal(syscall.SIGTERM)
		select {
		case <-time.After(time.Second):
			cmd.Process.Kill()
		case <-waitDone:
		}
	case <-waitDone:
	}
}()
cmd.Wait()
close(waitDone)

2. Instead of adding the custom signal, adding a callback to let user specify custom behavior

cmd := exec.CommandContext(ctx, "sleep", "10")
// reuse waitDone in exec.Cmd
cmd.ContextDone = func(p *os.Process, waitDone <-chan struct{}) {
	p.Signal(syscall.SIGTERM)
	select {
	case <-time.After(time.Second):
		p.Kill()
	case <-waitDone:
	}
}
cmd.Start()
cmd.Wait()

c.waitDone = make(chan struct{})
go func() {
	select {
	case <-c.ctx.Done():
		if c.ContextDone == nil {
			c.Process.Kill()
		} else {
			c.ContextDone(c.Process, c.waitDone)
		}
	case <-c.waitDone:
	}
}()

[bcmills]

you probably want to SIGTERM, timeout, then SIGKILL

When we cancel a context.Context, we do not follow up by terminating the process running the goroutine. Why do assume clean, responsive terminations within the process but not in subprocesses?

Just changing the signal should work fine for well-behaved programs, and it is up to the caller to decide whether the program they are executing is likely to be well-behaved.

[rsc]

OK, well, ignoring the fact that the CommandContext ship has sailed, the rest of what I wrote still applies:

You can certainly build other signaling semantics on top of os/exec without it. It's unclear we should embellish this one instead of just encouraging people to do other things (like SIGTERM, sleep 5 seconds, then SIGKILL) with code in other packages.

If you want a custom signal it's still easy to build outside of the standard library. Is there enough demand for putting this in the standard library? It doesn't sound like it.

[crosbymichael]

Could you add a method to set a signal on the context using the Value() methods?

func ContextSignal(ctx context.Context, sig os.Signal) context.Context { }

ctx = exec.ContextSignal(ctx, unix.SIGTERM)
cmd := exec.CommandContext(ctx, ...)

[rsc]

There still has been no clear signal of demand that forces this into the standard library (no responses to my comment from Aug 28 above). Let's let this functionality be provided by packages outside the standard library for now.

[ghost]

Would be nice :)

[odeke-em]

Closed proposal but am just directing demand here as #22757 has just been opened and seems like a subset of this issue.