From 77625c56011dad5d8a931842277e6943c7b419d3 Mon Sep 17 00:00:00 2001 From: Mura Li Date: Sat, 8 Apr 2017 15:48:12 +0800 Subject: [PATCH 1/3] Use exec.CommandContext to simplfy timeout handling And fixing the data races which can be identified by the added tests when -race enabled. --- modules/process/manager.go | 33 ++++++++++----------------------- modules/process/manager_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 23 deletions(-) diff --git a/modules/process/manager.go b/modules/process/manager.go index 8649304cf79bf..3a3e7dd73aba6 100644 --- 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,15 @@ 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 + if err == nil { + return stdOut.String(), stdErr.String(), nil } - return stdOut.String(), stdErr.String(), nil + out := fmt.Errorf("exec(%d:%s) failed: %v stdout: %v stderr: %v", pid, desc, ctx.Err(), stdOut, stdErr) + return stdOut.String(), stdErr.String(), out } // Kill and remove a process from list. diff --git a/modules/process/manager_test.go b/modules/process/manager_test.go index e638264ce1216..81e6bace5d42f 100644 --- 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", "git", "hash-object", "--stdin") + // TODO Simplify logging and errors to get precise error type. E.g. checking "if err != context.DeadlineExceeded". + if err == nil { + t.Fatalf("git hash-object --stdin: %v(%s)", err, stderr) + } + } +} From cb66d5b2c35c0b8f189b9edde02866209b2a7db2 Mon Sep 17 00:00:00 2001 From: Mura Li Date: Sat, 8 Apr 2017 16:56:25 +0800 Subject: [PATCH 2/3] Use sleep commmand instead of reading from stdin --- modules/process/manager_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/process/manager_test.go b/modules/process/manager_test.go index 81e6bace5d42f..9980aba921724 100644 --- a/modules/process/manager_test.go +++ b/modules/process/manager_test.go @@ -49,10 +49,10 @@ func TestExecTimeoutAlways(t *testing.T) { maxLoops := 100 for i := 1; i < maxLoops; i++ { - _, stderr, err := GetManager().ExecTimeout(100*time.Microsecond, "ExecTimeout", "git", "hash-object", "--stdin") + _, 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("git hash-object --stdin: %v(%s)", err, stderr) + t.Fatalf("sleep 5 secs: %v(%s)", err, stderr) } } } From 7dbf2d2120cb3c74bee9a3db37ce87f74a95670e Mon Sep 17 00:00:00 2001 From: Mura Li Date: Mon, 13 Nov 2017 13:29:26 +0800 Subject: [PATCH 3/3] Make the error handling go-esque --- modules/process/manager.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/modules/process/manager.go b/modules/process/manager.go index 3a3e7dd73aba6..9ac3af86f106e 100644 --- a/modules/process/manager.go +++ b/modules/process/manager.go @@ -116,12 +116,11 @@ func (pm *Manager) ExecDirEnv(timeout time.Duration, dir, desc string, env []str err := cmd.Wait() pm.Remove(pid) - if err == nil { - return stdOut.String(), stdErr.String(), nil + if err != nil { + err = fmt.Errorf("exec(%d:%s) failed: %v(%v) stdout: %v stderr: %v", pid, desc, err, ctx.Err(), stdOut, stdErr) } - out := fmt.Errorf("exec(%d:%s) failed: %v stdout: %v stderr: %v", pid, desc, ctx.Err(), stdOut, stdErr) - return stdOut.String(), stdErr.String(), out + return stdOut.String(), stdErr.String(), err } // Kill and remove a process from list.