Fix run command race (#1470)

* Use exec.CommandContext to simplfy timeout handling

And fixing the data races which can be identified by the added tests when -race enabled.

* Use sleep commmand instead of reading from stdin

* Make the error handling go-esque

diff --git a/modules/process/manager.go b/modules/process/manager.go
index 8649304cf79bfcbb7bcdfc7d0aeaf61ed40ca291..9ac3af86f106e76d459ef724e51854f319ca8695 100644 (file)
--- a/modules/process/manager.go
+++ b/modules/process/manager.go
@@ -6,13 +6,12 @@ package process
 
 import (
        "bytes"
+       "context"
        "errors"
        "fmt"
        "os/exec"
        "sync"
        "time"
-
-       "code.gitea.io/gitea/modules/log"
 )
 
 // TODO: This packages still uses a singleton for the Manager.
@@ -101,7 +100,10 @@ func (pm *Manager) ExecDirEnv(timeout time.Duration, dir, desc string, env []str
        stdOut := new(bytes.Buffer)
        stdErr := new(bytes.Buffer)
 
-       cmd := exec.Command(cmdName, args...)
+       ctx, cancel := context.WithTimeout(context.Background(), timeout)
+       defer cancel()
+
+       cmd := exec.CommandContext(ctx, cmdName, args...)
        cmd.Dir = dir
        cmd.Env = env
        cmd.Stdout = stdOut
@@ -111,30 +113,14 @@ func (pm *Manager) ExecDirEnv(timeout time.Duration, dir, desc string, env []str
        }
 
        pid := pm.Add(desc, cmd)
-       done := make(chan error)
-       go func() {
-               done <- cmd.Wait()
-       }()
-
-       var err error
-       select {
-       case <-time.After(timeout):
-               if errKill := pm.Kill(pid); errKill != nil {
-                       log.Error(4, "exec(%d:%s) failed to kill: %v", pid, desc, errKill)
-               }
-               <-done
-               return "", "", ErrExecTimeout
-       case err = <-done:
-       }
-
+       err := cmd.Wait()
        pm.Remove(pid)
 
        if err != nil {
-               out := fmt.Errorf("exec(%d:%s) failed: %v stdout: %v stderr: %v", pid, desc, err, stdOut, stdErr)
-               return stdOut.String(), stdErr.String(), out
+               err = fmt.Errorf("exec(%d:%s) failed: %v(%v) stdout: %v stderr: %v", pid, desc, err, ctx.Err(), stdOut, stdErr)
        }
 
-       return stdOut.String(), stdErr.String(), nil
+       return stdOut.String(), stdErr.String(), err
 }
 
 // Kill and remove a process from list.

diff --git a/modules/process/manager_test.go b/modules/process/manager_test.go
index e638264ce1216bd1719a0d68339364c367ec3f6b..9980aba921724ab3dec95d37ca32a84b4a5db00d 100644 (file)
--- a/modules/process/manager_test.go
+++ b/modules/process/manager_test.go
@@ -3,6 +3,7 @@ package process
 import (
        "os/exec"
        "testing"
+       "time"
 
        "github.com/stretchr/testify/assert"
 )
@@ -31,3 +32,27 @@ func TestManager_Remove(t *testing.T) {
        _, exists := pm.Processes[pid2]
        assert.False(t, exists, "PID %d is in the list but shouldn't", pid2)
 }
+
+func TestExecTimeoutNever(t *testing.T) {
+
+       // TODO Investigate how to improve the time elapsed per round.
+       maxLoops := 10
+       for i := 1; i < maxLoops; i++ {
+               _, stderr, err := GetManager().ExecTimeout(5*time.Second, "ExecTimeout", "git", "--version")
+               if err != nil {
+                       t.Fatalf("git --version: %v(%s)", err, stderr)
+               }
+       }
+}
+
+func TestExecTimeoutAlways(t *testing.T) {
+
+       maxLoops := 100
+       for i := 1; i < maxLoops; i++ {
+               _, stderr, err := GetManager().ExecTimeout(100*time.Microsecond, "ExecTimeout", "sleep", "5")
+               // TODO Simplify logging and errors to get precise error type. E.g. checking "if err != context.DeadlineExceeded".
+               if err == nil {
+                       t.Fatalf("sleep 5 secs: %v(%s)", err, stderr)
+               }
+       }
+}