From 0cfce4177571542fa448601a353fe5bbec11aac2 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Fri, 29 Nov 2019 11:21:36 -0600 Subject: [PATCH 01/11] Graceful: Say when Gitea is completely finished --- cmd/web.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cmd/web.go b/cmd/web.go index 3c346ef87a07..e0e47a181f5a 100644 --- a/cmd/web.go +++ b/cmd/web.go @@ -197,6 +197,7 @@ func runWeb(ctx *cli.Context) error { log.Info("HTTP Listener: %s Closed", listenAddr) graceful.Manager.WaitForServers() graceful.Manager.WaitForTerminate() + log.Info("PID: %d Gitea Web Finished", os.Getpid()) log.Close() return nil } From 65f608e72bf21800874be0048afe0a46b4db1a09 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 17 Nov 2019 20:08:29 +0000 Subject: [PATCH 02/11] Graceful: Create callbacks to with contexts --- modules/graceful/context.go | 90 +++++++++++++++++++++++++ modules/graceful/manager.go | 100 +++++++++++++++++++++++++++- modules/graceful/manager_unix.go | 68 ++++++++++--------- modules/graceful/manager_windows.go | 35 ++++++---- 4 files changed, 247 insertions(+), 46 deletions(-) create mode 100644 modules/graceful/context.go diff --git a/modules/graceful/context.go b/modules/graceful/context.go new file mode 100644 index 000000000000..a4a4df7dea9c --- /dev/null +++ b/modules/graceful/context.go @@ -0,0 +1,90 @@ +// Copyright 2019 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. + +package graceful + +import ( + "context" + "fmt" + "time" +) + +// Errors for context.Err() +var ( + ErrShutdown = fmt.Errorf("Graceful Manager called Shutdown") + ErrHammer = fmt.Errorf("Graceful Manager called Hammer") + ErrTerminate = fmt.Errorf("Graceful Manager called Terminate") +) + +// ChannelContext is a context that wraps a channel and error as a context +type ChannelContext struct { + done <-chan struct{} + err error +} + +// NewChannelContext creates a ChannelContext from a channel and error +func NewChannelContext(done <-chan struct{}, err error) *ChannelContext { + return &ChannelContext{ + done: done, + err: err, + } +} + +// Deadline returns the time when work done on behalf of this context +// should be canceled. There is no Deadline for a ChannelContext +func (ctx *ChannelContext) Deadline() (deadline time.Time, ok bool) { + return +} + +// Done returns the channel provided at the creation of this context. +// When closed, work done on behalf of this context should be canceled. +func (ctx *ChannelContext) Done() <-chan struct{} { + return ctx.done +} + +// Err returns nil, if Done is not closed. If Done is closed, +// Err returns the error provided at the creation of this context +func (ctx *ChannelContext) Err() error { + select { + case <-ctx.done: + return ctx.err + default: + return nil + } +} + +// Value returns nil for all calls as no values are or can be associated with this context +func (ctx *ChannelContext) Value(key interface{}) interface{} { + return nil +} + +// ShutdownContext returns a context.Context that is Done at shutdown +// Callers using this context should ensure that they are registered as a running server +// in order that they are waited for. +func (g *gracefulManager) ShutdownContext() context.Context { + return &ChannelContext{ + done: g.IsShutdown(), + err: ErrShutdown, + } +} + +// HammerContext returns a context.Context that is Done at hammer +// Callers using this context should ensure that they are registered as a running server +// in order that they are waited for. +func (g *gracefulManager) HammerContext() context.Context { + return &ChannelContext{ + done: g.IsHammer(), + err: ErrHammer, + } +} + +// TerminateContext returns a context.Context that is Done at terminate +// Callers using this context should ensure that they are registered as a terminating server +// in order that they are waited for. +func (g *gracefulManager) TerminateContext() context.Context { + return &ChannelContext{ + done: g.IsTerminate(), + err: ErrTerminate, + } +} diff --git a/modules/graceful/manager.go b/modules/graceful/manager.go index 48f76635ff94..b5b5293fdd6e 100644 --- a/modules/graceful/manager.go +++ b/modules/graceful/manager.go @@ -5,6 +5,7 @@ package graceful import ( + "context" "time" "code.gitea.io/gitea/modules/log" @@ -34,9 +35,106 @@ const numberOfServersToCreate = 3 var Manager *gracefulManager func init() { - Manager = newGracefulManager() + Manager = newGracefulManager(context.Background()) } +// CallbackWithContext is combined runnable and context to watch to see if the caller has finished +type CallbackWithContext func(ctx context.Context, callback func()) + +// RunnableWithShutdownFns is a runnable with functions to run at shutdown and terminate +// After the callback to atShutdown is called and is complete, the main function must return. +// Similarly the callback function provided to atTerminate must return once termination is complete. +// Please note that use of the atShutdown and atTerminate callbacks will create go-routines that will wait till till their respective signals +// - users must therefore be careful to only call these as necessary. +// If run is not expected to run indefinitely RunWithShutdownChan is likely to be more appropriate. +type RunnableWithShutdownFns func(atShutdown, atTerminate func(context.Context, func())) + +// RunWithShutdownFns takes a function that has both atShutdown and atTerminate callbacks +// After the callback to atShutdown is called and is complete, the main function must return. +// Similarly the callback function provided to atTerminate must return once termination is complete. +// Please note that use of the atShutdown and atTerminate callbacks will create go-routines that will wait till till their respective signals +// - users must therefore be careful to only call these as necessary. +// If run is not expected to run indefinitely RunWithShutdownChan is likely to be more appropriate. +func (g *gracefulManager) RunWithShutdownFns(run RunnableWithShutdownFns) { + g.runningServerWaitGroup.Add(1) + defer g.runningServerWaitGroup.Done() + run(func(ctx context.Context, atShutdown func()) { + go func() { + select { + case <-g.IsShutdown(): + atShutdown() + case <-ctx.Done(): + return + } + }() + }, func(ctx context.Context, atTerminate func()) { + g.RunAtTerminate(ctx, atTerminate) + }) +} + +// RunnableWithShutdownChan is a runnable with functions to run at shutdown and terminate. +// After the atShutdown channel is closed, the main function must return once shutdown is complete. +// (Optionally IsHammer may be waited for instead however, this should be avoided if possible.) +// The callback function provided to atTerminate must return once termination is complete. +// Please note that use of the atTerminate function will create a go-routine that will wait till terminate - users must therefore be careful to only call this as necessary. +type RunnableWithShutdownChan func(atShutdown <-chan struct{}, atTerminate CallbackWithContext) + +// RunWithShutdownChan takes a function that has channel to watch for shutdown and atTerminate callbacks +// After the atShutdown channel is closed, the main function must return once shutdown is complete. +// (Optionally IsHammer may be waited for instead however, this should be avoided if possible.) +// The callback function provided to atTerminate must return once termination is complete. +// Please note that use of the atTerminate function will create a go-routine that will wait till terminate - users must therefore be careful to only call this as necessary. +func (g *gracefulManager) RunWithShutdownChan(run RunnableWithShutdownChan) { + g.runningServerWaitGroup.Add(1) + defer g.runningServerWaitGroup.Done() + run(g.IsShutdown(), func(ctx context.Context, atTerminate func()) { + g.RunAtTerminate(ctx, atTerminate) + }) +} + +// RunWithShutdownContext takes a function that has a context to watch for shutdown. +// After the provided context is Done(), the main function must return once shutdown is complete. +// (Optionally the HammerContext may be obtained and waited for however, this should be avoided if possible.) +func (g *gracefulManager) RunWithShutdownContext(run func(context.Context)) { + g.runningServerWaitGroup.Add(1) + defer g.runningServerWaitGroup.Done() + run(g.ShutdownContext()) +} + +// RunAtTerminate adds to the terminate wait group and creates a go-routine to run the provided function at termination +func (g *gracefulManager) RunAtTerminate(ctx context.Context, terminate func()) { + g.terminateWaitGroup.Add(1) + go func() { + select { + case <-g.IsTerminate(): + terminate() + case <-ctx.Done(): + } + g.terminateWaitGroup.Done() + }() +} + +// RunAtShutdown creates a go-routine to run the provided function at shutdown +func (g *gracefulManager) RunAtShutdown(ctx context.Context, shutdown func()) { + go func() { + select { + case <-g.IsShutdown(): + shutdown() + case <-ctx.Done(): + } + }() +} + +// RunAtHammer creates a go-routine to run the provided function at shutdown +func (g *gracefulManager) RunAtHammer(ctx context.Context, hammer func()) { + go func() { + select { + case <-g.IsHammer(): + hammer() + case <-ctx.Done(): + } + }() +} func (g *gracefulManager) doShutdown() { if !g.setStateTransition(stateRunning, stateShuttingDown) { return diff --git a/modules/graceful/manager_unix.go b/modules/graceful/manager_unix.go index 15b0ff4448e3..1ffc59f0df70 100644 --- a/modules/graceful/manager_unix.go +++ b/modules/graceful/manager_unix.go @@ -7,6 +7,7 @@ package graceful import ( + "context" "errors" "os" "os/signal" @@ -31,19 +32,19 @@ type gracefulManager struct { terminateWaitGroup sync.WaitGroup } -func newGracefulManager() *gracefulManager { +func newGracefulManager(ctx context.Context) *gracefulManager { manager := &gracefulManager{ isChild: len(os.Getenv(listenFDs)) > 0 && os.Getppid() > 1, lock: &sync.RWMutex{}, } manager.createServerWaitGroup.Add(numberOfServersToCreate) - manager.Run() + manager.Run(ctx) return manager } -func (g *gracefulManager) Run() { +func (g *gracefulManager) Run(ctx context.Context) { g.setState(stateRunning) - go g.handleSignals() + go g.handleSignals(ctx) c := make(chan struct{}) go func() { defer close(c) @@ -69,9 +70,7 @@ func (g *gracefulManager) Run() { } } -func (g *gracefulManager) handleSignals() { - var sig os.Signal - +func (g *gracefulManager) handleSignals(ctx context.Context) { signalChannel := make(chan os.Signal, 1) signal.Notify( @@ -86,35 +85,40 @@ func (g *gracefulManager) handleSignals() { pid := syscall.Getpid() for { - sig = <-signalChannel - switch sig { - case syscall.SIGHUP: - if setting.GracefulRestartable { - log.Info("PID: %d. Received SIGHUP. Forking...", pid) - err := g.doFork() - if err != nil && err.Error() != "another process already forked. Ignoring this one" { - log.Error("Error whilst forking from PID: %d : %v", pid, err) + select { + case sig := <-signalChannel: + switch sig { + case syscall.SIGHUP: + if setting.GracefulRestartable { + log.Info("PID: %d. Received SIGHUP. Forking...", pid) + err := g.doFork() + if err != nil && err.Error() != "another process already forked. Ignoring this one" { + log.Error("Error whilst forking from PID: %d : %v", pid, err) + } + } else { + log.Info("PID: %d. Received SIGHUP. Not set restartable. Shutting down...", pid) + + g.doShutdown() } - } else { - log.Info("PID: %d. Received SIGHUP. Not set restartable. Shutting down...", pid) - + case syscall.SIGUSR1: + log.Info("PID %d. Received SIGUSR1.", pid) + case syscall.SIGUSR2: + log.Warn("PID %d. Received SIGUSR2. Hammering...", pid) + g.doHammerTime(0 * time.Second) + case syscall.SIGINT: + log.Warn("PID %d. Received SIGINT. Shutting down...", pid) g.doShutdown() + case syscall.SIGTERM: + log.Warn("PID %d. Received SIGTERM. Shutting down...", pid) + g.doShutdown() + case syscall.SIGTSTP: + log.Info("PID %d. Received SIGTSTP.", pid) + default: + log.Info("PID %d. Received %v.", pid, sig) } - case syscall.SIGUSR1: - log.Info("PID %d. Received SIGUSR1.", pid) - case syscall.SIGUSR2: - log.Warn("PID %d. Received SIGUSR2. Hammering...", pid) - g.doHammerTime(0 * time.Second) - case syscall.SIGINT: - log.Warn("PID %d. Received SIGINT. Shutting down...", pid) - g.doShutdown() - case syscall.SIGTERM: - log.Warn("PID %d. Received SIGTERM. Shutting down...", pid) + case <-ctx.Done(): + log.Warn("PID: %d. Background context for manager closed - %v - Shutting down...", pid, ctx.Err()) g.doShutdown() - case syscall.SIGTSTP: - log.Info("PID %d. Received SIGTSTP.", pid) - default: - log.Info("PID %d. Received %v.", pid, sig) } } } diff --git a/modules/graceful/manager_windows.go b/modules/graceful/manager_windows.go index 925b1fc56000..dd48a8d74c31 100644 --- a/modules/graceful/manager_windows.go +++ b/modules/graceful/manager_windows.go @@ -8,6 +8,7 @@ package graceful import ( + "context" "os" "strconv" "sync" @@ -29,6 +30,7 @@ const ( ) type gracefulManager struct { + ctx context.Context isChild bool lock *sync.RWMutex state state @@ -40,10 +42,11 @@ type gracefulManager struct { terminateWaitGroup sync.WaitGroup } -func newGracefulManager() *gracefulManager { +func newGracefulManager(ctx context.Context) *gracefulManager { manager := &gracefulManager{ isChild: false, lock: &sync.RWMutex{}, + ctx: ctx, } manager.createServerWaitGroup.Add(numberOfServersToCreate) manager.Run() @@ -89,23 +92,29 @@ func (g *gracefulManager) Execute(args []string, changes <-chan svc.ChangeReques waitTime := 30 * time.Second loop: - for change := range changes { - switch change.Cmd { - case svc.Interrogate: - status <- change.CurrentStatus - case svc.Stop, svc.Shutdown: + for { + select { + case <-g.ctx.Done(): g.doShutdown() waitTime += setting.GracefulHammerTime break loop - case hammerCode: - g.doShutdown() - g.doHammerTime(0 * time.Second) - break loop - default: - log.Debug("Unexpected control request: %v", change.Cmd) + case change := <-changes: + switch change.Cmd { + case svc.Interrogate: + status <- change.CurrentStatus + case svc.Stop, svc.Shutdown: + g.doShutdown() + waitTime += setting.GracefulHammerTime + break loop + case hammerCode: + g.doShutdown() + g.doHammerTime(0 * time.Second) + break loop + default: + log.Debug("Unexpected control request: %v", change.Cmd) + } } } - status <- svc.Status{ State: svc.StopPending, WaitHint: uint32(waitTime / time.Millisecond), From f2013d40e9181bc2564de92bdbe67fc5c82df348 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 17 Nov 2019 21:58:47 +0000 Subject: [PATCH 03/11] Graceful: Git and Process within HammerTime Force all git commands to terminate at HammerTime Force all process commands to terminate at HammerTime Move almost all git processes to run as git Commands --- models/pull.go | 31 ++++++------- models/repo.go | 92 ++++++++++++++++++------------------- models/repo_generate.go | 14 ++---- modules/git/blame.go | 4 +- modules/git/command.go | 32 ++++++++++--- modules/git/git.go | 4 ++ modules/graceful/manager.go | 6 +++ modules/process/manager.go | 5 +- routers/repo/http.go | 8 +++- services/gitdiff/gitdiff.go | 21 +++++---- services/mirror/mirror.go | 56 +++++++++++++++------- services/release/release.go | 11 ++--- 12 files changed, 172 insertions(+), 112 deletions(-) diff --git a/models/pull.go b/models/pull.go index 174faee97ab0..5b18c9ed10c1 100644 --- a/models/pull.go +++ b/models/pull.go @@ -18,7 +18,6 @@ import ( "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" - "code.gitea.io/gitea/modules/process" "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/sync" @@ -536,16 +535,13 @@ func (pr *PullRequest) getMergeCommit() (*git.Commit, error) { headFile := pr.GetGitRefName() // Check if a pull request is merged into BaseBranch - _, stderr, err := process.GetManager().ExecDirEnv(-1, "", fmt.Sprintf("isMerged (git merge-base --is-ancestor): %d", pr.BaseRepo.ID), - []string{"GIT_INDEX_FILE=" + indexTmpPath, "GIT_DIR=" + pr.BaseRepo.RepoPath()}, - git.GitExecutable, "merge-base", "--is-ancestor", headFile, pr.BaseBranch) - + _, err := git.NewCommand("merge-base", "--is-ancestor", headFile, pr.BaseBranch).RunInDirWithEnv(pr.BaseRepo.RepoPath(), []string{"GIT_INDEX_FILE=" + indexTmpPath, "GIT_DIR=" + pr.BaseRepo.RepoPath()}) if err != nil { // Errors are signaled by a non-zero status that is not 1 if strings.Contains(err.Error(), "exit status 1") { return nil, nil } - return nil, fmt.Errorf("git merge-base --is-ancestor: %v %v", stderr, err) + return nil, fmt.Errorf("git merge-base --is-ancestor: %v", err) } commitIDBytes, err := ioutil.ReadFile(pr.BaseRepo.RepoPath() + "/" + headFile) @@ -559,11 +555,9 @@ func (pr *PullRequest) getMergeCommit() (*git.Commit, error) { cmd := commitID[:40] + ".." + pr.BaseBranch // Get the commit from BaseBranch where the pull request got merged - mergeCommit, stderr, err := process.GetManager().ExecDirEnv(-1, "", fmt.Sprintf("isMerged (git rev-list --ancestry-path --merges --reverse): %d", pr.BaseRepo.ID), - []string{"GIT_INDEX_FILE=" + indexTmpPath, "GIT_DIR=" + pr.BaseRepo.RepoPath()}, - git.GitExecutable, "rev-list", "--ancestry-path", "--merges", "--reverse", cmd) + mergeCommit, err := git.NewCommand("rev-list", "--ancestry-path", "--merges", "--reverse", cmd).RunInDirWithEnv("", []string{"GIT_INDEX_FILE=" + indexTmpPath, "GIT_DIR=" + pr.BaseRepo.RepoPath()}) if err != nil { - return nil, fmt.Errorf("git rev-list --ancestry-path --merges --reverse: %v %v", stderr, err) + return nil, fmt.Errorf("git rev-list --ancestry-path --merges --reverse: %v", err) } else if len(mergeCommit) < 40 { // PR was fast-forwarded, so just use last commit of PR mergeCommit = commitID[:40] @@ -621,12 +615,9 @@ func (pr *PullRequest) testPatch(e Engine) (err error) { indexTmpPath := filepath.Join(os.TempDir(), "gitea-"+pr.BaseRepo.Name+"-"+strconv.Itoa(time.Now().Nanosecond())) defer os.Remove(indexTmpPath) - var stderr string - _, stderr, err = process.GetManager().ExecDirEnv(-1, "", fmt.Sprintf("testPatch (git read-tree): %d", pr.BaseRepo.ID), - []string{"GIT_DIR=" + pr.BaseRepo.RepoPath(), "GIT_INDEX_FILE=" + indexTmpPath}, - git.GitExecutable, "read-tree", pr.BaseBranch) + _, err = git.NewCommand("read-tree", pr.BaseBranch).RunInDirWithEnv("", []string{"GIT_DIR=" + pr.BaseRepo.RepoPath(), "GIT_INDEX_FILE=" + indexTmpPath}) if err != nil { - return fmt.Errorf("git read-tree --index-output=%s %s: %v - %s", indexTmpPath, pr.BaseBranch, err, stderr) + return fmt.Errorf("git read-tree --index-output=%s %s: %v", indexTmpPath, pr.BaseBranch, err) } prUnit, err := pr.BaseRepo.getUnit(e, UnitTypePullRequests) @@ -642,9 +633,15 @@ func (pr *PullRequest) testPatch(e Engine) (err error) { args = append(args, patchPath) pr.ConflictedFiles = []string{} - _, stderr, err = process.GetManager().ExecDirEnv(-1, "", fmt.Sprintf("testPatch (git apply --check): %d", pr.BaseRepo.ID), + stderrBuilder := new(strings.Builder) + err = git.NewCommand(args...).RunInDirTimeoutEnvPipeline( []string{"GIT_INDEX_FILE=" + indexTmpPath, "GIT_DIR=" + pr.BaseRepo.RepoPath()}, - git.GitExecutable, args...) + -1, + "", + nil, + stderrBuilder) + stderr := stderrBuilder.String() + if err != nil { for i := range patchConflicts { if strings.Contains(stderr, patchConflicts[i]) { diff --git a/models/repo.go b/models/repo.go index 0ccf786db3bf..eec906535941 100644 --- a/models/repo.go +++ b/models/repo.go @@ -30,7 +30,6 @@ import ( "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/markup" "code.gitea.io/gitea/modules/options" - "code.gitea.io/gitea/modules/process" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/structs" api "code.gitea.io/gitea/modules/structs" @@ -1202,11 +1201,11 @@ func initRepoCommit(tmpPath string, u *User) (err error) { "GIT_COMMITTER_DATE="+commitTimeStr, ) - var stderr string - if _, stderr, err = process.GetManager().ExecDir(-1, - tmpPath, fmt.Sprintf("initRepoCommit (git add): %s", tmpPath), - git.GitExecutable, "add", "--all"); err != nil { - return fmt.Errorf("git add: %s", stderr) + if stdout, err := git.NewCommand("add", "--all"). + SetDescription(fmt.Sprintf("initRepoCommit (git add): %s", tmpPath)). + RunInDir(tmpPath); err != nil { + log.Error("git add --all failed: Stdout: %s\nError: %v", stdout, err) + return fmt.Errorf("git add --all: %v", err) } binVersion, err := git.BinVersion() @@ -1228,18 +1227,20 @@ func initRepoCommit(tmpPath string, u *User) (err error) { } } - if _, stderr, err = process.GetManager().ExecDirEnv(-1, - tmpPath, fmt.Sprintf("initRepoCommit (git commit): %s", tmpPath), - env, - git.GitExecutable, args...); err != nil { - return fmt.Errorf("git commit: %s", stderr) + if stdout, err := git.NewCommand(args...). + SetDescription(fmt.Sprintf("initRepoCommit (git commit): %s", tmpPath)). + RunInDirWithEnv(tmpPath, env); err != nil { + log.Error("Failed to commit: %v: Stdout: %s\nError: %v", args, stdout, err) + return fmt.Errorf("git commit: %v", err) } - if _, stderr, err = process.GetManager().ExecDir(-1, - tmpPath, fmt.Sprintf("initRepoCommit (git push): %s", tmpPath), - git.GitExecutable, "push", "origin", "master"); err != nil { - return fmt.Errorf("git push: %s", stderr) + if stdout, err := git.NewCommand("push", "origin", "master"). + SetDescription(fmt.Sprintf("initRepoCommit (git push): %s", tmpPath)). + RunInDir(tmpPath); err != nil { + log.Error("Failed to push back to master: Stdout: %s\nError: %v", stdout, err) + return fmt.Errorf("git push: %v", err) } + return nil } @@ -1297,14 +1298,11 @@ func prepareRepoCommit(e Engine, repo *Repository, tmpDir, repoPath string, opts ) // Clone to temporary path and do the init commit. - _, stderr, err := process.GetManager().ExecDirEnv( - -1, "", - fmt.Sprintf("initRepository(git clone): %s", repoPath), - env, - git.GitExecutable, "clone", repoPath, tmpDir, - ) - if err != nil { - return fmt.Errorf("git clone: %v - %s", err, stderr) + if stdout, err := git.NewCommand("clone", repoPath, tmpDir). + SetDescription(fmt.Sprintf("initRepository (git clone): %s to %s", repoPath, tmpDir)). + RunInDirWithEnv("", env); err != nil { + log.Error("Failed to clone from %v into %s: stdout: %s\nError: %v", repo, tmpDir, stdout, err) + return fmt.Errorf("git clone: %v", err) } // README @@ -1584,11 +1582,11 @@ func CreateRepository(doer, u *User, opts CreateRepoOptions) (_ *Repository, err } } - _, stderr, err := process.GetManager().ExecDir(-1, - repoPath, fmt.Sprintf("CreateRepository(git update-server-info): %s", repoPath), - git.GitExecutable, "update-server-info") - if err != nil { - return nil, errors.New("CreateRepository(git update-server-info): " + stderr) + if stdout, err := git.NewCommand("update-server-info"). + SetDescription(fmt.Sprintf("CreateRepository(git update-server-info): %s", repoPath)). + RunInDir(repoPath); err != nil { + log.Error("CreateRepitory(git update-server-info) in %v: Stdout: %s\nError: %v", repo, stdout, err) + return nil, fmt.Errorf("CreateRepository(git update-server-info): %v", err) } } @@ -2422,12 +2420,13 @@ func GitGcRepos() error { if err := repo.GetOwner(); err != nil { return err } - _, stderr, err := process.GetManager().ExecDir( - time.Duration(setting.Git.Timeout.GC)*time.Second, - RepoPath(repo.Owner.Name, repo.Name), "Repository garbage collection", - git.GitExecutable, args...) - if err != nil { - return fmt.Errorf("%v: %v", err, stderr) + if stdout, err := git.NewCommand(args...). + SetDescription(fmt.Sprintf("Repository Garbage Collection: %s", repo.FullName())). + RunInDirTimeout( + time.Duration(setting.Git.Timeout.GC)*time.Second, + RepoPath(repo.Owner.Name, repo.Name)); err != nil { + log.Error("Repository garbage collection failed for %v. Stdout: %s\nError: %v", repo, stdout, err) + return fmt.Errorf("Repository garbage collection failed: Error: %v", err) } return nil }) @@ -2647,18 +2646,19 @@ func ForkRepository(doer, owner *User, oldRepo *Repository, name, desc string) ( } repoPath := RepoPath(owner.Name, repo.Name) - _, stderr, err := process.GetManager().ExecTimeout(10*time.Minute, - fmt.Sprintf("ForkRepository(git clone): %s/%s", owner.Name, repo.Name), - git.GitExecutable, "clone", "--bare", oldRepo.repoPath(sess), repoPath) - if err != nil { - return nil, fmt.Errorf("git clone: %v", stderr) - } - - _, stderr, err = process.GetManager().ExecDir(-1, - repoPath, fmt.Sprintf("ForkRepository(git update-server-info): %s", repoPath), - git.GitExecutable, "update-server-info") - if err != nil { - return nil, fmt.Errorf("git update-server-info: %v", stderr) + if stdout, err := git.NewCommand( + "clone", "--bare", oldRepo.repoPath(sess), repoPath). + SetDescription(fmt.Sprintf("ForkRepository(git clone): %s to %s", oldRepo.FullName(), repo.FullName())). + RunInDirTimeout(10*time.Minute, ""); err != nil { + log.Error("Fork Repository (git clone) Failed for %v (from %v):\nStdout: %s\nError: %v", repo, oldRepo, stdout, err) + return nil, fmt.Errorf("git clone: %v", err) + } + + if stdout, err := git.NewCommand("update-server-info"). + SetDescription(fmt.Sprintf("ForkRepository(git update-server-info): %s", repo.FullName())). + RunInDir(repoPath); err != nil { + log.Error("Fork Repository (git update-server-info) failed for %v:\nStdout: %s\nError: %v", repo, stdout, err) + return nil, fmt.Errorf("git update-server-info: %v", err) } if err = createDelegateHooks(repoPath); err != nil { diff --git a/models/repo_generate.go b/models/repo_generate.go index 56a3940ac18c..6dd8540d9e14 100644 --- a/models/repo_generate.go +++ b/models/repo_generate.go @@ -16,7 +16,6 @@ import ( "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" - "code.gitea.io/gitea/modules/process" "code.gitea.io/gitea/modules/util" "github.com/gobwas/glob" @@ -168,14 +167,11 @@ func generateRepoCommit(e Engine, repo, templateRepo, generateRepo *Repository, } repoPath := repo.repoPath(e) - _, stderr, err := process.GetManager().ExecDirEnv( - -1, tmpDir, - fmt.Sprintf("generateRepoCommit(git remote add): %s", repoPath), - env, - git.GitExecutable, "remote", "add", "origin", repoPath, - ) - if err != nil { - return fmt.Errorf("git remote add: %v - %s", err, stderr) + if stdout, err := git.NewCommand("remote", "add", "origin", repoPath). + SetDescription(fmt.Sprintf("generateRepoCommit (git remote add): %s to %s", templateRepoPath, tmpDir)). + RunInDirWithEnv(tmpDir, env); err != nil { + log.Error("Unable to add %v as remote origin to temporary repo to %s: stdout %s\nError: %v", repo, tmpDir, stdout, err) + return fmt.Errorf("git remote add: %v", err) } return initRepoCommit(tmpDir, repo.Owner) diff --git a/modules/git/blame.go b/modules/git/blame.go index 4f4343fe9642..8884fc074099 100644 --- a/modules/git/blame.go +++ b/modules/git/blame.go @@ -97,7 +97,9 @@ func CreateBlameReader(repoPath, commitID, file string) (*BlameReader, error) { } func createBlameReader(dir string, command ...string) (*BlameReader, error) { - cmd := exec.Command(command[0], command[1:]...) + // FIXME: graceful: This should have a timeout + // FIXME: graceful: This should create its own subcontext + cmd := exec.CommandContext(DefaultContext, command[0], command[1:]...) cmd.Dir = dir cmd.Stderr = os.Stderr diff --git a/modules/git/command.go b/modules/git/command.go index 65878edb7de6..74aa6b356278 100644 --- a/modules/git/command.go +++ b/modules/git/command.go @@ -30,8 +30,10 @@ const DefaultLocale = "C" // Command represents a command with its subcommands or arguments. type Command struct { - name string - args []string + name string + args []string + parentContext context.Context + desc string } func (c *Command) String() string { @@ -47,8 +49,9 @@ func NewCommand(args ...string) *Command { cargs := make([]string, len(GlobalCommandArgs)) copy(cargs, GlobalCommandArgs) return &Command{ - name: GitExecutable, - args: append(cargs, args...), + name: GitExecutable, + args: append(cargs, args...), + parentContext: DefaultContext, } } @@ -60,6 +63,19 @@ func NewCommandNoGlobals(args ...string) *Command { } } +// SetParentContext sets the parent context for this command +func (c *Command) SetParentContext(ctx context.Context) *Command { + c.parentContext = ctx + return c +} + +// SetDescription sets the description for this command which be returned on +// c.String() +func (c *Command) SetDescription(desc string) *Command { + c.desc = desc + return c +} + // AddArguments adds new argument(s) to the command. func (c *Command) AddArguments(args ...string) *Command { c.args = append(c.args, args...) @@ -92,7 +108,7 @@ func (c *Command) RunInDirTimeoutEnvFullPipelineFunc(env []string, timeout time. log("%s: %v", dir, c) } - ctx, cancel := context.WithTimeout(context.Background(), timeout) + ctx, cancel := context.WithTimeout(c.parentContext, timeout) defer cancel() cmd := exec.CommandContext(ctx, c.name, c.args...) @@ -110,7 +126,11 @@ func (c *Command) RunInDirTimeoutEnvFullPipelineFunc(env []string, timeout time. return err } - pid := process.GetManager().Add(fmt.Sprintf("%s %s %s [repo_path: %s]", GitExecutable, c.name, strings.Join(c.args, " "), dir), cmd) + desc := c.desc + if desc == "" { + desc = fmt.Sprintf("%s %s %s [repo_path: %s]", GitExecutable, c.name, strings.Join(c.args, " "), dir) + } + pid := process.GetManager().Add(desc, cmd) defer process.GetManager().Remove(pid) if fn != nil { diff --git a/modules/git/git.go b/modules/git/git.go index df50eac72a3e..286e1ad8b4c4 100644 --- a/modules/git/git.go +++ b/modules/git/git.go @@ -6,6 +6,7 @@ package git import ( + "context" "fmt" "os/exec" "runtime" @@ -35,6 +36,9 @@ var ( // Could be updated to an absolute path while initialization GitExecutable = "git" + // DefaultContext is the default context to run git commands in + DefaultContext = context.Background() + gitVersion string ) diff --git a/modules/graceful/manager.go b/modules/graceful/manager.go index b5b5293fdd6e..3520e202fd1d 100644 --- a/modules/graceful/manager.go +++ b/modules/graceful/manager.go @@ -8,6 +8,8 @@ import ( "context" "time" + "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/process" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" ) @@ -36,6 +38,10 @@ var Manager *gracefulManager func init() { Manager = newGracefulManager(context.Background()) + // Set the git default context to the HammerContext + git.DefaultContext = Manager.HammerContext() + // Set the process default context to the HammerContext + process.DefaultContext = Manager.HammerContext() } // CallbackWithContext is combined runnable and context to watch to see if the caller has finished diff --git a/modules/process/manager.go b/modules/process/manager.go index 3e77c0a6a90f..04eb7ef5868f 100644 --- a/modules/process/manager.go +++ b/modules/process/manager.go @@ -24,6 +24,9 @@ var ( // ErrExecTimeout represent a timeout error ErrExecTimeout = errors.New("Process execution timeout") manager *Manager + + // DefaultContext is the default context to run processing commands in + DefaultContext = context.Background() ) // Process represents a working process inherit from Gogs. @@ -110,7 +113,7 @@ func (pm *Manager) ExecDirEnvStdIn(timeout time.Duration, dir, desc string, env stdOut := new(bytes.Buffer) stdErr := new(bytes.Buffer) - ctx, cancel := context.WithTimeout(context.Background(), timeout) + ctx, cancel := context.WithTimeout(DefaultContext, timeout) defer cancel() cmd := exec.CommandContext(ctx, cmdName, args...) diff --git a/routers/repo/http.go b/routers/repo/http.go index 0025ba2af4bb..e56325d21c98 100644 --- a/routers/repo/http.go +++ b/routers/repo/http.go @@ -24,6 +24,7 @@ import ( "code.gitea.io/gitea/modules/context" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/process" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/timeutil" ) @@ -463,8 +464,9 @@ func serviceRPC(h serviceHandler, service string) { // set this for allow pre-receive and post-receive execute h.environ = append(h.environ, "SSH_ORIGINAL_COMMAND="+service) + // FIXME: graceful: Is the defaultContext appropriate here or should it be the requests context? var stderr bytes.Buffer - cmd := exec.Command(git.GitExecutable, service, "--stateless-rpc", h.dir) + cmd := exec.CommandContext(git.DefaultContext, git.GitExecutable, service, "--stateless-rpc", h.dir) cmd.Dir = h.dir if service == "receive-pack" { cmd.Env = append(os.Environ(), h.environ...) @@ -472,6 +474,10 @@ func serviceRPC(h serviceHandler, service string) { cmd.Stdout = h.w cmd.Stdin = reqBody cmd.Stderr = &stderr + + pid := process.GetManager().Add(fmt.Sprintf("%s %s %s [repo_path: %s]", git.GitExecutable, service, "--stateless-rpc", h.dir), cmd) + defer process.GetManager().Remove(pid) + if err := cmd.Run(); err != nil { log.Error("Fail to serve RPC(%s): %v - %s", service, err, stderr.String()) return diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 351bfef156d9..e834d9832bc0 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -825,9 +825,10 @@ func GetDiffRangeWithWhitespaceBehavior(repoPath, beforeCommitID, afterCommitID return nil, err } + // FIXME: graceful: These commands should likely have a timeout var cmd *exec.Cmd if len(beforeCommitID) == 0 && commit.ParentCount() == 0 { - cmd = exec.Command(git.GitExecutable, "show", afterCommitID) + cmd = exec.CommandContext(git.DefaultContext, git.GitExecutable, "show", afterCommitID) } else { actualBeforeCommitID := beforeCommitID if len(actualBeforeCommitID) == 0 { @@ -840,7 +841,7 @@ func GetDiffRangeWithWhitespaceBehavior(repoPath, beforeCommitID, afterCommitID } diffArgs = append(diffArgs, actualBeforeCommitID) diffArgs = append(diffArgs, afterCommitID) - cmd = exec.Command(git.GitExecutable, diffArgs...) + cmd = exec.CommandContext(git.DefaultContext, git.GitExecutable, diffArgs...) beforeCommitID = actualBeforeCommitID } cmd.Dir = repoPath @@ -908,27 +909,28 @@ func GetRawDiffForFile(repoPath, startCommit, endCommit string, diffType RawDiff if len(file) > 0 { fileArgs = append(fileArgs, "--", file) } + // FIXME: graceful: These commands should have a timeout var cmd *exec.Cmd switch diffType { case RawDiffNormal: if len(startCommit) != 0 { - cmd = exec.Command(git.GitExecutable, append([]string{"diff", "-M", startCommit, endCommit}, fileArgs...)...) + cmd = exec.CommandContext(git.DefaultContext, git.GitExecutable, append([]string{"diff", "-M", startCommit, endCommit}, fileArgs...)...) } else if commit.ParentCount() == 0 { - cmd = exec.Command(git.GitExecutable, append([]string{"show", endCommit}, fileArgs...)...) + cmd = exec.CommandContext(git.DefaultContext, git.GitExecutable, append([]string{"show", endCommit}, fileArgs...)...) } else { c, _ := commit.Parent(0) - cmd = exec.Command(git.GitExecutable, append([]string{"diff", "-M", c.ID.String(), endCommit}, fileArgs...)...) + cmd = exec.CommandContext(git.DefaultContext, git.GitExecutable, append([]string{"diff", "-M", c.ID.String(), endCommit}, fileArgs...)...) } case RawDiffPatch: if len(startCommit) != 0 { query := fmt.Sprintf("%s...%s", endCommit, startCommit) - cmd = exec.Command(git.GitExecutable, append([]string{"format-patch", "--no-signature", "--stdout", "--root", query}, fileArgs...)...) + cmd = exec.CommandContext(git.DefaultContext, git.GitExecutable, append([]string{"format-patch", "--no-signature", "--stdout", "--root", query}, fileArgs...)...) } else if commit.ParentCount() == 0 { - cmd = exec.Command(git.GitExecutable, append([]string{"format-patch", "--no-signature", "--stdout", "--root", endCommit}, fileArgs...)...) + cmd = exec.CommandContext(git.DefaultContext, git.GitExecutable, append([]string{"format-patch", "--no-signature", "--stdout", "--root", endCommit}, fileArgs...)...) } else { c, _ := commit.Parent(0) query := fmt.Sprintf("%s...%s", endCommit, c.ID.String()) - cmd = exec.Command(git.GitExecutable, append([]string{"format-patch", "--no-signature", "--stdout", query}, fileArgs...)...) + cmd = exec.CommandContext(git.DefaultContext, git.GitExecutable, append([]string{"format-patch", "--no-signature", "--stdout", query}, fileArgs...)...) } default: return fmt.Errorf("invalid diffType: %s", diffType) @@ -939,6 +941,9 @@ func GetRawDiffForFile(repoPath, startCommit, endCommit string, diffType RawDiff cmd.Dir = repoPath cmd.Stdout = writer cmd.Stderr = stderr + pid := process.GetManager().Add(fmt.Sprintf("GetRawDiffForFile: [repo_path: %s]", repoPath), cmd) + defer process.GetManager().Remove(pid) + if err = cmd.Run(); err != nil { return fmt.Errorf("Run: %v - %s", err, stderr) } diff --git a/services/mirror/mirror.go b/services/mirror/mirror.go index d35a205d0013..9c52f1723ba3 100644 --- a/services/mirror/mirror.go +++ b/services/mirror/mirror.go @@ -15,7 +15,6 @@ import ( "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/notification" - "code.gitea.io/gitea/modules/process" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/sync" "code.gitea.io/gitea/modules/timeutil" @@ -172,25 +171,36 @@ func runSync(m *models.Mirror) ([]*mirrorSyncResult, bool) { gitArgs = append(gitArgs, "--prune") } - _, stderr, err := process.GetManager().ExecDir( - timeout, repoPath, fmt.Sprintf("Mirror.runSync: %s", repoPath), - git.GitExecutable, gitArgs...) - if err != nil { + stdoutBuilder := strings.Builder{} + stderrBuilder := strings.Builder{} + if err := git.NewCommand(gitArgs...). + SetDescription(fmt.Sprintf("Mirror.runSync: %s", m.Repo.FullName())). + RunInDirTimeoutPipeline(timeout, repoPath, &stdoutBuilder, &stderrBuilder); err != nil { + stdout := stdoutBuilder.String() + stderr := stderrBuilder.String() // sanitize the output, since it may contain the remote address, which may // contain a password - message, err := sanitizeOutput(stderr, repoPath) + stderrMessage, sanitizeErr := sanitizeOutput(stderr, repoPath) + if sanitizeErr != nil { + log.Error("sanitizeOutput failed on stderr: %v", sanitizeErr) + log.Error("Failed to update mirror repository %v:\nStdout: %s\nStderr: %s\nErr: %v", m.Repo, stdout, stderr, err) + return nil, false + } + stdoutMessage, err := sanitizeOutput(stdout, repoPath) if err != nil { - log.Error("sanitizeOutput: %v", err) + log.Error("sanitizeOutput failed: %v", sanitizeErr) + log.Error("Failed to update mirror repository %v:\nStdout: %s\nStderr: %s\nErr: %v", m.Repo, stdout, stderrMessage, err) return nil, false } - desc := fmt.Sprintf("Failed to update mirror repository '%s': %s", repoPath, message) - log.Error(desc) + + log.Error("Failed to update mirror repository %v:\nStdout: %s\nStderr: %s\nErr: %v", m.Repo, stdoutMessage, stderrMessage, err) + desc := fmt.Sprintf("Failed to update mirror repository '%s': %s", repoPath, stderrMessage) if err = models.CreateRepositoryNotice(desc); err != nil { log.Error("CreateRepositoryNotice: %v", err) } return nil, false } - output := stderr + output := stderrBuilder.String() gitRepo, err := git.OpenRepository(repoPath) if err != nil { @@ -208,18 +218,30 @@ func runSync(m *models.Mirror) ([]*mirrorSyncResult, bool) { } if m.Repo.HasWiki() { - if _, stderr, err := process.GetManager().ExecDir( - timeout, wikiPath, fmt.Sprintf("Mirror.runSync: %s", wikiPath), - git.GitExecutable, "remote", "update", "--prune"); err != nil { + stderrBuilder.Reset() + stdoutBuilder.Reset() + if err := git.NewCommand("remote", "update", "--prune"). + SetDescription(fmt.Sprintf("Mirror.runSync Wiki: %s ", m.Repo.FullName())). + RunInDirTimeoutPipeline(timeout, wikiPath, &stdoutBuilder, &stderrBuilder); err != nil { + stdout := stdoutBuilder.String() + stderr := stderrBuilder.String() // sanitize the output, since it may contain the remote address, which may // contain a password - message, err := sanitizeOutput(stderr, wikiPath) + stderrMessage, sanitizeErr := sanitizeOutput(stderr, repoPath) + if sanitizeErr != nil { + log.Error("sanitizeOutput failed on stderr: %v", sanitizeErr) + log.Error("Failed to update mirror repository wiki %v:\nStdout: %s\nStderr: %s\nErr: %v", m.Repo, stdout, stderr, err) + return nil, false + } + stdoutMessage, err := sanitizeOutput(stdout, repoPath) if err != nil { - log.Error("sanitizeOutput: %v", err) + log.Error("sanitizeOutput failed: %v", sanitizeErr) + log.Error("Failed to update mirror repository wiki %v:\nStdout: %s\nStderr: %s\nErr: %v", m.Repo, stdout, stderrMessage, err) return nil, false } - desc := fmt.Sprintf("Failed to update mirror wiki repository '%s': %s", wikiPath, message) - log.Error(desc) + + log.Error("Failed to update mirror repository wiki %v:\nStdout: %s\nStderr: %s\nErr: %v", m.Repo, stdoutMessage, stderrMessage, err) + desc := fmt.Sprintf("Failed to update mirror repository wiki '%s': %s", wikiPath, stderrMessage) if err = models.CreateRepositoryNotice(desc); err != nil { log.Error("CreateRepositoryNotice: %v", err) } diff --git a/services/release/release.go b/services/release/release.go index 2f70bbb66580..fd0c410e7cbf 100644 --- a/services/release/release.go +++ b/services/release/release.go @@ -13,7 +13,6 @@ import ( "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/notification" - "code.gitea.io/gitea/modules/process" "code.gitea.io/gitea/modules/timeutil" ) @@ -128,11 +127,11 @@ func DeleteReleaseByID(id int64, doer *models.User, delTag bool) error { } if delTag { - _, stderr, err := process.GetManager().ExecDir(-1, repo.RepoPath(), - fmt.Sprintf("DeleteReleaseByID (git tag -d): %d", rel.ID), - git.GitExecutable, "tag", "-d", rel.TagName) - if err != nil && !strings.Contains(stderr, "not found") { - return fmt.Errorf("git tag -d: %v - %s", err, stderr) + if stdout, err := git.NewCommand("tag", "-d", rel.TagName). + SetDescription(fmt.Sprintf("DeleteReleaseByID (git tag -d): %d", rel.ID)). + RunInDir(repo.RepoPath()); err != nil && !strings.Contains(err.Error(), "not found") { + log.Error("DeleteReleaseByID (git tag -d): %d in %v Failed:\nStdout: %s\nError: %v", rel.ID, repo, stdout, err) + return fmt.Errorf("git tag -d: %v", err) } if err := models.DeleteReleaseByID(id); err != nil { From 84315efa23a4bacd59abddf85943f47f2f9cd9ec Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 19 Nov 2019 16:43:06 +0000 Subject: [PATCH 04/11] Graceful: Always Hammer after Shutdown --- modules/git/command.go | 5 +++-- modules/graceful/manager.go | 4 +++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/modules/git/command.go b/modules/git/command.go index 74aa6b356278..ceb43dc09b44 100644 --- a/modules/git/command.go +++ b/modules/git/command.go @@ -58,8 +58,9 @@ func NewCommand(args ...string) *Command { // NewCommandNoGlobals creates and returns a new Git Command based on given command and arguments only with the specify args and don't care global command args func NewCommandNoGlobals(args ...string) *Command { return &Command{ - name: GitExecutable, - args: args, + name: GitExecutable, + args: args, + parentContext: DefaultContext, } } diff --git a/modules/graceful/manager.go b/modules/graceful/manager.go index 3520e202fd1d..c6d96a2146a2 100644 --- a/modules/graceful/manager.go +++ b/modules/graceful/manager.go @@ -9,8 +9,8 @@ import ( "time" "code.gitea.io/gitea/modules/git" - "code.gitea.io/gitea/modules/process" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/process" "code.gitea.io/gitea/modules/setting" ) @@ -154,6 +154,8 @@ func (g *gracefulManager) doShutdown() { } go func() { g.WaitForServers() + // Mop up any remaining unclosed events. + g.doHammerTime(0) <-time.After(1 * time.Second) g.doTerminate() }() From 03ffe0b3858675fd4bf4b9dd71d45e94a0bb2a34 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 27 Nov 2019 20:32:47 +0000 Subject: [PATCH 05/11] ProcessManager: Add cancel functionality --- modules/git/blame.go | 14 ++++++++++---- modules/git/command.go | 2 +- modules/process/manager.go | 18 +++++++++++++++--- options/locale/locale_en-US.ini | 3 +++ routers/admin/admin.go | 9 +++++++++ routers/repo/http.go | 7 +++++-- routers/routes/routes.go | 1 + services/gitdiff/gitdiff.go | 26 ++++++++++++++++---------- templates/admin/monitor.tmpl | 13 +++++++++++++ 9 files changed, 73 insertions(+), 20 deletions(-) diff --git a/modules/git/blame.go b/modules/git/blame.go index 8884fc074099..e963a559b4a6 100644 --- a/modules/git/blame.go +++ b/modules/git/blame.go @@ -6,6 +6,7 @@ package git import ( "bufio" + "context" "fmt" "io" "os" @@ -28,6 +29,7 @@ type BlameReader struct { output io.ReadCloser scanner *bufio.Scanner lastSha *string + cancel context.CancelFunc } var shaLineRegex = regexp.MustCompile("^([a-z0-9]{40})") @@ -76,7 +78,8 @@ func (r *BlameReader) NextPart() (*BlamePart, error) { // Close BlameReader - don't run NextPart after invoking that func (r *BlameReader) Close() error { - process.GetManager().Remove(r.pid) + defer process.GetManager().Remove(r.pid) + defer r.cancel() if err := r.cmd.Wait(); err != nil { return fmt.Errorf("Wait: %v", err) @@ -98,21 +101,23 @@ func CreateBlameReader(repoPath, commitID, file string) (*BlameReader, error) { func createBlameReader(dir string, command ...string) (*BlameReader, error) { // FIXME: graceful: This should have a timeout - // FIXME: graceful: This should create its own subcontext - cmd := exec.CommandContext(DefaultContext, command[0], command[1:]...) + ctx, cancel := context.WithCancel(DefaultContext) + cmd := exec.CommandContext(ctx, command[0], command[1:]...) cmd.Dir = dir cmd.Stderr = os.Stderr stdout, err := cmd.StdoutPipe() if err != nil { + defer cancel() return nil, fmt.Errorf("StdoutPipe: %v", err) } if err = cmd.Start(); err != nil { + defer cancel() return nil, fmt.Errorf("Start: %v", err) } - pid := process.GetManager().Add(fmt.Sprintf("GetBlame [repo_path: %s]", dir), cmd) + pid := process.GetManager().Add(fmt.Sprintf("GetBlame [repo_path: %s]", dir), cmd, cancel) scanner := bufio.NewScanner(stdout) @@ -122,5 +127,6 @@ func createBlameReader(dir string, command ...string) (*BlameReader, error) { stdout, scanner, nil, + cancel, }, nil } diff --git a/modules/git/command.go b/modules/git/command.go index ceb43dc09b44..ad77aabb6bf4 100644 --- a/modules/git/command.go +++ b/modules/git/command.go @@ -131,7 +131,7 @@ func (c *Command) RunInDirTimeoutEnvFullPipelineFunc(env []string, timeout time. if desc == "" { desc = fmt.Sprintf("%s %s %s [repo_path: %s]", GitExecutable, c.name, strings.Join(c.args, " "), dir) } - pid := process.GetManager().Add(desc, cmd) + pid := process.GetManager().Add(desc, cmd, cancel) defer process.GetManager().Remove(pid) if fn != nil { diff --git a/modules/process/manager.go b/modules/process/manager.go index 04eb7ef5868f..1b68f93253b4 100644 --- a/modules/process/manager.go +++ b/modules/process/manager.go @@ -29,12 +29,13 @@ var ( DefaultContext = context.Background() ) -// Process represents a working process inherit from Gogs. +// Process represents a working process inheriting from Gitea. type Process struct { PID int64 // Process ID, not system one. Description string Start time.Time Cmd *exec.Cmd + Cancel context.CancelFunc } // Manager knows about all processes and counts PIDs. @@ -56,7 +57,7 @@ func GetManager() *Manager { } // Add a process to the ProcessManager and returns its PID. -func (pm *Manager) Add(description string, cmd *exec.Cmd) int64 { +func (pm *Manager) Add(description string, cmd *exec.Cmd, cancel context.CancelFunc) int64 { pm.mutex.Lock() pid := pm.counter + 1 pm.Processes[pid] = &Process{ @@ -64,6 +65,7 @@ func (pm *Manager) Add(description string, cmd *exec.Cmd) int64 { Description: description, Start: time.Now(), Cmd: cmd, + Cancel: cancel, } pm.counter = pid pm.mutex.Unlock() @@ -78,6 +80,16 @@ func (pm *Manager) Remove(pid int64) { pm.mutex.Unlock() } +// Cancel a process in the ProcessManager. +func (pm *Manager) Cancel(pid int64) { + pm.mutex.Lock() + process, ok := pm.Processes[pid] + pm.mutex.Unlock() + if ok { + process.Cancel() + } +} + // Exec a command and use the default timeout. func (pm *Manager) Exec(desc, cmdName string, args ...string) (string, string, error) { return pm.ExecDir(-1, "", desc, cmdName, args...) @@ -129,7 +141,7 @@ func (pm *Manager) ExecDirEnvStdIn(timeout time.Duration, dir, desc string, env return "", "", err } - pid := pm.Add(desc, cmd) + pid := pm.Add(desc, cmd, cancel) err := cmd.Wait() pm.Remove(pid) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index be4522014f37..ae93026bddfd 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1999,6 +1999,9 @@ monitor.process = Running Processes monitor.desc = Description monitor.start = Start Time monitor.execute_time = Execution Time +monitor.process.cancel = Cancel process +monitor.process.cancel_desc = Cancelling a process may cause data loss +monitor.process.cancel_notices = Cancel: %s? notices.system_notice_list = System Notices notices.view_detail_header = View Notice Details diff --git a/routers/admin/admin.go b/routers/admin/admin.go index 45bdbfe7f2d4..4bc6f8a9285f 100644 --- a/routers/admin/admin.go +++ b/routers/admin/admin.go @@ -356,3 +356,12 @@ func Monitor(ctx *context.Context) { ctx.Data["Entries"] = cron.ListTasks() ctx.HTML(200, tplMonitor) } + +// MonitorCancel cancels a process +func MonitorCancel(ctx *context.Context) { + pid := ctx.ParamsInt64("pid") + process.GetManager().Cancel(pid) + ctx.JSON(200, map[string]interface{}{ + "redirect": ctx.Repo.RepoLink + "/admin/monitor", + }) +} diff --git a/routers/repo/http.go b/routers/repo/http.go index e56325d21c98..b67cc9c3bd57 100644 --- a/routers/repo/http.go +++ b/routers/repo/http.go @@ -8,6 +8,7 @@ package repo import ( "bytes" "compress/gzip" + gocontext "context" "fmt" "net/http" "os" @@ -465,8 +466,10 @@ func serviceRPC(h serviceHandler, service string) { h.environ = append(h.environ, "SSH_ORIGINAL_COMMAND="+service) // FIXME: graceful: Is the defaultContext appropriate here or should it be the requests context? + ctx, cancel := gocontext.WithCancel(git.DefaultContext) + defer cancel() var stderr bytes.Buffer - cmd := exec.CommandContext(git.DefaultContext, git.GitExecutable, service, "--stateless-rpc", h.dir) + cmd := exec.CommandContext(ctx, git.GitExecutable, service, "--stateless-rpc", h.dir) cmd.Dir = h.dir if service == "receive-pack" { cmd.Env = append(os.Environ(), h.environ...) @@ -475,7 +478,7 @@ func serviceRPC(h serviceHandler, service string) { cmd.Stdin = reqBody cmd.Stderr = &stderr - pid := process.GetManager().Add(fmt.Sprintf("%s %s %s [repo_path: %s]", git.GitExecutable, service, "--stateless-rpc", h.dir), cmd) + pid := process.GetManager().Add(fmt.Sprintf("%s %s %s [repo_path: %s]", git.GitExecutable, service, "--stateless-rpc", h.dir), cmd, cancel) defer process.GetManager().Remove(pid) if err := cmd.Run(); err != nil { diff --git a/routers/routes/routes.go b/routers/routes/routes.go index 6de293c5072f..bac7d37916d8 100644 --- a/routers/routes/routes.go +++ b/routers/routes/routes.go @@ -422,6 +422,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Get("/config", admin.Config) m.Post("/config/test_mail", admin.SendTestMail) m.Get("/monitor", admin.Monitor) + m.Post("/monitor/cancel/:pid", admin.MonitorCancel) m.Group("/users", func() { m.Get("", admin.Users) diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index e834d9832bc0..3534dd756760 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -8,6 +8,7 @@ package gitdiff import ( "bufio" "bytes" + "context" "fmt" "html" "html/template" @@ -826,9 +827,11 @@ func GetDiffRangeWithWhitespaceBehavior(repoPath, beforeCommitID, afterCommitID } // FIXME: graceful: These commands should likely have a timeout + ctx, cancel := context.WithCancel(git.DefaultContext) + defer cancel() var cmd *exec.Cmd if len(beforeCommitID) == 0 && commit.ParentCount() == 0 { - cmd = exec.CommandContext(git.DefaultContext, git.GitExecutable, "show", afterCommitID) + cmd = exec.CommandContext(ctx, git.GitExecutable, "show", afterCommitID) } else { actualBeforeCommitID := beforeCommitID if len(actualBeforeCommitID) == 0 { @@ -841,7 +844,7 @@ func GetDiffRangeWithWhitespaceBehavior(repoPath, beforeCommitID, afterCommitID } diffArgs = append(diffArgs, actualBeforeCommitID) diffArgs = append(diffArgs, afterCommitID) - cmd = exec.CommandContext(git.DefaultContext, git.GitExecutable, diffArgs...) + cmd = exec.CommandContext(ctx, git.GitExecutable, diffArgs...) beforeCommitID = actualBeforeCommitID } cmd.Dir = repoPath @@ -856,7 +859,7 @@ func GetDiffRangeWithWhitespaceBehavior(repoPath, beforeCommitID, afterCommitID return nil, fmt.Errorf("Start: %v", err) } - pid := process.GetManager().Add(fmt.Sprintf("GetDiffRange [repo_path: %s]", repoPath), cmd) + pid := process.GetManager().Add(fmt.Sprintf("GetDiffRange [repo_path: %s]", repoPath), cmd, cancel) defer process.GetManager().Remove(pid) diff, err := ParsePatch(maxLines, maxLineCharacters, maxFiles, stdout) @@ -910,27 +913,30 @@ func GetRawDiffForFile(repoPath, startCommit, endCommit string, diffType RawDiff fileArgs = append(fileArgs, "--", file) } // FIXME: graceful: These commands should have a timeout + ctx, cancel := context.WithCancel(git.DefaultContext) + defer cancel() + var cmd *exec.Cmd switch diffType { case RawDiffNormal: if len(startCommit) != 0 { - cmd = exec.CommandContext(git.DefaultContext, git.GitExecutable, append([]string{"diff", "-M", startCommit, endCommit}, fileArgs...)...) + cmd = exec.CommandContext(ctx, git.GitExecutable, append([]string{"diff", "-M", startCommit, endCommit}, fileArgs...)...) } else if commit.ParentCount() == 0 { - cmd = exec.CommandContext(git.DefaultContext, git.GitExecutable, append([]string{"show", endCommit}, fileArgs...)...) + cmd = exec.CommandContext(ctx, git.GitExecutable, append([]string{"show", endCommit}, fileArgs...)...) } else { c, _ := commit.Parent(0) - cmd = exec.CommandContext(git.DefaultContext, git.GitExecutable, append([]string{"diff", "-M", c.ID.String(), endCommit}, fileArgs...)...) + cmd = exec.CommandContext(ctx, git.GitExecutable, append([]string{"diff", "-M", c.ID.String(), endCommit}, fileArgs...)...) } case RawDiffPatch: if len(startCommit) != 0 { query := fmt.Sprintf("%s...%s", endCommit, startCommit) - cmd = exec.CommandContext(git.DefaultContext, git.GitExecutable, append([]string{"format-patch", "--no-signature", "--stdout", "--root", query}, fileArgs...)...) + cmd = exec.CommandContext(ctx, git.GitExecutable, append([]string{"format-patch", "--no-signature", "--stdout", "--root", query}, fileArgs...)...) } else if commit.ParentCount() == 0 { - cmd = exec.CommandContext(git.DefaultContext, git.GitExecutable, append([]string{"format-patch", "--no-signature", "--stdout", "--root", endCommit}, fileArgs...)...) + cmd = exec.CommandContext(ctx, git.GitExecutable, append([]string{"format-patch", "--no-signature", "--stdout", "--root", endCommit}, fileArgs...)...) } else { c, _ := commit.Parent(0) query := fmt.Sprintf("%s...%s", endCommit, c.ID.String()) - cmd = exec.CommandContext(git.DefaultContext, git.GitExecutable, append([]string{"format-patch", "--no-signature", "--stdout", query}, fileArgs...)...) + cmd = exec.CommandContext(ctx, git.GitExecutable, append([]string{"format-patch", "--no-signature", "--stdout", query}, fileArgs...)...) } default: return fmt.Errorf("invalid diffType: %s", diffType) @@ -941,7 +947,7 @@ func GetRawDiffForFile(repoPath, startCommit, endCommit string, diffType RawDiff cmd.Dir = repoPath cmd.Stdout = writer cmd.Stderr = stderr - pid := process.GetManager().Add(fmt.Sprintf("GetRawDiffForFile: [repo_path: %s]", repoPath), cmd) + pid := process.GetManager().Add(fmt.Sprintf("GetRawDiffForFile: [repo_path: %s]", repoPath), cmd, cancel) defer process.GetManager().Remove(pid) if err = cmd.Run(); err != nil { diff --git a/templates/admin/monitor.tmpl b/templates/admin/monitor.tmpl index 6cc927d68f3d..38402fece2be 100644 --- a/templates/admin/monitor.tmpl +++ b/templates/admin/monitor.tmpl @@ -42,6 +42,7 @@ {{.i18n.Tr "admin.monitor.desc"}} {{.i18n.Tr "admin.monitor.start"}} {{.i18n.Tr "admin.monitor.execute_time"}} + @@ -51,6 +52,7 @@ {{.Description}} {{DateFmtLong .Start}} {{TimeSince .Start $.Lang}} + {{end}} @@ -58,4 +60,15 @@ + {{template "base/footer" .}} From ca90bea52034f2128d2f442f7b9dcb9e35cfec91 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Fri, 29 Nov 2019 13:18:38 -0600 Subject: [PATCH 06/11] Fix tests --- modules/process/manager_test.go | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/modules/process/manager_test.go b/modules/process/manager_test.go index 9980aba92172..7d542520c887 100644 --- a/modules/process/manager_test.go +++ b/modules/process/manager_test.go @@ -1,6 +1,7 @@ package process import ( + "context" "os/exec" "testing" "time" @@ -11,20 +12,35 @@ import ( func TestManager_Add(t *testing.T) { pm := Manager{Processes: make(map[int64]*Process)} - pid := pm.Add("foo", exec.Command("foo")) + pid := pm.Add("foo", exec.Command("foo"), nil) assert.Equal(t, int64(1), pid, "expected to get pid 1 got %d", pid) - pid = pm.Add("bar", exec.Command("bar")) + pid = pm.Add("bar", exec.Command("bar"), nil) assert.Equal(t, int64(2), pid, "expected to get pid 2 got %d", pid) } +func TestManager_Cancel(t *testing.T) { + pm := Manager{Processes: make(map[int64]*Process)} + + ctx, cancel := context.WithCancel(context.Background()) + pid := pm.Add("foo", exec.Command("foo"), cancel) + + pm.Cancel(pid) + + select { + case <-ctx.Done(): + default: + assert.Fail(t, "Cancel should cancel the provided context") + } +} + func TestManager_Remove(t *testing.T) { pm := Manager{Processes: make(map[int64]*Process)} - pid1 := pm.Add("foo", exec.Command("foo")) + pid1 := pm.Add("foo", exec.Command("foo"), nil) assert.Equal(t, int64(1), pid1, "expected to get pid 1 got %d", pid1) - pid2 := pm.Add("bar", exec.Command("bar")) + pid2 := pm.Add("bar", exec.Command("bar"), nil) assert.Equal(t, int64(2), pid2, "expected to get pid 2 got %d", pid2) pm.Remove(pid2) From db82213aff11407132cff5eb1a6894d5d6672f1a Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Fri, 29 Nov 2019 13:41:50 -0600 Subject: [PATCH 07/11] Make sure that process.Manager.Kill() cancels --- modules/process/manager.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/modules/process/manager.go b/modules/process/manager.go index 1b68f93253b4..d698297570e3 100644 --- a/modules/process/manager.go +++ b/modules/process/manager.go @@ -156,6 +156,9 @@ func (pm *Manager) ExecDirEnvStdIn(timeout time.Duration, dir, desc string, env func (pm *Manager) Kill(pid int64) error { if proc, exists := pm.Processes[pid]; exists { pm.mutex.Lock() + if proc.Cancel != nil { + proc.Cancel() + } if proc.Cmd != nil && proc.Cmd.Process != nil && proc.Cmd.ProcessState != nil && From 972ded0d18ec8a8e70cfec535ddc5b3cec92da8b Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sat, 30 Nov 2019 07:27:12 -0600 Subject: [PATCH 08/11] Make threadsafe access to Processes and remove own unused Kill --- modules/process/manager.go | 53 ++++++++++++++++++--------------- modules/process/manager_test.go | 8 ++--- routers/admin/admin.go | 2 +- 3 files changed, 34 insertions(+), 29 deletions(-) diff --git a/modules/process/manager.go b/modules/process/manager.go index d698297570e3..3daffa0272f7 100644 --- a/modules/process/manager.go +++ b/modules/process/manager.go @@ -12,6 +12,7 @@ import ( "fmt" "io" "os/exec" + "sort" "sync" "time" ) @@ -43,14 +44,14 @@ type Manager struct { mutex sync.Mutex counter int64 - Processes map[int64]*Process + processes map[int64]*Process } // GetManager returns a Manager and initializes one as singleton if there's none yet func GetManager() *Manager { if manager == nil { manager = &Manager{ - Processes: make(map[int64]*Process), + processes: make(map[int64]*Process), } } return manager @@ -60,7 +61,7 @@ func GetManager() *Manager { func (pm *Manager) Add(description string, cmd *exec.Cmd, cancel context.CancelFunc) int64 { pm.mutex.Lock() pid := pm.counter + 1 - pm.Processes[pid] = &Process{ + pm.processes[pid] = &Process{ PID: pid, Description: description, Start: time.Now(), @@ -76,20 +77,32 @@ func (pm *Manager) Add(description string, cmd *exec.Cmd, cancel context.CancelF // Remove a process from the ProcessManager. func (pm *Manager) Remove(pid int64) { pm.mutex.Lock() - delete(pm.Processes, pid) + delete(pm.processes, pid) pm.mutex.Unlock() } // Cancel a process in the ProcessManager. func (pm *Manager) Cancel(pid int64) { pm.mutex.Lock() - process, ok := pm.Processes[pid] + process, ok := pm.processes[pid] pm.mutex.Unlock() if ok { process.Cancel() } } +// Processes gets the processes in a thread safe manner +func (pm *Manager) Processes() []*Process { + pm.mutex.Lock() + processes := make([]*Process, 0, len(pm.processes)) + for _, process := range pm.processes { + processes = append(processes, process) + } + pm.mutex.Unlock() + sort.Sort(processList(processes)) + return processes +} + // Exec a command and use the default timeout. func (pm *Manager) Exec(desc, cmdName string, args ...string) (string, string, error) { return pm.ExecDir(-1, "", desc, cmdName, args...) @@ -152,24 +165,16 @@ func (pm *Manager) ExecDirEnvStdIn(timeout time.Duration, dir, desc string, env return stdOut.String(), stdErr.String(), err } -// Kill and remove a process from list. -func (pm *Manager) Kill(pid int64) error { - if proc, exists := pm.Processes[pid]; exists { - pm.mutex.Lock() - if proc.Cancel != nil { - proc.Cancel() - } - if proc.Cmd != nil && - proc.Cmd.Process != nil && - proc.Cmd.ProcessState != nil && - !proc.Cmd.ProcessState.Exited() { - if err := proc.Cmd.Process.Kill(); err != nil { - return fmt.Errorf("failed to kill process(%d/%s): %v", pid, proc.Description, err) - } - } - delete(pm.Processes, pid) - pm.mutex.Unlock() - } +type processList []*Process + +func (l processList) Len() int { + return len(l) +} + +func (l processList) Less(i, j int) bool { + return l[i].PID < l[j].PID +} - return nil +func (l processList) Swap(i, j int) { + l[i], l[j] = l[j], l[i] } diff --git a/modules/process/manager_test.go b/modules/process/manager_test.go index 7d542520c887..8db9010dfd63 100644 --- a/modules/process/manager_test.go +++ b/modules/process/manager_test.go @@ -10,7 +10,7 @@ import ( ) func TestManager_Add(t *testing.T) { - pm := Manager{Processes: make(map[int64]*Process)} + pm := Manager{processes: make(map[int64]*Process)} pid := pm.Add("foo", exec.Command("foo"), nil) assert.Equal(t, int64(1), pid, "expected to get pid 1 got %d", pid) @@ -20,7 +20,7 @@ func TestManager_Add(t *testing.T) { } func TestManager_Cancel(t *testing.T) { - pm := Manager{Processes: make(map[int64]*Process)} + pm := Manager{processes: make(map[int64]*Process)} ctx, cancel := context.WithCancel(context.Background()) pid := pm.Add("foo", exec.Command("foo"), cancel) @@ -35,7 +35,7 @@ func TestManager_Cancel(t *testing.T) { } func TestManager_Remove(t *testing.T) { - pm := Manager{Processes: make(map[int64]*Process)} + pm := Manager{processes: make(map[int64]*Process)} pid1 := pm.Add("foo", exec.Command("foo"), nil) assert.Equal(t, int64(1), pid1, "expected to get pid 1 got %d", pid1) @@ -45,7 +45,7 @@ func TestManager_Remove(t *testing.T) { pm.Remove(pid2) - _, exists := pm.Processes[pid2] + _, exists := pm.processes[pid2] assert.False(t, exists, "PID %d is in the list but shouldn't", pid2) } diff --git a/routers/admin/admin.go b/routers/admin/admin.go index 4bc6f8a9285f..9f155ff008be 100644 --- a/routers/admin/admin.go +++ b/routers/admin/admin.go @@ -352,7 +352,7 @@ func Monitor(ctx *context.Context) { ctx.Data["Title"] = ctx.Tr("admin.monitor") ctx.Data["PageIsAdmin"] = true ctx.Data["PageIsAdminMonitor"] = true - ctx.Data["Processes"] = process.GetManager().Processes + ctx.Data["Processes"] = process.GetManager().Processes() ctx.Data["Entries"] = cron.ListTasks() ctx.HTML(200, tplMonitor) } From 84760e2264d30327838eaeeaa2002839f3bd2fe8 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sat, 30 Nov 2019 07:30:42 -0600 Subject: [PATCH 09/11] Remove cmd from the process manager as it is no longer used --- modules/git/blame.go | 2 +- modules/git/command.go | 2 +- modules/process/manager.go | 6 ++---- modules/process/manager_test.go | 11 +++++------ routers/repo/http.go | 2 +- services/gitdiff/gitdiff.go | 4 ++-- 6 files changed, 12 insertions(+), 15 deletions(-) diff --git a/modules/git/blame.go b/modules/git/blame.go index e963a559b4a6..5a9ae9a74f96 100644 --- a/modules/git/blame.go +++ b/modules/git/blame.go @@ -117,7 +117,7 @@ func createBlameReader(dir string, command ...string) (*BlameReader, error) { return nil, fmt.Errorf("Start: %v", err) } - pid := process.GetManager().Add(fmt.Sprintf("GetBlame [repo_path: %s]", dir), cmd, cancel) + pid := process.GetManager().Add(fmt.Sprintf("GetBlame [repo_path: %s]", dir), cancel) scanner := bufio.NewScanner(stdout) diff --git a/modules/git/command.go b/modules/git/command.go index ad77aabb6bf4..f01db2e1d885 100644 --- a/modules/git/command.go +++ b/modules/git/command.go @@ -131,7 +131,7 @@ func (c *Command) RunInDirTimeoutEnvFullPipelineFunc(env []string, timeout time. if desc == "" { desc = fmt.Sprintf("%s %s %s [repo_path: %s]", GitExecutable, c.name, strings.Join(c.args, " "), dir) } - pid := process.GetManager().Add(desc, cmd, cancel) + pid := process.GetManager().Add(desc, cancel) defer process.GetManager().Remove(pid) if fn != nil { diff --git a/modules/process/manager.go b/modules/process/manager.go index 3daffa0272f7..af6ee9b81d49 100644 --- a/modules/process/manager.go +++ b/modules/process/manager.go @@ -35,7 +35,6 @@ type Process struct { PID int64 // Process ID, not system one. Description string Start time.Time - Cmd *exec.Cmd Cancel context.CancelFunc } @@ -58,14 +57,13 @@ func GetManager() *Manager { } // Add a process to the ProcessManager and returns its PID. -func (pm *Manager) Add(description string, cmd *exec.Cmd, cancel context.CancelFunc) int64 { +func (pm *Manager) Add(description string, cancel context.CancelFunc) int64 { pm.mutex.Lock() pid := pm.counter + 1 pm.processes[pid] = &Process{ PID: pid, Description: description, Start: time.Now(), - Cmd: cmd, Cancel: cancel, } pm.counter = pid @@ -154,7 +152,7 @@ func (pm *Manager) ExecDirEnvStdIn(timeout time.Duration, dir, desc string, env return "", "", err } - pid := pm.Add(desc, cmd, cancel) + pid := pm.Add(desc, cancel) err := cmd.Wait() pm.Remove(pid) diff --git a/modules/process/manager_test.go b/modules/process/manager_test.go index 8db9010dfd63..b18f76f94418 100644 --- a/modules/process/manager_test.go +++ b/modules/process/manager_test.go @@ -2,7 +2,6 @@ package process import ( "context" - "os/exec" "testing" "time" @@ -12,10 +11,10 @@ import ( func TestManager_Add(t *testing.T) { pm := Manager{processes: make(map[int64]*Process)} - pid := pm.Add("foo", exec.Command("foo"), nil) + pid := pm.Add("foo", nil) assert.Equal(t, int64(1), pid, "expected to get pid 1 got %d", pid) - pid = pm.Add("bar", exec.Command("bar"), nil) + pid = pm.Add("bar", nil) assert.Equal(t, int64(2), pid, "expected to get pid 2 got %d", pid) } @@ -23,7 +22,7 @@ func TestManager_Cancel(t *testing.T) { pm := Manager{processes: make(map[int64]*Process)} ctx, cancel := context.WithCancel(context.Background()) - pid := pm.Add("foo", exec.Command("foo"), cancel) + pid := pm.Add("foo", cancel) pm.Cancel(pid) @@ -37,10 +36,10 @@ func TestManager_Cancel(t *testing.T) { func TestManager_Remove(t *testing.T) { pm := Manager{processes: make(map[int64]*Process)} - pid1 := pm.Add("foo", exec.Command("foo"), nil) + pid1 := pm.Add("foo", nil) assert.Equal(t, int64(1), pid1, "expected to get pid 1 got %d", pid1) - pid2 := pm.Add("bar", exec.Command("bar"), nil) + pid2 := pm.Add("bar", nil) assert.Equal(t, int64(2), pid2, "expected to get pid 2 got %d", pid2) pm.Remove(pid2) diff --git a/routers/repo/http.go b/routers/repo/http.go index b67cc9c3bd57..716ec154b761 100644 --- a/routers/repo/http.go +++ b/routers/repo/http.go @@ -478,7 +478,7 @@ func serviceRPC(h serviceHandler, service string) { cmd.Stdin = reqBody cmd.Stderr = &stderr - pid := process.GetManager().Add(fmt.Sprintf("%s %s %s [repo_path: %s]", git.GitExecutable, service, "--stateless-rpc", h.dir), cmd, cancel) + pid := process.GetManager().Add(fmt.Sprintf("%s %s %s [repo_path: %s]", git.GitExecutable, service, "--stateless-rpc", h.dir), cancel) defer process.GetManager().Remove(pid) if err := cmd.Run(); err != nil { diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 3534dd756760..b8efa8a43f6d 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -859,7 +859,7 @@ func GetDiffRangeWithWhitespaceBehavior(repoPath, beforeCommitID, afterCommitID return nil, fmt.Errorf("Start: %v", err) } - pid := process.GetManager().Add(fmt.Sprintf("GetDiffRange [repo_path: %s]", repoPath), cmd, cancel) + pid := process.GetManager().Add(fmt.Sprintf("GetDiffRange [repo_path: %s]", repoPath), cancel) defer process.GetManager().Remove(pid) diff, err := ParsePatch(maxLines, maxLineCharacters, maxFiles, stdout) @@ -947,7 +947,7 @@ func GetRawDiffForFile(repoPath, startCommit, endCommit string, diffType RawDiff cmd.Dir = repoPath cmd.Stdout = writer cmd.Stderr = stderr - pid := process.GetManager().Add(fmt.Sprintf("GetRawDiffForFile: [repo_path: %s]", repoPath), cmd, cancel) + pid := process.GetManager().Add(fmt.Sprintf("GetRawDiffForFile: [repo_path: %s]", repoPath), cancel) defer process.GetManager().Remove(pid) if err = cmd.Run(); err != nil { From 05063e647615c4a016a1d1ff976b13ddea2fa02a Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sat, 30 Nov 2019 07:31:26 -0600 Subject: [PATCH 10/11] the default context is the correct context --- routers/repo/http.go | 1 - 1 file changed, 1 deletion(-) diff --git a/routers/repo/http.go b/routers/repo/http.go index 716ec154b761..c66d7aae6572 100644 --- a/routers/repo/http.go +++ b/routers/repo/http.go @@ -465,7 +465,6 @@ func serviceRPC(h serviceHandler, service string) { // set this for allow pre-receive and post-receive execute h.environ = append(h.environ, "SSH_ORIGINAL_COMMAND="+service) - // FIXME: graceful: Is the defaultContext appropriate here or should it be the requests context? ctx, cancel := gocontext.WithCancel(git.DefaultContext) defer cancel() var stderr bytes.Buffer From fd804686fe7021568ab74bef66ab5d6941de27f3 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sat, 30 Nov 2019 07:33:41 -0600 Subject: [PATCH 11/11] get rid of double till --- modules/graceful/manager.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/graceful/manager.go b/modules/graceful/manager.go index c6d96a2146a2..b9a56ca9c6be 100644 --- a/modules/graceful/manager.go +++ b/modules/graceful/manager.go @@ -50,7 +50,7 @@ type CallbackWithContext func(ctx context.Context, callback func()) // RunnableWithShutdownFns is a runnable with functions to run at shutdown and terminate // After the callback to atShutdown is called and is complete, the main function must return. // Similarly the callback function provided to atTerminate must return once termination is complete. -// Please note that use of the atShutdown and atTerminate callbacks will create go-routines that will wait till till their respective signals +// Please note that use of the atShutdown and atTerminate callbacks will create go-routines that will wait till their respective signals // - users must therefore be careful to only call these as necessary. // If run is not expected to run indefinitely RunWithShutdownChan is likely to be more appropriate. type RunnableWithShutdownFns func(atShutdown, atTerminate func(context.Context, func())) @@ -58,7 +58,7 @@ type RunnableWithShutdownFns func(atShutdown, atTerminate func(context.Context, // RunWithShutdownFns takes a function that has both atShutdown and atTerminate callbacks // After the callback to atShutdown is called and is complete, the main function must return. // Similarly the callback function provided to atTerminate must return once termination is complete. -// Please note that use of the atShutdown and atTerminate callbacks will create go-routines that will wait till till their respective signals +// Please note that use of the atShutdown and atTerminate callbacks will create go-routines that will wait till their respective signals // - users must therefore be careful to only call these as necessary. // If run is not expected to run indefinitely RunWithShutdownChan is likely to be more appropriate. func (g *gracefulManager) RunWithShutdownFns(run RunnableWithShutdownFns) {