get communicator errors from a remote.Cmd

The remote.Cmd struct could not convey any transport related error to
the caller, meaning that interrupted commands would show that they
succeeded.

Change Cmd.SetExited to accept an exit status, as well as an error to
store for the caller.  Make the status and error fields internal,
require serialized access through the getter methods.

Users of remote.Cmd should not check both Cmd.Err() and Cmd.ExitStatus()
until after Wait returns.

Require communicators to call Cmd.Init before executing the command.
This will indicate incorrect usage of the remote.Cmd by causing a panic
in SetExitStatus.

communicator/communicator_mock.go

@@ -42,11 +42,13 @@ func (c *MockCommunicator) ScriptPath() string {
 
 // Start implementation of communicator.Communicator interface
 func (c *MockCommunicator) Start(r *remote.Cmd) error {
+	r.Init()
+
 	if !c.Commands[r.Command] {
 		return fmt.Errorf("Command not found!")
 	}
 
-	r.SetExited(0)
+	r.SetExitStatus(0, nil)
 
 	return nil
 }

communicator/remote/command.go

@@ -23,45 +23,59 @@ type Cmd struct {
 	Stdout io.Writer
 	Stderr io.Writer
 
-	// This will be set to true when the remote command has exited. It
+	// Once Wait returns, his will contain the exit code of the process.
-	// shouldn't be set manually by the user, but there is no harm in
+	exitStatus int
-	// doing so.
-	Exited bool
-
-	// Once Exited is true, this will contain the exit code of the process.
-	ExitStatus int
 
 	// Internal fields
 	exitCh chan struct{}
 
+	// err is used to store any error reported by the Communicator during
+	// execution.
+	err error
+
 	// This thing is a mutex, lock when making modifications concurrently
 	sync.Mutex
 }
 
-// SetExited is a helper for setting that this process is exited. This
+// Init must be called by the Communicator before executing the command.
-// should be called by communicators who are running a remote command in
+func (c *Cmd) Init() {
-// order to set that the command is done.
+	c.Lock()
-func (r *Cmd) SetExited(status int) {
+	defer c.Unlock()
-	r.Lock()
-	defer r.Unlock()
 
-	if r.exitCh == nil {
+	c.exitCh = make(chan struct{})
-		r.exitCh = make(chan struct{})
+}
-	}
 
-	r.Exited = true
+// SetExitStatus stores the exit status of the remote command as well as any
-	r.ExitStatus = status
+// communicator related error. SetExitStatus then unblocks any pending calls
-	close(r.exitCh)
+// to Wait.
+// This should only be called by communicators executing the remote.Cmd.
+func (c *Cmd) SetExitStatus(status int, err error) {
+	c.Lock()
+	defer c.Unlock()
+
+	c.exitStatus = status
+	c.err = err
+
+	close(c.exitCh)
+}
+
+// Err returns any communicator related error.
+func (c *Cmd) Err() error {
+	c.Lock()
+	defer c.Unlock()
+
+	return c.err
+}
+
+// ExitStatus returns the exit status of the remote command
+func (c *Cmd) ExitStatus() int {
+	c.Lock()
+	defer c.Unlock()
+
+	return c.exitStatus
 }
 
 // Wait waits for the remote command to complete.
-func (r *Cmd) Wait() {
+func (c *Cmd) Wait() {
-	// Make sure our condition variable is initialized.
+	<-c.exitCh
-	r.Lock()
-	if r.exitCh == nil {
-		r.exitCh = make(chan struct{})
-	}
-	r.Unlock()
-
-	<-r.exitCh
 }

communicator/ssh/communicator.go

@@ -243,6 +243,8 @@ func (c *Communicator) ScriptPath() string {
 
 // Start implementation of communicator.Communicator interface
 func (c *Communicator) Start(cmd *remote.Cmd) error {
+	cmd.Init()
+
 	session, err := c.newSession()
 	if err != nil {
 		return err
@@ -267,7 +269,7 @@ func (c *Communicator) Start(cmd *remote.Cmd) error {
 	}
 
 	log.Printf("starting remote command: %s", cmd.Command)
-	err = session.Start(cmd.Command + "\n")
+	err = session.Start(strings.TrimSpace(cmd.Command) + "\n")
 	if err != nil {
 		return err
 	}
@@ -286,8 +288,8 @@ func (c *Communicator) Start(cmd *remote.Cmd) error {
 			}
 		}
 
+		cmd.SetExitStatus(exitStatus, err)
 		log.Printf("remote command exited with '%d': %s", exitStatus, cmd.Command)
-		cmd.SetExited(exitStatus)
 	}()
 
 	return nil
@@ -358,10 +360,10 @@ func (c *Communicator) UploadScript(path string, input io.Reader) error {
 				"machine: %s", err)
 	}
 	cmd.Wait()
-	if cmd.ExitStatus != 0 {
+	if cmd.ExitStatus() != 0 {
 		return fmt.Errorf(
 			"Error chmodding script file to 0777 in remote "+
-				"machine %d: %s %s", cmd.ExitStatus, stdout.String(), stderr.String())
+				"machine %d: %s %s", cmd.ExitStatus(), stdout.String(), stderr.String())
 	}
 
 	return nil

communicator/ssh/communicator_test.go

@@ -17,6 +17,7 @@ import (
 	"strconv"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/hashicorp/terraform/communicator/remote"
 	"github.com/hashicorp/terraform/terraform"
@@ -178,6 +179,55 @@ func TestStart(t *testing.T) {
 	}
 }
 
+func TestLostConnection(t *testing.T) {
+	address := newMockLineServer(t, nil)
+	parts := strings.Split(address, ":")
+
+	r := &terraform.InstanceState{
+		Ephemeral: terraform.EphemeralState{
+			ConnInfo: map[string]string{
+				"type":     "ssh",
+				"user":     "user",
+				"password": "pass",
+				"host":     parts[0],
+				"port":     parts[1],
+				"timeout":  "30s",
+			},
+		},
+	}
+
+	c, err := New(r)
+	if err != nil {
+		t.Fatalf("error creating communicator: %s", err)
+	}
+
+	var cmd remote.Cmd
+	stdout := new(bytes.Buffer)
+	cmd.Command = "echo foo"
+	cmd.Stdout = stdout
+
+	err = c.Start(&cmd)
+	if err != nil {
+		t.Fatalf("error executing remote command: %s", err)
+	}
+
+	// The test server can't execute anything, so Wait will block, unless
+	// there's an error.  Disconnect the communicator transport, to cause the
+	// command to fail.
+	go func() {
+		time.Sleep(100 * time.Millisecond)
+		c.Disconnect()
+	}()
+
+	cmd.Wait()
+	if cmd.Err() == nil {
+		t.Fatal("expected communicator error")
+	}
+	if cmd.ExitStatus() != 0 {
+		t.Fatal("command should not have returned an exit status")
+	}
+}
+
 func TestHostKey(t *testing.T) {
 	// get the server's public key
 	signer, err := ssh.ParsePrivateKey([]byte(testServerPrivateKey))

communicator/winrm/communicator.go

@@ -131,6 +131,8 @@ func (c *Communicator) ScriptPath() string {
 
 // Start implementation of communicator.Communicator interface
 func (c *Communicator) Start(rc *remote.Cmd) error {
+	rc.Init()
+
 	err := c.Connect(nil)
 	if err != nil {
 		return err
@@ -168,7 +170,8 @@ func runCommand(shell *winrm.Shell, cmd *winrm.Command, rc *remote.Cmd) {
 
 	cmd.Wait()
 	wg.Wait()
-	rc.SetExited(cmd.ExitCode())
+
+	rc.SetExitStatus(cmd.ExitCode(), nil)
 }
 
 // Upload implementation of communicator.Communicator interface