aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorAlberto Cortés <alcortesm@gmail.com>2017-02-15 12:41:02 +0100
committerGitHub <noreply@github.com>2017-02-15 12:41:02 +0100
commitf5a9c7ed5ab58afa255c2f405b30f06f88a6aecf (patch)
treeb334e2b42560fac69469b352329b8527adab7ab6
parent48fcea26a90a0e3afea13a23fa4e5e9f6a910f0b (diff)
downloadgo-git-f5a9c7ed5ab58afa255c2f405b30f06f88a6aecf.tar.gz
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: ```
-rw-r--r--plumbing/transport/file/server_test.go49
1 files changed, 4 insertions, 45 deletions
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))
}