From f5a9c7ed5ab58afa255c2f405b30f06f88a6aecf Mon Sep 17 00:00:00 2001
From: Alberto Cortés <alcortesm@gmail.com>
Date: Wed, 15 Feb 2017 12:41:02 +0100
Subject: transport/file: fix race condition on test (#267)

Sometimes, the `TestClone` and `TestPush` tests of `transport/file` fail
in travis.

This is due to a race condition caused by an incorrect usage of
`Cmd.Wait()` while reading from the output and error pipes of the
command.

This patch fixes the problem by using `Cmd.CombinedOutput()` instead of
calling `Cmd.Start()` and `Cmd.Wait()` while reading from the output and
error pipes.

Details:

From the `exec` package documentation:

```
Wait will close the pipe after seeing the command exit, so most callers
need not close the pipe themselves; however, an implication is that it
is incorrect to call Wait before all reads from the pipe have completed.
For the same reason, it is incorrect to call Run when using StdoutPipe.
```

In our tests, the old `execAndGetOutput` function was creating two
gorutines to read from the stderr and stdout pipes of the command and
then call `Wait` on the command.

This caused a race condition: when the `Wait` call finished before the
gorutines have read from the pipes, they returned caused an error on
`outErr`.

The problem only happens sometimes on travis.

To reproduce the problem locally, just add a call to
time.Sleep(time.Second) to the gorutine before its `ioutil.ReadAll` call
to delay them, then `Wait` will always finish before them, closing the
pipes, and the gorutines will fail.  The returned error detected by the
test will be:

```
FAIL: server_test.go:55: ServerSuite.TestClone
server_test.go:65:
    c.Assert(err, IsNil, Commentf("STDOUT:\n%s\nSTDERR:\n%s\n", stdout, stderr))
    ... value *os.PathError = &os.PathError{Op:"read", Path:"|0", Err:0x9} ("read |0: bad file descriptor")
    ... STDOUT:
    STDERR:
```
---
 plumbing/transport/file/server_test.go | 49 +++-------------------------------
 1 file changed, 4 insertions(+), 45 deletions(-)

(limited to 'plumbing/transport')

diff --git a/plumbing/transport/file/server_test.go b/plumbing/transport/file/server_test.go
index 775b031..a7b4e34 100644
--- a/plumbing/transport/file/server_test.go
+++ b/plumbing/transport/file/server_test.go
@@ -2,8 +2,6 @@ package file
 
 import (
 	"fmt"
-	"io"
-	"io/ioutil"
 	"os"
 	"os/exec"
 
@@ -48,8 +46,8 @@ func (s *ServerSuite) TestPush(c *C) {
 	cmd.Dir = s.SrcPath
 	cmd.Env = os.Environ()
 	cmd.Env = append(cmd.Env, "GIT_TRACE=true", "GIT_TRACE_PACKET=true")
-	stdout, stderr, err := execAndGetOutput(c, cmd)
-	c.Assert(err, IsNil, Commentf("STDOUT:\n%s\nSTDERR:\n%s\n", stdout, stderr))
+	out, err := cmd.CombinedOutput()
+	c.Assert(err, IsNil, Commentf("combined stdout and stderr:\n%s\n", out))
 }
 
 func (s *ServerSuite) TestClone(c *C) {
@@ -61,45 +59,6 @@ func (s *ServerSuite) TestClone(c *C) {
 	)
 	cmd.Env = os.Environ()
 	cmd.Env = append(cmd.Env, "GIT_TRACE=true", "GIT_TRACE_PACKET=true")
-	stdout, stderr, err := execAndGetOutput(c, cmd)
-	c.Assert(err, IsNil, Commentf("STDOUT:\n%s\nSTDERR:\n%s\n", stdout, stderr))
-}
-
-func execAndGetOutput(c *C, cmd *exec.Cmd) (stdout, stderr string, err error) {
-	sout, err := cmd.StdoutPipe()
-	c.Assert(err, IsNil)
-	serr, err := cmd.StderrPipe()
-	c.Assert(err, IsNil)
-
-	outChan, outErr := readAllAsync(sout)
-	errChan, errErr := readAllAsync(serr)
-
-	c.Assert(cmd.Start(), IsNil)
-
-	if err = cmd.Wait(); err != nil {
-		return <-outChan, <-errChan, err
-	}
-
-	if err := <-outErr; err != nil {
-		return <-outChan, <-errChan, err
-	}
-
-	return <-outChan, <-errChan, <-errErr
-}
-
-func readAllAsync(r io.Reader) (out chan string, err chan error) {
-	out = make(chan string, 1)
-	err = make(chan error, 1)
-	go func() {
-		b, e := ioutil.ReadAll(r)
-		if e != nil {
-			err <- e
-		} else {
-			err <- nil
-		}
-
-		out <- string(b)
-	}()
-
-	return out, err
+	out, err := cmd.CombinedOutput()
+	c.Assert(err, IsNil, Commentf("combined stdout and stderr:\n%s\n", out))
 }
-- 
cgit