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

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

Closed
dqminh opened this issue Jul 23, 2017 · 12 comments

Comments

@dqminh
Copy link

dqminh commented Jul 23, 2017

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 odeke-em changed the title User of CommandContext should be able to specify the kill signal when context is done os/exec: allow user of CommandContext to specify the kill signal when context is done Jul 23, 2017
@odeke-em
Copy link
Member

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

@odeke-em odeke-em added this to the Go1.10 milestone Jul 23, 2017
@odeke-em
Copy link
Member

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
Copy link

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

@odeke-em odeke-em changed the title os/exec: allow user of CommandContext to specify the kill signal when context is done proposal: os/exec: allow user of CommandContext to specify the kill signal when context is done Aug 21, 2017
@rsc
Copy link
Contributor

rsc commented Aug 21, 2017

/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
Copy link
Contributor

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
Copy link
Author

dqminh commented Aug 23, 2017

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)
  1. 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
Copy link
Contributor

bcmills commented Aug 23, 2017

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
Copy link
Contributor

rsc commented Aug 28, 2017

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
Copy link

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
Copy link
Contributor

rsc commented Sep 25, 2017

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.

@rsc rsc closed this as completed Sep 25, 2017
@ghost
Copy link

ghost commented Oct 30, 2017

Would be nice :)

@odeke-em
Copy link
Member

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants