proposal: os/exec: add option to send Interrupt instead of Kill on Context cancellation #22757
Comments
|
@andreynering this issue looks like a subset of the closed proposal #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. |
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. |
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...")
} |
|
@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. |
|
@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: |
You're right! Using Anyway, I still think the proposal should be considered:
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...")
} |
|
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. |
|
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. |
|
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 thanks! You showed a treasure I was not aware of. |
This is important to make the backend runner able to support partial traces; the default CommandContext setup always sigkills, and we need to sigterm things like rmem to get them to dump a partial result. Seems to work; could do with some testing, but how?, and refactoring, but how?. This isn't yet supported in the machine node, mainly because that doesn't quite support the service.Runner idiom yet. See golang/go#22757.
|
@bcmills thanks for the link above. I'm a bit curious - isn't using that function for "graceful shutdown" on windows largely unhelpful, since os.Interrupt is not supported there? In other words, waitOrStop on Windows seems to kill the process immediately after the context is done, meaning waitOrStop isn't really improving things for Windows. I'm once again wanting to have a "graceful shutdown of an |
|
@mvdan, I honestly haven't thought much about Windows since I'm mostly using the function to get better test output in case of deadlocks, and (in the programs I'm working with) deadlocks that occur on Windows also tend to occur elsewhere. That said, I could easily imagine a variant of the above |
I’m not competent enough in UNIX, so may I ask whether it makes sense to leave |
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 -raceindicates a race when doing so, which I think it's not fixable because the internal mutex is unexported.Background:
The text was updated successfully, but these errors were encountered: