Fix data race

[kevinburke]

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.

[lox]

Thanks! How about something like this to avoid the mutex's?

	cmd := exec.Command(input.Command, input.Args...)
	cmd.Env = env
	cmd.Stdin = os.Stdin
	cmd.Stdout = os.Stdout
	cmd.Stderr = os.Stderr

	if err = cmd.Start(); err != nil {
		app.Fatalf("%v", err)
		return
	}

	// wait for the command to finish
	waitCh := make(chan error)
	go func() {
		waitCh <- cmd.Wait()
	}()

	for {
		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())
			}
			if err != nil {
				app.Errorf("%v", err)
				return
			}
		}
	}

[kevinburke]

Done - I think we don't want the for loop, since every branch of the select terminates - the for loop causes one of the examples to run forever.

[lox]

Hmmmm, I suspect we will need the for loop, otherwise receiving any signal before the process finishes will cause it to terminate. Theoretically it should support multiple signals too, although the current implementation doesn't.

Which example will run forever?

[kevinburke]

This one:

func ExampleExecCommand() {
	awsConfigFile = &vault.FileConfig{}
	keyringImpl = keyring.NewArrayKeyring([]keyring.Item{
		{Key: "llamas", Data: []byte(`{"AccessKeyID":"ABC","SecretAccessKey":"XYZ"}`)},
	})

	app := kingpin.New("aws-vault", "")
	ConfigureGlobals(app)
	ConfigureExecCommand(app)
	kingpin.MustParse(app.Parse([]string{
		"--debug", "exec", "--no-session", "llamas", "--", "sh", "-c", "echo $AWS_ACCESS_KEY_ID",
	}))

	// Output:
	// ABC
}

[lox]

Hmmm. Ah right, it needs a break at the end of the <-waitCh case even if the error is nil.

	cmd := exec.Command(input.Command, input.Args...)
	cmd.Env = env
	cmd.Stdin = os.Stdin
	cmd.Stdout = os.Stdout
	cmd.Stderr = os.Stderr

	if err = cmd.Start(); err != nil {
		app.Fatalf("%v", err)
		return
	}

	// wait for the command to finish
	waitCh := make(chan error)
	go func() {
		waitCh <- cmd.Wait()
	}()

	for {
		select {
                // listen for signals and send them to the process
		case sig := <-input.Signals:
			if err = cmd.Process.Signal(sig); err != nil {
				app.Errorf("%v", err)
				break
			}
                // wait for return to exit
		case err := <-waitCh:
			var waitStatus syscall.WaitStatus
			if exitError, ok := err.(*exec.ExitError); ok {
				waitStatus = exitError.Sys().(syscall.WaitStatus)
				os.Exit(waitStatus.ExitStatus())
			}
			if err != nil {
				app.Errorf("%v", err)
			}
                        break
		}
	}

[kevinburke]

Apologies; if we try the code you pasted, I still get a hang in the test.

I think what we need is to break/continue processing after we send a signal, but to always return if the underlying program exits/ we receive a value on waitCh. The diff I just pushed does that.

[lox]

Looks good, thanks!