Skip to content

Commit

Permalink
Fix data race
Browse files Browse the repository at this point in the history
We try to read cmd.Process in the thread that handles incoming
signals, but there's no synchronization between that goroutine and the
one that sets cmd.Process to a non-nil value.

This might be overkill, e.g. there may be a way to write this with
less code, but I confirmed that this patch addresses the
race when shutting down the program.

Fixes #158.
  • Loading branch information
kevinburke committed Oct 12, 2017
1 parent 1a2907d commit 761159b
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 7 deletions.
21 changes: 15 additions & 6 deletions cli/exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -161,15 +161,24 @@ func ExecCommand(app *kingpin.Application, input ExecCommandInput) {
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr

if err := cmd.Start(); err != nil {
app.Errorf("%v", err)
return
}
// wait for the command to finish
waitCh := make(chan error)
go func() {
sig := <-input.Signals
if cmd.Process != nil {
cmd.Process.Signal(sig)
}
waitCh <- cmd.Wait()
}()

var waitStatus syscall.WaitStatus
if err := cmd.Run(); err != nil {
select {
case sig := <-input.Signals:
if err = cmd.Process.Signal(sig); err != nil {
app.Errorf("%v", err)
return
}
case err := <-waitCh:
var waitStatus syscall.WaitStatus
if exitError, ok := err.(*exec.ExitError); ok {
waitStatus = exitError.Sys().(syscall.WaitStatus)
os.Exit(waitStatus.ExitStatus())
Expand Down
2 changes: 1 addition & 1 deletion cli/exec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ func ExampleExecCommand() {
{Key: "llamas", Data: []byte(`{"AccessKeyID":"ABC","SecretAccessKey":"XYZ"}`)},
})

app := kingpin.New(`aws-vault`, ``)
app := kingpin.New("aws-vault", "")
ConfigureGlobals(app)
ConfigureExecCommand(app)
kingpin.MustParse(app.Parse([]string{
Expand Down

0 comments on commit 761159b

Please sign in to comment.