From b877504b03a43dc5ed19109f1a00f08a343ebaba Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 31 Mar 2022 19:56:22 +0800 Subject: [PATCH] Refactor `git.Command.Run*`, introduce `RunWithContextString` and `RunWithContextBytes` (#19266) This follows * https://github.com/go-gitea/gitea/issues/18553 Introduce `RunWithContextString` and `RunWithContextBytes` to help the refactoring. Add related unit tests. They keep the same behavior to save stderr into err.Error() as `RunInXxx` before. Remove `RunInDirTimeoutPipeline` `RunInDirTimeoutFullPipeline` `RunInDirTimeout` `RunInDirTimeoutEnv` `RunInDirPipeline` `RunInDirFullPipeline` `RunTimeout`, `RunInDirTimeoutEnvPipeline`, `RunInDirTimeoutEnvFullPipeline`, `RunInDirTimeoutEnvFullPipelineFunc`. Then remaining `RunInDir` `RunInDirBytes` `RunInDirWithEnv` can be easily refactored in next PR with a simple search & replace: * before: `stdout, err := RunInDir(path)` * next: `stdout, _, err := RunWithContextString(&git.RunContext{Dir:path})` Other changes: 1. When `timeout <= 0`, use default. Because `timeout==0` is meaningless and could cause bugs. And now many functions becomes more simple, eg: `GitGcRepos` 9 lines to 1 line. `Fsck` 6 lines to 1 line. 2. Only set defaultCommandExecutionTimeout when the option `setting.Git.Timeout.Default > 0` --- modules/git/command.go | 138 +++++++++++++------------------ modules/git/command_race_test.go | 40 +++++++++ modules/git/command_test.go | 41 ++++----- modules/git/git.go | 11 +-- modules/process/manager.go | 6 +- routers/web/repo/http.go | 2 +- services/repository/check.go | 10 +-- services/repository/fork.go | 4 +- 8 files changed, 126 insertions(+), 126 deletions(-) create mode 100644 modules/git/command_race_test.go diff --git a/modules/git/command.go b/modules/git/command.go index 5d2e1dd67..764114a3b 100644 --- a/modules/git/command.go +++ b/modules/git/command.go @@ -14,6 +14,7 @@ import ( "os/exec" "strings" "time" + "unsafe" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/process" @@ -93,32 +94,6 @@ func (c *Command) AddArguments(args ...string) *Command { return c } -// RunInDirTimeoutEnvPipeline executes the command in given directory with given timeout, -// it pipes stdout and stderr to given io.Writer. -func (c *Command) RunInDirTimeoutEnvPipeline(env []string, timeout time.Duration, dir string, stdout, stderr io.Writer) error { - return c.RunInDirTimeoutEnvFullPipeline(env, timeout, dir, stdout, stderr, nil) -} - -// RunInDirTimeoutEnvFullPipeline executes the command in given directory with given timeout, -// it pipes stdout and stderr to given io.Writer and passes in an io.Reader as stdin. -func (c *Command) RunInDirTimeoutEnvFullPipeline(env []string, timeout time.Duration, dir string, stdout, stderr io.Writer, stdin io.Reader) error { - return c.RunInDirTimeoutEnvFullPipelineFunc(env, timeout, dir, stdout, stderr, stdin, nil) -} - -// RunInDirTimeoutEnvFullPipelineFunc executes the command in given directory with given timeout, -// it pipes stdout and stderr to given io.Writer and passes in an io.Reader as stdin. Between cmd.Start and cmd.Wait the passed in function is run. -func (c *Command) RunInDirTimeoutEnvFullPipelineFunc(env []string, timeout time.Duration, dir string, stdout, stderr io.Writer, stdin io.Reader, fn func(context.Context, context.CancelFunc) error) error { - return c.RunWithContext(&RunContext{ - Env: env, - Timeout: timeout, - Dir: dir, - Stdout: stdout, - Stderr: stderr, - Stdin: stdin, - PipelineFunc: fn, - }) -} - // RunContext represents parameters to run the command type RunContext struct { Env []string @@ -131,7 +106,7 @@ type RunContext struct { // RunWithContext run the command with context func (c *Command) RunWithContext(rc *RunContext) error { - if rc.Timeout == -1 { + if rc.Timeout <= 0 { rc.Timeout = defaultCommandExecutionTimeout } @@ -203,58 +178,73 @@ func (c *Command) RunWithContext(rc *RunContext) error { return ctx.Err() } -// RunInDirTimeoutPipeline executes the command in given directory with given timeout, -// it pipes stdout and stderr to given io.Writer. -func (c *Command) RunInDirTimeoutPipeline(timeout time.Duration, dir string, stdout, stderr io.Writer) error { - return c.RunInDirTimeoutEnvPipeline(nil, timeout, dir, stdout, stderr) +type RunError interface { + error + Stderr() string } -// RunInDirTimeoutFullPipeline executes the command in given directory with given timeout, -// it pipes stdout and stderr to given io.Writer, and stdin from the given io.Reader -func (c *Command) RunInDirTimeoutFullPipeline(timeout time.Duration, dir string, stdout, stderr io.Writer, stdin io.Reader) error { - return c.RunInDirTimeoutEnvFullPipeline(nil, timeout, dir, stdout, stderr, stdin) +type runError struct { + err error + stderr string + errMsg string } -// RunInDirTimeout executes the command in given directory with given timeout, -// and returns stdout in []byte and error (combined with stderr). -func (c *Command) RunInDirTimeout(timeout time.Duration, dir string) ([]byte, error) { - return c.RunInDirTimeoutEnv(nil, timeout, dir) +func (r *runError) Error() string { + // the stderr must be in the returned error text, some code only checks `strings.Contains(err.Error(), "git error")` + if r.errMsg == "" { + r.errMsg = ConcatenateError(r.err, r.stderr).Error() + } + return r.errMsg } -// RunInDirTimeoutEnv executes the command in given directory with given timeout, -// and returns stdout in []byte and error (combined with stderr). -func (c *Command) RunInDirTimeoutEnv(env []string, timeout time.Duration, dir string) ([]byte, error) { - stdout := new(bytes.Buffer) - stderr := new(bytes.Buffer) - if err := c.RunInDirTimeoutEnvPipeline(env, timeout, dir, stdout, stderr); err != nil { - return nil, ConcatenateError(err, stderr.String()) - } - if stdout.Len() > 0 && log.IsTrace() { - tracelen := stdout.Len() - if tracelen > 1024 { - tracelen = 1024 - } - log.Trace("Stdout:\n %s", stdout.Bytes()[:tracelen]) - } - return stdout.Bytes(), nil +func (r *runError) Unwrap() error { + return r.err +} + +func (r *runError) Stderr() string { + return r.stderr } -// RunInDirPipeline executes the command in given directory, -// it pipes stdout and stderr to given io.Writer. -func (c *Command) RunInDirPipeline(dir string, stdout, stderr io.Writer) error { - return c.RunInDirFullPipeline(dir, stdout, stderr, nil) +func bytesToString(b []byte) string { + return *(*string)(unsafe.Pointer(&b)) // that's what Golang's strings.Builder.String() does (go/src/strings/builder.go) } -// RunInDirFullPipeline executes the command in given directory, -// it pipes stdout and stderr to given io.Writer. -func (c *Command) RunInDirFullPipeline(dir string, stdout, stderr io.Writer, stdin io.Reader) error { - return c.RunInDirTimeoutFullPipeline(-1, dir, stdout, stderr, stdin) +// RunWithContextString run the command with context and returns stdout/stderr as string. and store stderr to returned error (err combined with stderr). +func (c *Command) RunWithContextString(rc *RunContext) (stdout, stderr string, runErr RunError) { + stdoutBytes, stderrBytes, err := c.RunWithContextBytes(rc) + stdout = bytesToString(stdoutBytes) + stderr = bytesToString(stderrBytes) + if err != nil { + return stdout, stderr, &runError{err: err, stderr: stderr} + } + // even if there is no err, there could still be some stderr output, so we just return stdout/stderr as they are + return stdout, stderr, nil +} + +// RunWithContextBytes run the command with context and returns stdout/stderr as bytes. and store stderr to returned error (err combined with stderr). +func (c *Command) RunWithContextBytes(rc *RunContext) (stdout, stderr []byte, runErr RunError) { + if rc.Stdout != nil || rc.Stderr != nil { + // we must panic here, otherwise there would be bugs if developers set Stdin/Stderr by mistake, and it would be very difficult to debug + panic("stdout and stderr field must be nil when using RunWithContextBytes") + } + stdoutBuf := &bytes.Buffer{} + stderrBuf := &bytes.Buffer{} + rc.Stdout = stdoutBuf + rc.Stderr = stderrBuf + err := c.RunWithContext(rc) + stderr = stderrBuf.Bytes() + if err != nil { + return nil, stderr, &runError{err: err, stderr: string(stderr)} + } + // even if there is no err, there could still be some stderr output + return stdoutBuf.Bytes(), stderr, nil } // RunInDirBytes executes the command in given directory // and returns stdout in []byte and error (combined with stderr). func (c *Command) RunInDirBytes(dir string) ([]byte, error) { - return c.RunInDirTimeout(-1, dir) + stdout, _, err := c.RunWithContextBytes(&RunContext{Dir: dir}) + return stdout, err } // RunInDir executes the command in given directory @@ -266,27 +256,15 @@ func (c *Command) RunInDir(dir string) (string, error) { // RunInDirWithEnv executes the command in given directory // and returns stdout in string and error (combined with stderr). func (c *Command) RunInDirWithEnv(dir string, env []string) (string, error) { - stdout, err := c.RunInDirTimeoutEnv(env, -1, dir) - if err != nil { - return "", err - } - return string(stdout), nil -} - -// RunTimeout executes the command in default working directory with given timeout, -// and returns stdout in string and error (combined with stderr). -func (c *Command) RunTimeout(timeout time.Duration) (string, error) { - stdout, err := c.RunInDirTimeout(timeout, "") - if err != nil { - return "", err - } - return string(stdout), nil + stdout, _, err := c.RunWithContextString(&RunContext{Env: env, Dir: dir}) + return stdout, err } // Run executes the command in default working directory // and returns stdout in string and error (combined with stderr). func (c *Command) Run() (string, error) { - return c.RunTimeout(-1) + stdout, _, err := c.RunWithContextString(&RunContext{}) + return stdout, err } // AllowLFSFiltersArgs return globalCommandArgs with lfs filter, it should only be used for tests diff --git a/modules/git/command_race_test.go b/modules/git/command_race_test.go new file mode 100644 index 000000000..2c975860d --- /dev/null +++ b/modules/git/command_race_test.go @@ -0,0 +1,40 @@ +// Copyright 2017 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +//go:build race +// +build race + +package git + +import ( + "context" + "testing" + "time" +) + +func TestRunWithContextNoTimeout(t *testing.T) { + maxLoops := 10 + + // 'git --version' does not block so it must be finished before the timeout triggered. + cmd := NewCommand(context.Background(), "--version") + for i := 0; i < maxLoops; i++ { + if err := cmd.RunWithContext(&RunContext{}); err != nil { + t.Fatal(err) + } + } +} + +func TestRunWithContextTimeout(t *testing.T) { + maxLoops := 10 + + // 'git hash-object --stdin' blocks on stdin so we can have the timeout triggered. + cmd := NewCommand(context.Background(), "hash-object", "--stdin") + for i := 0; i < maxLoops; i++ { + if err := cmd.RunWithContext(&RunContext{Timeout: 1 * time.Millisecond}); err != nil { + if err != context.DeadlineExceeded { + t.Fatalf("Testing %d/%d: %v", i, maxLoops, err) + } + } + } +} diff --git a/modules/git/command_test.go b/modules/git/command_test.go index f92f526d2..33a6661d4 100644 --- a/modules/git/command_test.go +++ b/modules/git/command_test.go @@ -1,40 +1,29 @@ -// Copyright 2017 The Gitea Authors. All rights reserved. +// Copyright 2022 The Gitea Authors. All rights reserved. // Use of this source code is governed by a MIT-style // license that can be found in the LICENSE file. -//go:build race -// +build race - package git import ( "context" "testing" - "time" -) -func TestRunInDirTimeoutPipelineNoTimeout(t *testing.T) { - maxLoops := 1000 + "github.com/stretchr/testify/assert" +) - // 'git --version' does not block so it must be finished before the timeout triggered. +func TestRunWithContextStd(t *testing.T) { cmd := NewCommand(context.Background(), "--version") - for i := 0; i < maxLoops; i++ { - if err := cmd.RunInDirTimeoutPipeline(-1, "", nil, nil); err != nil { - t.Fatal(err) - } - } -} - -func TestRunInDirTimeoutPipelineAlwaysTimeout(t *testing.T) { - maxLoops := 1000 + stdout, stderr, err := cmd.RunWithContextString(&RunContext{}) + assert.NoError(t, err) + assert.Empty(t, stderr) + assert.Contains(t, stdout, "git version") - // 'git hash-object --stdin' blocks on stdin so we can have the timeout triggered. - cmd := NewCommand(context.Background(), "hash-object", "--stdin") - for i := 0; i < maxLoops; i++ { - if err := cmd.RunInDirTimeoutPipeline(1*time.Microsecond, "", nil, nil); err != nil { - if err != context.DeadlineExceeded { - t.Fatalf("Testing %d/%d: %v", i, maxLoops, err) - } - } + cmd = NewCommand(context.Background(), "--no-such-arg") + stdout, stderr, err = cmd.RunWithContextString(&RunContext{}) + if assert.Error(t, err) { + assert.Equal(t, stderr, err.Stderr()) + assert.Contains(t, err.Stderr(), "unknown option:") + assert.Contains(t, err.Error(), "exit status 129 - unknown option:") + assert.Empty(t, stdout) } } diff --git a/modules/git/git.go b/modules/git/git.go index 14940d1f1..6b7dbfd16 100644 --- a/modules/git/git.go +++ b/modules/git/git.go @@ -124,7 +124,9 @@ func VersionInfo() string { func Init(ctx context.Context) error { DefaultContext = ctx - defaultCommandExecutionTimeout = time.Duration(setting.Git.Timeout.Default) * time.Second + if setting.Git.Timeout.Default > 0 { + defaultCommandExecutionTimeout = time.Duration(setting.Git.Timeout.Default) * time.Second + } if err := SetExecutablePath(setting.Git.Path); err != nil { return err @@ -295,10 +297,5 @@ func checkAndRemoveConfig(key, value string) error { // Fsck verifies the connectivity and validity of the objects in the database func Fsck(ctx context.Context, repoPath string, timeout time.Duration, args ...string) error { - // Make sure timeout makes sense. - if timeout <= 0 { - timeout = -1 - } - _, err := NewCommand(ctx, "fsck").AddArguments(args...).RunInDirTimeout(timeout, repoPath) - return err + return NewCommand(ctx, "fsck").AddArguments(args...).RunWithContext(&RunContext{Timeout: timeout, Dir: repoPath}) } diff --git a/modules/process/manager.go b/modules/process/manager.go index 50dbbbe6c..26dd6d535 100644 --- a/modules/process/manager.go +++ b/modules/process/manager.go @@ -86,6 +86,10 @@ func (pm *Manager) AddContext(parent context.Context, description string) (ctx c // Most processes will not need to use the cancel function but there will be cases whereby you want to cancel the process but not immediately remove it from the // process table. func (pm *Manager) AddContextTimeout(parent context.Context, timeout time.Duration, description string) (ctx context.Context, cancel context.CancelFunc, finshed FinishedFunc) { + if timeout <= 0 { + // it's meaningless to use timeout <= 0, and it must be a bug! so we must panic here to tell developers to make the timeout correct + panic("the timeout must be greater than zero, otherwise the context will be cancelled immediately") + } ctx, cancel = context.WithTimeout(parent, timeout) ctx, pid, finshed := pm.Add(ctx, description, cancel) @@ -239,7 +243,7 @@ func (pm *Manager) ExecDirEnv(ctx context.Context, timeout time.Duration, dir, d // Returns its complete stdout and stderr // outputs and an error, if any (including timeout) func (pm *Manager) ExecDirEnvStdIn(ctx context.Context, timeout time.Duration, dir, desc string, env []string, stdIn io.Reader, cmdName string, args ...string) (string, string, error) { - if timeout == -1 { + if timeout <= 0 { timeout = 60 * time.Second } diff --git a/routers/web/repo/http.go b/routers/web/repo/http.go index d149d4f89..82ac95c1b 100644 --- a/routers/web/repo/http.go +++ b/routers/web/repo/http.go @@ -542,7 +542,7 @@ func GetInfoRefs(ctx *context.Context) { } h.environ = append(os.Environ(), h.environ...) - refs, err := git.NewCommand(ctx, service, "--stateless-rpc", "--advertise-refs", ".").RunInDirTimeoutEnv(h.environ, -1, h.dir) + refs, _, err := git.NewCommand(ctx, service, "--stateless-rpc", "--advertise-refs", ".").RunWithContextBytes(&git.RunContext{Env: h.environ, Dir: h.dir}) if err != nil { log.Error(fmt.Sprintf("%v - %s", err, string(refs))) } diff --git a/services/repository/check.go b/services/repository/check.go index 6962090f8..efce308f5 100644 --- a/services/repository/check.go +++ b/services/repository/check.go @@ -77,15 +77,7 @@ func GitGcRepos(ctx context.Context, timeout time.Duration, args ...string) erro SetDescription(fmt.Sprintf("Repository Garbage Collection: %s", repo.FullName())) var stdout string var err error - if timeout > 0 { - var stdoutBytes []byte - stdoutBytes, err = command.RunInDirTimeout( - timeout, - repo.RepoPath()) - stdout = string(stdoutBytes) - } else { - stdout, err = command.RunInDir(repo.RepoPath()) - } + stdout, _, err = command.RunWithContextString(&git.RunContext{Timeout: timeout, Dir: repo.RepoPath()}) if err != nil { log.Error("Repository garbage collection failed for %v. Stdout: %s\nError: %v", repo, stdout, err) diff --git a/services/repository/fork.go b/services/repository/fork.go index 1a5d35840..220fbeb99 100644 --- a/services/repository/fork.go +++ b/services/repository/fork.go @@ -108,10 +108,10 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork needsRollback = true repoPath := repo_model.RepoPath(owner.Name, repo.Name) - if stdout, err := git.NewCommand(txCtx, + if stdout, _, err := git.NewCommand(txCtx, "clone", "--bare", oldRepoPath, repoPath). SetDescription(fmt.Sprintf("ForkRepository(git clone): %s to %s", opts.BaseRepo.FullName(), repo.FullName())). - RunInDirTimeout(10*time.Minute, ""); err != nil { + RunWithContextBytes(&git.RunContext{Timeout: 10 * time.Minute}); err != nil { log.Error("Fork Repository (git clone) Failed for %v (from %v):\nStdout: %s\nError: %v", repo, opts.BaseRepo, stdout, err) return fmt.Errorf("git clone: %v", err) }