Go: proposal: os/exec: add option to send Interrupt instead of Kill on Context cancellation

[andreynering]

The reason is that some executables do cleanup on Interrupt, like deleting temporary files.

Ideally it should send Interrupt but force a Kill after a timeout (of 2 seconds?). Also, it should be no-op on Windows that don't support sending Interrupt (and just send Kill instead).

It's already possible to manually listen to the Context and send the signals, but go test -race indicates a race when doing so, which I think it's not fixable because the internal mutex is unexported.

Background:

• 1
• 2

[odeke-em]

@andreynering this issue looks like a subset of the closed proposal https://github.com/golang/go/issues/21135 which was closed due to lack of demand since no one responded to @rsc's request for comments so an indication of less demand.

[bcmills]

go test -race indicates a race when doing so

Please include the actual code you tried (ideally as a Playground link) and the actual error you got. If the obvious way to do it is racy, perhaps we can just fix the race.

[andreynering]

@bcmills

package main

import (
    "context"
    "fmt"
    "os"
    "os/exec"
    "runtime"
    "time"
)

func main() {
    fmt.Println("Starting...")

    ctx, _ := context.WithTimeout(context.Background(), 2*time.Second)
    cmd := exec.CommandContext(ctx, "sleep", "5")

    go func() {
        <-ctx.Done()

        // Windows doesn't support Interrupt
        if runtime.GOOS == "windows" {
            _ = cmd.Process.Signal(os.Kill)
            return
        }

        go func() {
            time.Sleep(2 * time.Second)
            _ = cmd.Process.Signal(os.Kill)
        }()
        cmd.Process.Signal(os.Interrupt)
    }()

    if err := cmd.Run(); err != nil {
        panic(err)
    }

    fmt.Println("Finishing...")
}

[rsc]

@andreynering, sorry but can you also post the output you get from running that program with -race? I am not getting any race failures when I use "go run -race x.go" (where x.go is your program).

It's possible that the race you are seeing is on cmd.Process, which cmd.Run initializes and your goroutine uses. If so the solution would be to run cmd.Start then kick off the goroutine, then use cmd.Wait. Start is what initializes cmd.Process, so the goroutine does need to run only after Start has done that.

[andreynering]

@rsc Sorry, there was a mistake in that script. Just change this line:

- cmd := exec.CommandContext(ctx, "sleep", "5")
+ cmd := exec.Command("sleep", "5")

And the output:

$ go run -race signal.go
Starting...
==================
WARNING: DATA RACE
Read at 0x00c42007a200 by goroutine 7:
  main.main.func1()
      /home/andrey/signal.go:31 +0x90

Previous write at 0x00c42007a200 by main goroutine:
  os/exec.(*Cmd).Start()
      /home/andrey/GOROOT/src/os/exec/exec.go:363 +0x8d6
  os/exec.(*Cmd).Run()
      /home/andrey/GOROOT/src/os/exec/exec.go:286 +0x3c
  main.main()
      /home/andrey/signal.go:34 +0x190

Goroutine 7 (running) created at:
  main.main()
      /home/andrey/signal.go:18 +0x182
==================
==================
WARNING: DATA RACE
Read at 0x00c4200800c0 by goroutine 7:
  os.(*Process).signal()
      /home/andrey/GOROOT/src/os/exec_unix.go:56 +0x51
  os.(*Process).Signal()
      /home/andrey/GOROOT/src/os/exec.go:121 +0x4c
  main.main.func1()
      /home/andrey/signal.go:31 +0xcd

Previous write at 0x00c4200800c0 by main goroutine:
  os.newProcess()
      /home/andrey/GOROOT/src/os/exec.go:24 +0x5d
  os.startProcess()
      /home/andrey/GOROOT/src/os/exec_posix.go:49 +0x542
  os.StartProcess()
      /home/andrey/GOROOT/src/os/exec.go:94 +0x7a
  os/exec.(*Cmd).Start()
      /home/andrey/GOROOT/src/os/exec/exec.go:363 +0x89b
  os/exec.(*Cmd).Run()
      /home/andrey/GOROOT/src/os/exec/exec.go:286 +0x3c
  main.main()
      /home/andrey/signal.go:34 +0x190

Goroutine 7 (running) created at:
  main.main()
      /home/andrey/signal.go:18 +0x182
==================
==================
WARNING: DATA RACE
Read at 0x00c4200800d0 by goroutine 7:
  sync/atomic.LoadInt32()
      /home/andrey/GOROOT/src/runtime/race_amd64.s:206 +0xb
  os.(*Process).done()
      /home/andrey/GOROOT/src/os/exec.go:34 +0x3e
  os.(*Process).signal()
      /home/andrey/GOROOT/src/os/exec_unix.go:64 +0x1dc
  os.(*Process).Signal()
      /home/andrey/GOROOT/src/os/exec.go:121 +0x4c
  main.main.func1()
      /home/andrey/signal.go:31 +0xcd

Previous write at 0x00c4200800d0 by main goroutine:
  os.newProcess()
      /home/andrey/GOROOT/src/os/exec.go:24 +0x5d
  os.startProcess()
      /home/andrey/GOROOT/src/os/exec_posix.go:49 +0x542
  os.StartProcess()
      /home/andrey/GOROOT/src/os/exec.go:94 +0x7a
  os/exec.(*Cmd).Start()
      /home/andrey/GOROOT/src/os/exec/exec.go:363 +0x89b
  os/exec.(*Cmd).Run()
      /home/andrey/GOROOT/src/os/exec/exec.go:286 +0x3c
  main.main()
      /home/andrey/signal.go:34 +0x190

Goroutine 7 (running) created at:
  main.main()
      /home/andrey/signal.go:18 +0x182
==================
panic: signal: interrupt

goroutine 1 [running]:
main.main()
        /home/andrey/signal.go:35 +0x248
exit status 2

[andreynering]

It's possible that the race you are seeing is on cmd.Process, which cmd.Run initializes and your goroutine uses. If so the solution would be to run cmd.Start then kick off the goroutine, then use cmd.Wait. Start is what initializes cmd.Process, so the goroutine does need to run only after Start has done that.

You're right! Using Start() and Wait() fixes the race. (Updated script below).

Anyway, I still think the proposal should be considered:

• It's convenient to have that in the stdlib;
• It's a common use case;
• It's too easy to someone commit this mistake and have a racy code.

---

package main

import (
    "context"
    "fmt"
    "os"
    "os/exec"
    "runtime"
    "time"
)

func main() {
    fmt.Println("Starting...")

    ctx, _ := context.WithTimeout(context.Background(), 2*time.Second)
    cmd := exec.Command("sleep", "5")
    if err := cmd.Start(); err != nil {
        panic(err)
    }

    go func() {
        <-ctx.Done()

        // Windows doesn't support Interrupt
        if runtime.GOOS == "windows" {
            _ = cmd.Process.Signal(os.Kill)
            return
        }

        go func() {
            time.Sleep(2 * time.Second)
            _ = cmd.Process.Signal(os.Kill)
        }()
        cmd.Process.Signal(os.Interrupt)
    }()

    if err := cmd.Wait(); err != nil {
        panic(err)
    }

    fmt.Println("Finishing...")
}

[rsc]

As noted earlier, we did consider it at length in #21135 but decided not to. You can see the discussion there. We're not going to revisit this now, but maybe in a year if more evidence of demand accumulates. Sorry.

[marco-m]

My 2 cents to indicate additional demand to reconsider the option of being able to send SIGINT (+ timeout + SIGKILL) instead of SIGKILL on Context cancellation.

[cpuguy83]

I threw together a wrapper to handle this: https://github.com/cpuguy83/execctx

It indeed seems iffy to include something like this in the stdlib.

[bcmills]

@cpuguy83, we've had a bit more experience with gracefully terminating subprocesses in various parts of the Go project, and the pattern that is emerging seems to be a variant of (*exec.Cmd).Wait rather than exec.Command:
https://github.com/golang/go/blob/cacac8bdc5c93e7bc71df71981fdf32dded017bf/src/cmd/go/script_test.go#L1091-L1098

[marco-m]

@bcmills thanks! You showed a treasure I was not aware of.