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
This commit is contained in:
parent
e9728bf3b4
commit
f4d12f8d97
|
@ -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.
|
||||
|
|
|
@ -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)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
Loading…
Reference in New Issue