Merge pull request #203 from bogdanteleaga/exec-fixes

exec: always return at least an empty ExecResponse

I've had some really fringe errors with some code that still tried to check the ExecResponse even though the err is there deep inside the uniter(and it might exist in other places). Point is it's safer to always return at least an empty ExecResponse.

(Review request: http://reviews.vapour.ws/r/4386/)

exec/exec.go

@@ -196,7 +196,12 @@ func (r *RunParams) Wait() (*ExecResponse, error) {
 		}
 		logger.Infof("run result: %v", ee)
 	}
-	return result, err
+
+	if err != nil {
+		return nil, errors.Trace(err)
+	}
+
+	return result, nil
 }
 
 // ErrCancelled is returned by WaitWithCancel in case it successfully manages to kill
@@ -232,7 +237,10 @@ func (r *RunParams) WaitWithCancel(cancel <-chan struct{}) (*ExecResponse, error
 
 	select {
 	case resWithError := <-done:
-		return resWithError.execResult, errors.Trace(resWithError.err)
+		if resWithError.err != nil {
+			return nil, errors.Trace(resWithError.err)
+		}
+		return resWithError.execResult, nil
 	case <-cancel:
 		logger.Debugf("attempting to kill process")
 		err := r.KillProcess(r.ps.Process)
@@ -243,8 +251,8 @@ func (r *RunParams) WaitWithCancel(cancel <-chan struct{}) (*ExecResponse, error
 		// After we issue a kill we expect the wait above to return within timeWaitForKill.
 		// In case it doesn't we just go on and assume the process is stuck, but we don't block
 		select {
-		case resWithError := <-done:
-			return resWithError.execResult, ErrCancelled
+		case <-done:
+			return nil, ErrCancelled
 		case <-_clock.After(timeWaitForKill):
 			return nil, errors.Errorf("tried to kill process %v, but timed out", r.ps.Process.Pid)
 		}

exec/exec_linux_test.go

@@ -10,10 +10,6 @@ import (
 	"github.com/juju/utils/exec"
 )
 
-// 0 is thrown by linux because RunParams.Wait
-// only sets the code if the process exits cleanly
-const cancelErrCode = 0
-
 func (*execSuite) TestRunCommands(c *gc.C) {
 	newDir := c.MkDir()
 

exec/exec_test.go

@@ -38,9 +38,7 @@ func (*execSuite) TestWaitWithCancel(c *gc.C) {
 	cancelChan <- struct{}{}
 	result, err := params.WaitWithCancel(cancelChan)
 	c.Assert(err, gc.Equals, exec.ErrCancelled)
-	c.Assert(string(result.Stdout), gc.Equals, "")
-	c.Assert(string(result.Stderr), gc.Equals, "")
-	c.Assert(result.Code, gc.Equals, cancelErrCode)
+	c.Assert(result, gc.IsNil)
 }
 
 func (s *execSuite) TestKillAbortedIfUnsuccessfull(c *gc.C) {

exec/exec_windows_test.go

@@ -14,9 +14,6 @@ import (
 	"github.com/juju/utils/exec"
 )
 
-// 1 is thrown by powershell after the a command is cancelled
-const cancelErrCode = 1
-
 // longPath is copied over from the symlink package. This should be removed
 // if we add it to gc or in some other convenience package
 func longPath(path string) ([]uint16, error) {