From 3a7e68293abef5a25d31df14c179e7871f8cd15b Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sat, 18 Sep 2021 02:16:45 +0100 Subject: [PATCH 01/26] Move process to create contexts Signed-off-by: Andrew Thornton --- modules/cron/tasks.go | 7 +- modules/git/blame.go | 10 +-- modules/git/command.go | 14 ++-- modules/git/diff.go | 4 +- modules/markup/external/external.go | 6 +- modules/process/context.go | 67 ++++++++++++++++++ modules/process/manager.go | 105 ++++++++++++++++++++++++---- modules/process/manager_test.go | 49 ++++++++----- modules/task/migrate.go | 7 +- routers/web/repo/http.go | 8 +-- services/gitdiff/gitdiff.go | 6 +- services/mailer/mailer.go | 8 +-- 12 files changed, 214 insertions(+), 77 deletions(-) create mode 100644 modules/process/context.go diff --git a/modules/cron/tasks.go b/modules/cron/tasks.go index 6f3a96fd3f85..1d2b896afcc2 100644 --- a/modules/cron/tasks.go +++ b/modules/cron/tasks.go @@ -80,11 +80,10 @@ func (t *Task) RunWithUser(doer *models.User, config Config) { } }() graceful.GetManager().RunWithShutdownContext(func(baseCtx context.Context) { - ctx, cancel := context.WithCancel(baseCtx) - defer cancel() pm := process.GetManager() - pid := pm.Add(config.FormatMessage(t.Name, "process", doer), cancel) - defer pm.Remove(pid) + ctx, cancel := pm.AddContext(baseCtx, config.FormatMessage(t.Name, "process", doer)) + defer cancel() + if err := t.fun(ctx, doer, config); err != nil { if models.IsErrCancelled(err) { message := err.(models.ErrCancelled).Message diff --git a/modules/git/blame.go b/modules/git/blame.go index fcbf183981c3..42597af8dca2 100644 --- a/modules/git/blame.go +++ b/modules/git/blame.go @@ -25,7 +25,6 @@ type BlamePart struct { // BlameReader returns part of file blame one by one type BlameReader struct { cmd *exec.Cmd - pid int64 output io.ReadCloser reader *bufio.Reader lastSha *string @@ -100,8 +99,7 @@ func (r *BlameReader) NextPart() (*BlamePart, error) { // Close BlameReader - don't run NextPart after invoking that func (r *BlameReader) Close() error { - defer process.GetManager().Remove(r.pid) - r.cancel() + defer r.cancel() _ = r.output.Close() @@ -125,7 +123,8 @@ func CreateBlameReader(ctx context.Context, repoPath, commitID, file string) (*B func createBlameReader(ctx context.Context, dir string, command ...string) (*BlameReader, error) { // Here we use the provided context - this should be tied to the request performing the blame so that it does not hang around. - ctx, cancel := context.WithCancel(ctx) + ctx, cancel := process.GetManager().AddContext(ctx, fmt.Sprintf("GetBlame [repo_path: %s]", dir)) + cmd := exec.CommandContext(ctx, command[0], command[1:]...) cmd.Dir = dir cmd.Stderr = os.Stderr @@ -141,13 +140,10 @@ func createBlameReader(ctx context.Context, dir string, command ...string) (*Bla return nil, fmt.Errorf("Start: %v", err) } - pid := process.GetManager().Add(fmt.Sprintf("GetBlame [repo_path: %s]", dir), cancel) - reader := bufio.NewReader(stdout) return &BlameReader{ cmd, - pid, stdout, reader, nil, diff --git a/modules/git/command.go b/modules/git/command.go index e7496f072cb1..6bfa01dc21c1 100644 --- a/modules/git/command.go +++ b/modules/git/command.go @@ -143,7 +143,12 @@ func (c *Command) RunWithContext(rc *RunContext) error { log.Debug("%s: %v", rc.Dir, c) } - ctx, cancel := context.WithTimeout(c.parentContext, rc.Timeout) + desc := c.desc + if desc == "" { + desc = fmt.Sprintf("%s %s [repo_path: %s]", c.name, strings.Join(c.args, " "), rc.Dir) + } + + ctx, cancel := process.GetManager().AddContextTimeout(c.parentContext, rc.Timeout, desc) defer cancel() cmd := exec.CommandContext(ctx, c.name, c.args...) @@ -172,13 +177,6 @@ func (c *Command) RunWithContext(rc *RunContext) error { return err } - desc := c.desc - if desc == "" { - desc = fmt.Sprintf("%s %s %s [repo_path: %s]", GitExecutable, c.name, strings.Join(c.args, " "), rc.Dir) - } - pid := process.GetManager().Add(desc, cancel) - defer process.GetManager().Remove(pid) - if rc.PipelineFunc != nil { err := rc.PipelineFunc(ctx, cancel) if err != nil { diff --git a/modules/git/diff.go b/modules/git/diff.go index b473dc73f60e..c8bb4fe07056 100644 --- a/modules/git/diff.go +++ b/modules/git/diff.go @@ -56,7 +56,7 @@ func GetRepoRawDiffForFile(repo *Repository, startCommit, endCommit string, diff fileArgs = append(fileArgs, "--", file) } // FIXME: graceful: These commands should have a timeout - ctx, cancel := context.WithCancel(DefaultContext) + ctx, cancel := process.GetManager().AddContext(DefaultContext, fmt.Sprintf("GetRawDiffForFile: [repo_path: %s]", repo.Path)) defer cancel() var cmd *exec.Cmd @@ -90,8 +90,6 @@ func GetRepoRawDiffForFile(repo *Repository, startCommit, endCommit string, diff cmd.Dir = repo.Path cmd.Stdout = writer cmd.Stderr = stderr - pid := process.GetManager().Add(fmt.Sprintf("GetRawDiffForFile: [repo_path: %s]", repo.Path), cancel) - defer process.GetManager().Remove(pid) if err = cmd.Run(); err != nil { return fmt.Errorf("Run: %v - %s", err, stderr) diff --git a/modules/markup/external/external.go b/modules/markup/external/external.go index 36cbd69f92c9..875e79fa3353 100644 --- a/modules/markup/external/external.go +++ b/modules/markup/external/external.go @@ -5,7 +5,6 @@ package external import ( - "context" "fmt" "io" "os" @@ -107,12 +106,9 @@ func (p *Renderer) Render(ctx *markup.RenderContext, input io.Reader, output io. ctx.Ctx = graceful.GetManager().ShutdownContext() } - processCtx, cancel := context.WithCancel(ctx.Ctx) + processCtx, cancel := process.GetManager().AddContext(ctx.Ctx, fmt.Sprintf("Render [%s] for %s", commands[0], ctx.URLPrefix)) defer cancel() - pid := process.GetManager().Add(fmt.Sprintf("Render [%s] for %s", commands[0], ctx.URLPrefix), cancel) - defer process.GetManager().Remove(pid) - cmd := exec.CommandContext(processCtx, commands[0], args...) cmd.Env = append( os.Environ(), diff --git a/modules/process/context.go b/modules/process/context.go new file mode 100644 index 000000000000..f8e4cec7cdd3 --- /dev/null +++ b/modules/process/context.go @@ -0,0 +1,67 @@ +// Copyright 2021 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 process + +import ( + "context" +) + +// Context is a wrapper around context.Context for having the current pid for this context +type Context struct { + context.Context + pid int64 +} + +// GetPID returns the PID for this context +func (c *Context) GetPID() int64 { + return c.pid +} + +// GetParent returns the parent process context if any +func (c *Context) GetParent() *Context { + return GetContext(c.Context) +} + +func (c *Context) Value(key interface{}) interface{} { + if key == ProcessContextKey { + return c + } + return c.Context.Value(key) +} + +// ProcessContextKey is the key under which process contexts are stored +var ProcessContextKey interface{} = "process-context" + +// GetContext will return a process context if one exists +func GetContext(ctx context.Context) *Context { + if pCtx, ok := ctx.(*Context); ok { + return pCtx + } + pCtxInterface := ctx.Value(ProcessContextKey) + if pCtxInterface == nil { + return nil + } + if pCtx, ok := pCtxInterface.(*Context); ok { + return pCtx + } + return nil +} + +// GetPID returns the PID for this context +func GetPID(ctx context.Context) int64 { + pCtx := GetContext(ctx) + if pCtx == nil { + return 0 + } + return pCtx.GetPID() +} + +func GetParentPID(ctx context.Context) int64 { + parentPID := int64(0) + if parentProcess := GetContext(ctx); parentProcess != nil { + parentPID = parentProcess.GetPID() + } + return parentPID +} diff --git a/modules/process/manager.go b/modules/process/manager.go index e42e38a0f02a..2bf6fee6f8a8 100644 --- a/modules/process/manager.go +++ b/modules/process/manager.go @@ -26,11 +26,18 @@ var ( // DefaultContext is the default context to run processing commands in DefaultContext = context.Background() + + // RecyclePID is the PID number at which we will attempt to recycle PIDs + RecyclePID int64 = 1 << 16 + + // HuntSize is the size of the hunt for the lowest free PID + HuntSize int64 = 512 ) // Process represents a working process inheriting from Gitea. type Process struct { PID int64 // Process ID, not system one. + ParentPID int64 Description string Start time.Time Cancel context.CancelFunc @@ -40,7 +47,9 @@ type Process struct { type Manager struct { mutex sync.Mutex - counter int64 + next int64 + low int64 + processes map[int64]*Process } @@ -49,31 +58,105 @@ func GetManager() *Manager { managerInit.Do(func() { manager = &Manager{ processes: make(map[int64]*Process), + next: 1, + low: 1, } }) return manager } -// Add a process to the ProcessManager and returns its PID. -func (pm *Manager) Add(description string, cancel context.CancelFunc) int64 { +// AddContext create a new context and add it as a process +func (pm *Manager) AddContext(parent context.Context, description string) (context.Context, context.CancelFunc) { + parentPID := GetParentPID(parent) + + ctx, cancel := context.WithCancel(parent) + + pid, cancel := pm.Add(parentPID, description, cancel) + + return &Context{ + Context: ctx, + pid: pid, + }, cancel +} + +// AddContextTimeout create a new context and add it as a process +func (pm *Manager) AddContextTimeout(parent context.Context, timeout time.Duration, description string) (context.Context, context.CancelFunc) { + parentPID := GetParentPID(parent) + + ctx, cancel := context.WithTimeout(parent, timeout) + + pid, cancel := pm.Add(parentPID, description, cancel) + + return &Context{ + Context: ctx, + pid: pid, + }, cancel +} + +// Add create a new process +func (pm *Manager) Add(parentPID int64, description string, cancel context.CancelFunc) (int64, context.CancelFunc) { pm.mutex.Lock() - pid := pm.counter + 1 - pm.processes[pid] = &Process{ + pid := pm.nextPID() + process := &Process{ PID: pid, + ParentPID: parentPID, Description: description, Start: time.Now(), - Cancel: cancel, } - pm.counter = pid + + process.Cancel = func() { + cancel() + pm.remove(process) + } + + pm.processes[pid] = process pm.mutex.Unlock() - return pid + return pid, process.Cancel +} + +// nextPID will return the next available PID. pm.mutex should already be locked. +func (pm *Manager) nextPID() int64 { + if pm.next > RecyclePID { + for i := int64(0); i < HuntSize; i++ { + if pm.low >= pm.next { + pm.low = 1 + break + } + if _, ok := pm.processes[pm.low]; !ok { + next := pm.low + pm.low++ + return next + } + pm.low++ + } + } + next := pm.next + pm.next++ + return next +} + +// releasePID will release the PID. pm.mutex should already be locked. +func (pm *Manager) releasePID(pid int64) { + if pid < pm.low+RecyclePID { + pm.low = pid + } } // Remove a process from the ProcessManager. func (pm *Manager) Remove(pid int64) { pm.mutex.Lock() delete(pm.processes, pid) + pm.releasePID(pid) + pm.mutex.Unlock() +} + +func (pm *Manager) remove(process *Process) { + pm.mutex.Lock() + if p := pm.processes[process.PID]; p == process { + delete(pm.processes, process.PID) + pm.releasePID(process.PID) + } pm.mutex.Unlock() } @@ -134,7 +217,7 @@ func (pm *Manager) ExecDirEnvStdIn(timeout time.Duration, dir, desc string, env stdOut := new(bytes.Buffer) stdErr := new(bytes.Buffer) - ctx, cancel := context.WithTimeout(DefaultContext, timeout) + ctx, cancel := pm.AddContextTimeout(DefaultContext, timeout, desc) defer cancel() cmd := exec.CommandContext(ctx, cmdName, args...) @@ -150,13 +233,11 @@ func (pm *Manager) ExecDirEnvStdIn(timeout time.Duration, dir, desc string, env return "", "", err } - pid := pm.Add(desc, cancel) err := cmd.Wait() - pm.Remove(pid) if err != nil { err = &Error{ - PID: pid, + PID: GetPID(ctx), Description: desc, Err: err, CtxErr: ctx.Err(), diff --git a/modules/process/manager_test.go b/modules/process/manager_test.go index a515fc32cda7..96a2744b4b56 100644 --- a/modules/process/manager_test.go +++ b/modules/process/manager_test.go @@ -21,23 +21,30 @@ func TestGetManager(t *testing.T) { assert.NotNil(t, pm) } -func TestManager_Add(t *testing.T) { - pm := Manager{processes: make(map[int64]*Process)} +func TestManager_AddContext(t *testing.T) { + pm := Manager{processes: make(map[int64]*Process), next: 1, low: 1} - pid := pm.Add("foo", nil) - assert.Equal(t, int64(1), pid, "expected to get pid 1 got %d", pid) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + p1Ctx, cancel := pm.AddContext(ctx, "foo") + defer cancel() + assert.Equal(t, int64(1), GetContext(p1Ctx).GetPID(), "expected to get pid 1 got %d", GetContext(p1Ctx).GetPID()) - pid = pm.Add("bar", nil) - assert.Equal(t, int64(2), pid, "expected to get pid 2 got %d", pid) + p2Ctx, cancel := pm.AddContext(p1Ctx, "bar") + defer cancel() + + assert.Equal(t, int64(2), GetContext(p2Ctx).GetPID(), "expected to get pid 2 got %d", GetContext(p2Ctx).GetPID()) + assert.Equal(t, int64(1), GetContext(p2Ctx).GetParent().GetPID(), "expected to get pid 1 got %d", GetContext(p2Ctx).GetParent().GetPID()) } func TestManager_Cancel(t *testing.T) { - pm := Manager{processes: make(map[int64]*Process)} + pm := Manager{processes: make(map[int64]*Process), next: 1, low: 1} - ctx, cancel := context.WithCancel(context.Background()) - pid := pm.Add("foo", cancel) + ctx, cancel := pm.AddContext(context.Background(), "foo") + defer cancel() - pm.Cancel(pid) + pm.Cancel(GetPID(ctx)) select { case <-ctx.Done(): @@ -47,18 +54,24 @@ 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), next: 1, low: 1} + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + p1Ctx, cancel := pm.AddContext(ctx, "foo") + defer cancel() + assert.Equal(t, int64(1), GetContext(p1Ctx).GetPID(), "expected to get pid 1 got %d", GetContext(p1Ctx).GetPID()) - pid1 := pm.Add("foo", nil) - assert.Equal(t, int64(1), pid1, "expected to get pid 1 got %d", pid1) + p2Ctx, cancel := pm.AddContext(p1Ctx, "bar") + defer cancel() - pid2 := pm.Add("bar", nil) - assert.Equal(t, int64(2), pid2, "expected to get pid 2 got %d", pid2) + assert.Equal(t, int64(2), GetContext(p2Ctx).GetPID(), "expected to get pid 2 got %d", GetContext(p2Ctx).GetPID()) - pm.Remove(pid2) + pm.Remove(GetPID(p2Ctx)) - _, exists := pm.processes[pid2] - assert.False(t, exists, "PID %d is in the list but shouldn't", pid2) + _, exists := pm.processes[GetPID(p2Ctx)] + assert.False(t, exists, "PID %d is in the list but shouldn't", GetPID(p2Ctx)) } func TestExecTimeoutNever(t *testing.T) { diff --git a/modules/task/migrate.go b/modules/task/migrate.go index 52f4bb91cf72..6a7993e30d79 100644 --- a/modules/task/migrate.go +++ b/modules/task/migrate.go @@ -5,7 +5,6 @@ package task import ( - "context" "errors" "fmt" "strings" @@ -94,11 +93,9 @@ func runMigrateTask(t *models.Task) (err error) { opts.MigrateToRepoID = t.RepoID - ctx, cancel := context.WithCancel(graceful.GetManager().ShutdownContext()) - defer cancel() pm := process.GetManager() - pid := pm.Add(fmt.Sprintf("MigrateTask: %s/%s", t.Owner.Name, opts.RepoName), cancel) - defer pm.Remove(pid) + ctx, cancel := pm.AddContext(graceful.GetManager().ShutdownContext(), fmt.Sprintf("MigrateTask: %s/%s", t.Owner.Name, opts.RepoName)) + defer cancel() t.StartTime = timeutil.TimeStampNow() t.Status = structs.TaskStatusRunning diff --git a/routers/web/repo/http.go b/routers/web/repo/http.go index fbd1e19a8219..5391080123b6 100644 --- a/routers/web/repo/http.go +++ b/routers/web/repo/http.go @@ -8,7 +8,6 @@ package repo import ( "bytes" "compress/gzip" - gocontext "context" "fmt" "net/http" "os" @@ -481,8 +480,10 @@ func serviceRPC(h serviceHandler, service string) { h.environ = append(h.environ, "GIT_PROTOCOL="+protocol) } - ctx, cancel := gocontext.WithCancel(git.DefaultContext) + // ctx, cancel := gocontext.WithCancel(git.DefaultContext) + ctx, cancel := process.GetManager().AddContext(git.DefaultContext, fmt.Sprintf("%s %s %s [repo_path: %s]", git.GitExecutable, service, "--stateless-rpc", h.dir)) defer cancel() + var stderr bytes.Buffer cmd := exec.CommandContext(ctx, git.GitExecutable, service, "--stateless-rpc", h.dir) cmd.Dir = h.dir @@ -491,9 +492,6 @@ 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), cancel) - defer process.GetManager().Remove(pid) - if err := cmd.Run(); err != nil { log.Error("Fail to serve RPC(%s) in %s: %v - %s", service, h.dir, err, stderr.String()) return diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index ac5e947d158c..e0d975ffb383 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -1225,7 +1225,8 @@ func GetDiffRangeWithWhitespaceBehavior(gitRepo *git.Repository, beforeCommitID, return nil, err } - ctx, cancel := context.WithTimeout(git.DefaultContext, time.Duration(setting.Git.Timeout.Default)*time.Second) + timeout := time.Duration(setting.Git.Timeout.Default) * time.Second + ctx, cancel := process.GetManager().AddContextTimeout(git.DefaultContext, timeout, fmt.Sprintf("GetDiffRange [repo_path: %s]", repoPath)) defer cancel() var cmd *exec.Cmd @@ -1265,9 +1266,6 @@ func GetDiffRangeWithWhitespaceBehavior(gitRepo *git.Repository, beforeCommitID, return nil, fmt.Errorf("Start: %v", err) } - pid := process.GetManager().Add(fmt.Sprintf("GetDiffRange [repo_path: %s]", repoPath), cancel) - defer process.GetManager().Remove(pid) - diff, err := ParsePatch(maxLines, maxLineCharacters, maxFiles, stdout) if err != nil { return nil, fmt.Errorf("ParsePatch: %v", err) diff --git a/services/mailer/mailer.go b/services/mailer/mailer.go index fae8d473e341..c9d5d254f177 100644 --- a/services/mailer/mailer.go +++ b/services/mailer/mailer.go @@ -7,7 +7,6 @@ package mailer import ( "bytes" - "context" "crypto/tls" "fmt" "io" @@ -247,10 +246,9 @@ func (s *sendmailSender) Send(from string, to []string, msg io.WriterTo) error { args = append(args, to...) log.Trace("Sending with: %s %v", setting.MailService.SendmailPath, args) - pm := process.GetManager() desc := fmt.Sprintf("SendMail: %s %v", setting.MailService.SendmailPath, args) - ctx, cancel := context.WithTimeout(graceful.GetManager().HammerContext(), setting.MailService.SendmailTimeout) + ctx, cancel := process.GetManager().AddContextTimeout(graceful.GetManager().HammerContext(), setting.MailService.SendmailTimeout, desc) defer cancel() cmd := exec.CommandContext(ctx, setting.MailService.SendmailPath, args...) @@ -264,15 +262,13 @@ func (s *sendmailSender) Send(from string, to []string, msg io.WriterTo) error { return err } - pid := pm.Add(desc, cancel) - _, err = msg.WriteTo(pipe) // we MUST close the pipe or sendmail will hang waiting for more of the message // Also we should wait on our sendmail command even if something fails closeError = pipe.Close() waitError = cmd.Wait() - pm.Remove(pid) + if err != nil { return err } else if closeError != nil { From 47bda444d683492b5727a7013792aa070ffc3a2b Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sat, 18 Sep 2021 20:32:09 +0100 Subject: [PATCH 02/26] display children processes Signed-off-by: Andrew Thornton --- modules/process/manager.go | 55 ++++++++++++++++++++++++++++---- options/locale/locale_en-US.ini | 1 + routers/web/admin/admin.go | 2 +- templates/admin/monitor.tmpl | 28 +--------------- templates/admin/process-row.tmpl | 19 +++++++++++ templates/admin/process.tmpl | 10 ++++++ 6 files changed, 81 insertions(+), 34 deletions(-) create mode 100644 templates/admin/process-row.tmpl create mode 100644 templates/admin/process.tmpl diff --git a/modules/process/manager.go b/modules/process/manager.go index 2bf6fee6f8a8..b3be1a64f570 100644 --- a/modules/process/manager.go +++ b/modules/process/manager.go @@ -38,6 +38,7 @@ var ( type Process struct { PID int64 // Process ID, not system one. ParentPID int64 + Children []*Process Description string Start time.Time Cancel context.CancelFunc @@ -65,7 +66,7 @@ func GetManager() *Manager { return manager } -// AddContext create a new context and add it as a process +// AddContext create a new context and add it as a process. The CancelFunc must always be called even if the context is Done() func (pm *Manager) AddContext(parent context.Context, description string) (context.Context, context.CancelFunc) { parentPID := GetParentPID(parent) @@ -97,6 +98,12 @@ func (pm *Manager) AddContextTimeout(parent context.Context, timeout time.Durati func (pm *Manager) Add(parentPID int64, description string, cancel context.CancelFunc) (int64, context.CancelFunc) { pm.mutex.Lock() pid := pm.nextPID() + + parent := pm.processes[parentPID] + if parent == nil { + parentPID = 0 + } + process := &Process{ PID: pid, ParentPID: parentPID, @@ -109,6 +116,9 @@ func (pm *Manager) Add(parentPID int64, description string, cancel context.Cance pm.remove(process) } + if parent != nil { + parent.Children = append(parent.Children, process) + } pm.processes[pid] = process pm.mutex.Unlock() @@ -153,11 +163,23 @@ func (pm *Manager) Remove(pid int64) { func (pm *Manager) remove(process *Process) { pm.mutex.Lock() + defer pm.mutex.Unlock() if p := pm.processes[process.PID]; p == process { delete(pm.processes, process.PID) pm.releasePID(process.PID) + for _, child := range process.Children { + child.ParentPID = 0 + } + parent := pm.processes[process.ParentPID] + if parent != nil { + for i, child := range parent.Children { + if child == process { + parent.Children = append(parent.Children[:i], parent.Children[i+1:]...) + return + } + } + } } - pm.mutex.Unlock() } // Cancel a process in the ProcessManager. @@ -171,13 +193,28 @@ func (pm *Manager) Cancel(pid int64) { } // Processes gets the processes in a thread safe manner -func (pm *Manager) Processes() []*Process { +func (pm *Manager) Processes(onlyRoots bool) []*Process { pm.mutex.Lock() processes := make([]*Process, 0, len(pm.processes)) - for _, process := range pm.processes { - processes = append(processes, process) + if onlyRoots { + for _, process := range pm.processes { + if process.ParentPID == 0 { + processes = append(processes, process) + } + } + } else { + for _, process := range pm.processes { + processes = append(processes, process) + } } pm.mutex.Unlock() + + sort.Slice(processes, func(i, j int) bool { + left, right := processes[i], processes[j] + + return left.Start.Before(right.Start) + }) + sort.Sort(processList(processes)) return processes } @@ -256,7 +293,13 @@ func (l processList) Len() int { } func (l processList) Less(i, j int) bool { - return l[i].PID < l[j].PID + if l[i].ParentPID < l[j].ParentPID { + return true + } + if l[i].ParentPID == l[j].ParentPID { + return l[i].PID < l[j].PID + } + return false } func (l processList) Swap(i, j int) { diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index b38b249b2cf1..8355fd58c06c 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -2650,6 +2650,7 @@ 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? +monitor.process.children = Children monitor.queues = Queues monitor.queue = Queue: %s monitor.queue.name = Name diff --git a/routers/web/admin/admin.go b/routers/web/admin/admin.go index ce177ea09087..0b98987be51c 100644 --- a/routers/web/admin/admin.go +++ b/routers/web/admin/admin.go @@ -323,7 +323,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(true) ctx.Data["Entries"] = cron.ListTasks() ctx.Data["Queues"] = queue.GetManager().ManagedQueues() ctx.HTML(http.StatusOK, tplMonitor) diff --git a/templates/admin/monitor.tmpl b/templates/admin/monitor.tmpl index 16c4d88002a2..8a90f9b6474c 100644 --- a/templates/admin/monitor.tmpl +++ b/templates/admin/monitor.tmpl @@ -65,33 +65,7 @@ -

- {{.i18n.Tr "admin.monitor.process"}} -

-
- - - - - - - - - - - - {{range .Processes}} - - - - - - - - {{end}} - -
Pid{{.i18n.Tr "admin.monitor.desc"}}{{.i18n.Tr "admin.monitor.start"}}{{.i18n.Tr "admin.monitor.execute_time"}}
{{.PID}}{{.Description}}{{DateFmtLong .Start}}{{TimeSince .Start $.Lang}}{{svg "octicon-trash" 16 "text-red"}}
-
+ {{template "admin/process" .}}
diff --git a/templates/admin/process-row.tmpl b/templates/admin/process-row.tmpl new file mode 100644 index 000000000000..cba62e503d25 --- /dev/null +++ b/templates/admin/process-row.tmpl @@ -0,0 +1,19 @@ +
+
+
+
{{.Process.Description}}
+
{{TimeSince .Process.Start .root.Lang}}
+
+ +
+ + {{if .Process.Children}} +
+ {{range .Process.Children}} + {{template "admin/process-row" dict "Process" . "root" $.root}} + {{end}} +
+ {{end}} +
diff --git a/templates/admin/process.tmpl b/templates/admin/process.tmpl new file mode 100644 index 000000000000..719c10cead3d --- /dev/null +++ b/templates/admin/process.tmpl @@ -0,0 +1,10 @@ +

+ {{.i18n.Tr "admin.monitor.process"}} +

+
+
+ {{range .Processes}} + {{template "admin/process-row" dict "Process" . "root" $}} + {{end}} +
+
From cd16cbbfc724f5f946055c565e7af128ddc5325d Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sat, 18 Sep 2021 02:58:01 +0100 Subject: [PATCH 03/26] Make requests a process Signed-off-by: Andrew Thornton --- routers/common/middleware.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/routers/common/middleware.go b/routers/common/middleware.go index 1d96522dd9d1..e1eebe579ecc 100644 --- a/routers/common/middleware.go +++ b/routers/common/middleware.go @@ -11,6 +11,7 @@ import ( "code.gitea.io/gitea/modules/context" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/process" "code.gitea.io/gitea/modules/setting" "github.com/chi-middleware/proxy" @@ -22,7 +23,9 @@ func Middlewares() []func(http.Handler) http.Handler { var handlers = []func(http.Handler) http.Handler{ func(next http.Handler) http.Handler { return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) { - next.ServeHTTP(context.NewResponse(resp), req) + ctx, cancel := process.GetManager().AddContext(req.Context(), fmt.Sprintf("%s: %s", req.Method, req.RequestURI)) + defer cancel() + next.ServeHTTP(context.NewResponse(resp), req.WithContext(ctx)) }) }, } From 12039b9dd7ba87da034746464c29bbab741d08ba Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sat, 18 Sep 2021 21:24:20 +0100 Subject: [PATCH 04/26] Add context to repo and add ctx to OpenRepository Signed-off-by: Andrew Thornton --- modules/context/repo.go | 4 ++-- modules/git/batch_reader.go | 8 ++++---- modules/git/blob_nogogit.go | 4 ++-- modules/git/commit_info_nogogit.go | 4 ++-- modules/git/pipeline/lfs_nogogit.go | 2 +- modules/git/repo_base_gogit.go | 9 +++++++++ modules/git/repo_base_nogogit.go | 20 ++++++++++++++------ modules/git/repo_branch_nogogit.go | 4 ++-- modules/git/repo_commit_nogogit.go | 6 +++--- modules/git/repo_language_stats_nogogit.go | 2 +- modules/git/repo_tree_nogogit.go | 2 +- modules/git/tree_nogogit.go | 2 +- modules/indexer/code/bleve.go | 2 +- modules/indexer/code/elastic_search.go | 2 +- 14 files changed, 44 insertions(+), 27 deletions(-) diff --git a/modules/context/repo.go b/modules/context/repo.go index eceefd9e5925..ca3fe90b7cb8 100644 --- a/modules/context/repo.go +++ b/modules/context/repo.go @@ -526,7 +526,7 @@ func RepoAssignment(ctx *Context) (cancel context.CancelFunc) { return } - gitRepo, err := git.OpenRepository(models.RepoPath(userName, repoName)) + gitRepo, err := git.OpenRepositoryCtx(ctx, models.RepoPath(userName, repoName)) if err != nil { ctx.ServerError("RepoAssignment Invalid repo "+models.RepoPath(userName, repoName), err) return @@ -742,7 +742,7 @@ func RepoRefByType(refType RepoRefType, ignoreNotExistErr ...bool) func(*Context if ctx.Repo.GitRepo == nil { repoPath := models.RepoPath(ctx.Repo.Owner.Name, ctx.Repo.Repository.Name) - ctx.Repo.GitRepo, err = git.OpenRepository(repoPath) + ctx.Repo.GitRepo, err = git.OpenRepositoryCtx(ctx, repoPath) if err != nil { ctx.ServerError("RepoRef Invalid repo "+repoPath, err) return diff --git a/modules/git/batch_reader.go b/modules/git/batch_reader.go index 8e3c23251b63..324b069512c7 100644 --- a/modules/git/batch_reader.go +++ b/modules/git/batch_reader.go @@ -28,10 +28,10 @@ type WriteCloserError interface { } // CatFileBatchCheck opens git cat-file --batch-check in the provided repo and returns a stdin pipe, a stdout reader and cancel function -func CatFileBatchCheck(repoPath string) (WriteCloserError, *bufio.Reader, func()) { +func CatFileBatchCheck(ctx context.Context, repoPath string) (WriteCloserError, *bufio.Reader, func()) { batchStdinReader, batchStdinWriter := io.Pipe() batchStdoutReader, batchStdoutWriter := io.Pipe() - ctx, ctxCancel := context.WithCancel(DefaultContext) + ctx, ctxCancel := context.WithCancel(ctx) closed := make(chan struct{}) cancel := func() { _ = batchStdinReader.Close() @@ -67,12 +67,12 @@ func CatFileBatchCheck(repoPath string) (WriteCloserError, *bufio.Reader, func() } // CatFileBatch opens git cat-file --batch in the provided repo and returns a stdin pipe, a stdout reader and cancel function -func CatFileBatch(repoPath string) (WriteCloserError, *bufio.Reader, func()) { +func CatFileBatch(ctx context.Context, repoPath string) (WriteCloserError, *bufio.Reader, func()) { // We often want to feed the commits in order into cat-file --batch, followed by their trees and sub trees as necessary. // so let's create a batch stdin and stdout batchStdinReader, batchStdinWriter := io.Pipe() batchStdoutReader, batchStdoutWriter := nio.Pipe(buffer.New(32 * 1024)) - ctx, ctxCancel := context.WithCancel(DefaultContext) + ctx, ctxCancel := context.WithCancel(ctx) closed := make(chan struct{}) cancel := func() { _ = batchStdinReader.Close() diff --git a/modules/git/blob_nogogit.go b/modules/git/blob_nogogit.go index 65a73ebc522f..aabf1b34aded 100644 --- a/modules/git/blob_nogogit.go +++ b/modules/git/blob_nogogit.go @@ -29,7 +29,7 @@ type Blob struct { // DataAsync gets a ReadCloser for the contents of a blob without reading it all. // Calling the Close function on the result will discard all unread output. func (b *Blob) DataAsync() (io.ReadCloser, error) { - wr, rd, cancel := b.repo.CatFileBatch() + wr, rd, cancel := b.repo.CatFileBatch(b.repo.Ctx) _, err := wr.Write([]byte(b.ID.String() + "\n")) if err != nil { @@ -67,7 +67,7 @@ func (b *Blob) Size() int64 { return b.size } - wr, rd, cancel := b.repo.CatFileBatchCheck() + wr, rd, cancel := b.repo.CatFileBatchCheck(b.repo.Ctx) defer cancel() _, err := wr.Write([]byte(b.ID.String() + "\n")) if err != nil { diff --git a/modules/git/commit_info_nogogit.go b/modules/git/commit_info_nogogit.go index f57355d16ed4..3c25d0369b90 100644 --- a/modules/git/commit_info_nogogit.go +++ b/modules/git/commit_info_nogogit.go @@ -99,7 +99,7 @@ func (tes Entries) GetCommitsInfo(ctx context.Context, commit *Commit, treePath } func getLastCommitForPathsByCache(ctx context.Context, commitID, treePath string, paths []string, cache *LastCommitCache) (map[string]*Commit, []string, error) { - wr, rd, cancel := cache.repo.CatFileBatch() + wr, rd, cancel := cache.repo.CatFileBatch(ctx) defer cancel() var unHitEntryPaths []string @@ -128,7 +128,7 @@ func GetLastCommitForPaths(ctx context.Context, commit *Commit, treePath string, return nil, err } - batchStdinWriter, batchReader, cancel := commit.repo.CatFileBatch() + batchStdinWriter, batchReader, cancel := commit.repo.CatFileBatch(ctx) defer cancel() commitsMap := map[string]*Commit{} diff --git a/modules/git/pipeline/lfs_nogogit.go b/modules/git/pipeline/lfs_nogogit.go index a441e37b60e1..4db1a443118f 100644 --- a/modules/git/pipeline/lfs_nogogit.go +++ b/modules/git/pipeline/lfs_nogogit.go @@ -63,7 +63,7 @@ func FindLFSFile(repo *git.Repository, hash git.SHA1) ([]*LFSResult, error) { // Next feed the commits in order into cat-file --batch, followed by their trees and sub trees as necessary. // so let's create a batch stdin and stdout - batchStdinWriter, batchReader, cancel := repo.CatFileBatch() + batchStdinWriter, batchReader, cancel := repo.CatFileBatch(repo.Ctx) defer cancel() // We'll use a scanner for the revList because it's simpler than a bufio.Reader diff --git a/modules/git/repo_base_gogit.go b/modules/git/repo_base_gogit.go index afa5383b1164..d4aef56746ae 100644 --- a/modules/git/repo_base_gogit.go +++ b/modules/git/repo_base_gogit.go @@ -9,6 +9,7 @@ package git import ( + "context" "errors" "path/filepath" @@ -30,10 +31,17 @@ type Repository struct { gogitRepo *gogit.Repository gogitStorage *filesystem.Storage gpgSettings *GPGSettings + + ctx context.Context } // OpenRepository opens the repository at the given path. func OpenRepository(repoPath string) (*Repository, error) { + return OpenRepositoryCtx(DefaultContext, repoPath) +} + +// OpenRepositoryCtx opens the repository at the given path within the context.Context +func OpenRepositoryCtx(ctx context.Context, repoPath string) (*Repository, error) { repoPath, err := filepath.Abs(repoPath) if err != nil { return nil, err @@ -60,6 +68,7 @@ func OpenRepository(repoPath string) (*Repository, error) { gogitRepo: gogitRepo, gogitStorage: storage, tagCache: newObjectCache(), + ctx: ctx, }, nil } diff --git a/modules/git/repo_base_nogogit.go b/modules/git/repo_base_nogogit.go index 22c4dfdcb3b8..f307facd915f 100644 --- a/modules/git/repo_base_nogogit.go +++ b/modules/git/repo_base_nogogit.go @@ -32,10 +32,17 @@ type Repository struct { checkCancel context.CancelFunc checkReader *bufio.Reader checkWriter WriteCloserError + + Ctx context.Context } // OpenRepository opens the repository at the given path. func OpenRepository(repoPath string) (*Repository, error) { + return OpenRepositoryCtx(DefaultContext, repoPath) +} + +// OpenRepository opens the repository at the given path. +func OpenRepositoryCtx(ctx context.Context, repoPath string) (*Repository, error) { repoPath, err := filepath.Abs(repoPath) if err != nil { return nil, err @@ -46,28 +53,29 @@ func OpenRepository(repoPath string) (*Repository, error) { repo := &Repository{ Path: repoPath, tagCache: newObjectCache(), + Ctx: ctx, } - repo.batchWriter, repo.batchReader, repo.batchCancel = CatFileBatch(repoPath) - repo.checkWriter, repo.checkReader, repo.checkCancel = CatFileBatchCheck(repo.Path) + repo.batchWriter, repo.batchReader, repo.batchCancel = CatFileBatch(ctx, repoPath) + repo.checkWriter, repo.checkReader, repo.checkCancel = CatFileBatchCheck(ctx, repo.Path) return repo, nil } // CatFileBatch obtains a CatFileBatch for this repository -func (repo *Repository) CatFileBatch() (WriteCloserError, *bufio.Reader, func()) { +func (repo *Repository) CatFileBatch(ctx context.Context) (WriteCloserError, *bufio.Reader, func()) { if repo.batchCancel == nil || repo.batchReader.Buffered() > 0 { log.Debug("Opening temporary cat file batch for: %s", repo.Path) - return CatFileBatch(repo.Path) + return CatFileBatch(ctx, repo.Path) } return repo.batchWriter, repo.batchReader, func() {} } // CatFileBatchCheck obtains a CatFileBatchCheck for this repository -func (repo *Repository) CatFileBatchCheck() (WriteCloserError, *bufio.Reader, func()) { +func (repo *Repository) CatFileBatchCheck(ctx context.Context) (WriteCloserError, *bufio.Reader, func()) { if repo.checkCancel == nil || repo.checkReader.Buffered() > 0 { log.Debug("Opening temporary cat file batch-check: %s", repo.Path) - return CatFileBatchCheck(repo.Path) + return CatFileBatchCheck(ctx, repo.Path) } return repo.checkWriter, repo.checkReader, func() {} } diff --git a/modules/git/repo_branch_nogogit.go b/modules/git/repo_branch_nogogit.go index 666ca81c1e8f..22383201813b 100644 --- a/modules/git/repo_branch_nogogit.go +++ b/modules/git/repo_branch_nogogit.go @@ -23,7 +23,7 @@ func (repo *Repository) IsObjectExist(name string) bool { return false } - wr, rd, cancel := repo.CatFileBatchCheck() + wr, rd, cancel := repo.CatFileBatchCheck(repo.Ctx) defer cancel() _, err := wr.Write([]byte(name + "\n")) if err != nil { @@ -40,7 +40,7 @@ func (repo *Repository) IsReferenceExist(name string) bool { return false } - wr, rd, cancel := repo.CatFileBatchCheck() + wr, rd, cancel := repo.CatFileBatchCheck(repo.Ctx) defer cancel() _, err := wr.Write([]byte(name + "\n")) if err != nil { diff --git a/modules/git/repo_commit_nogogit.go b/modules/git/repo_commit_nogogit.go index 8bfc953759d5..88cf56845e90 100644 --- a/modules/git/repo_commit_nogogit.go +++ b/modules/git/repo_commit_nogogit.go @@ -35,7 +35,7 @@ func (repo *Repository) ResolveReference(name string) (string, error) { // GetRefCommitID returns the last commit ID string of given reference (branch or tag). func (repo *Repository) GetRefCommitID(name string) (string, error) { - wr, rd, cancel := repo.CatFileBatchCheck() + wr, rd, cancel := repo.CatFileBatchCheck(repo.Ctx) defer cancel() _, _ = wr.Write([]byte(name + "\n")) shaBs, _, _, err := ReadBatchLine(rd) @@ -53,7 +53,7 @@ func (repo *Repository) IsCommitExist(name string) bool { } func (repo *Repository) getCommit(id SHA1) (*Commit, error) { - wr, rd, cancel := repo.CatFileBatch() + wr, rd, cancel := repo.CatFileBatch(repo.Ctx) defer cancel() _, _ = wr.Write([]byte(id.String() + "\n")) @@ -132,7 +132,7 @@ func (repo *Repository) ConvertToSHA1(commitID string) (SHA1, error) { } } - wr, rd, cancel := repo.CatFileBatchCheck() + wr, rd, cancel := repo.CatFileBatchCheck(repo.Ctx) defer cancel() _, err := wr.Write([]byte(commitID + "\n")) if err != nil { diff --git a/modules/git/repo_language_stats_nogogit.go b/modules/git/repo_language_stats_nogogit.go index 06269a466c72..0c4f4aa0f5a6 100644 --- a/modules/git/repo_language_stats_nogogit.go +++ b/modules/git/repo_language_stats_nogogit.go @@ -26,7 +26,7 @@ import ( func (repo *Repository) GetLanguageStats(commitID string) (map[string]int64, error) { // We will feed the commit IDs in order into cat-file --batch, followed by blobs as necessary. // so let's create a batch stdin and stdout - batchStdinWriter, batchReader, cancel := repo.CatFileBatch() + batchStdinWriter, batchReader, cancel := repo.CatFileBatch(repo.Ctx) defer cancel() writeID := func(id string) error { diff --git a/modules/git/repo_tree_nogogit.go b/modules/git/repo_tree_nogogit.go index b27abb6e02a6..56a4a732e034 100644 --- a/modules/git/repo_tree_nogogit.go +++ b/modules/git/repo_tree_nogogit.go @@ -12,7 +12,7 @@ import ( ) func (repo *Repository) getTree(id SHA1) (*Tree, error) { - wr, rd, cancel := repo.CatFileBatch() + wr, rd, cancel := repo.CatFileBatch(repo.Ctx) defer cancel() _, _ = wr.Write([]byte(id.String() + "\n")) diff --git a/modules/git/tree_nogogit.go b/modules/git/tree_nogogit.go index 3d3fd26ece76..cfa2291e8fd0 100644 --- a/modules/git/tree_nogogit.go +++ b/modules/git/tree_nogogit.go @@ -36,7 +36,7 @@ func (t *Tree) ListEntries() (Entries, error) { } if t.repo != nil { - wr, rd, cancel := t.repo.CatFileBatch() + wr, rd, cancel := t.repo.CatFileBatch(t.repo.Ctx) defer cancel() _, _ = wr.Write([]byte(t.ID.String() + "\n")) diff --git a/modules/indexer/code/bleve.go b/modules/indexer/code/bleve.go index 8e5df34e60be..50a35727c950 100644 --- a/modules/indexer/code/bleve.go +++ b/modules/indexer/code/bleve.go @@ -271,7 +271,7 @@ func (b *BleveIndexer) Index(repo *models.Repository, sha string, changes *repoC batch := gitea_bleve.NewFlushingBatch(b.indexer, maxBatchSize) if len(changes.Updates) > 0 { - batchWriter, batchReader, cancel := git.CatFileBatch(repo.RepoPath()) + batchWriter, batchReader, cancel := git.CatFileBatch(git.DefaultContext, repo.RepoPath()) defer cancel() for _, update := range changes.Updates { diff --git a/modules/indexer/code/elastic_search.go b/modules/indexer/code/elastic_search.go index 49633c31916d..fd01264df849 100644 --- a/modules/indexer/code/elastic_search.go +++ b/modules/indexer/code/elastic_search.go @@ -245,7 +245,7 @@ func (b *ElasticSearchIndexer) Index(repo *models.Repository, sha string, change reqs := make([]elastic.BulkableRequest, 0) if len(changes.Updates) > 0 { - batchWriter, batchReader, cancel := git.CatFileBatch(repo.RepoPath()) + batchWriter, batchReader, cancel := git.CatFileBatch(git.DefaultContext, repo.RepoPath()) defer cancel() for _, update := range changes.Updates { From d2b01e4a5737e363e0f57253e8320c77fda0c21f Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 22 Sep 2021 20:17:57 +0100 Subject: [PATCH 05/26] minor comments Signed-off-by: Andrew Thornton --- modules/process/manager.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/modules/process/manager.go b/modules/process/manager.go index b3be1a64f570..e9d4ada45c04 100644 --- a/modules/process/manager.go +++ b/modules/process/manager.go @@ -38,7 +38,7 @@ var ( type Process struct { PID int64 // Process ID, not system one. ParentPID int64 - Children []*Process + Children []*Process // FIXME: access may need to be locked! Description string Start time.Time Cancel context.CancelFunc @@ -215,7 +215,6 @@ func (pm *Manager) Processes(onlyRoots bool) []*Process { return left.Start.Before(right.Start) }) - sort.Sort(processList(processes)) return processes } From 9598eca122ed310c8eb0f5d4684dc08abcfd943f Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Thu, 23 Sep 2021 16:21:25 +0100 Subject: [PATCH 06/26] fix lint and children lock Signed-off-by: Andrew Thornton --- modules/git/repo_base_nogogit.go | 2 +- modules/process/context.go | 2 ++ modules/process/manager.go | 41 +++++++++++++------------------- templates/admin/process-row.tmpl | 5 ++-- 4 files changed, 22 insertions(+), 28 deletions(-) diff --git a/modules/git/repo_base_nogogit.go b/modules/git/repo_base_nogogit.go index f307facd915f..14a6cacb4470 100644 --- a/modules/git/repo_base_nogogit.go +++ b/modules/git/repo_base_nogogit.go @@ -41,7 +41,7 @@ func OpenRepository(repoPath string) (*Repository, error) { return OpenRepositoryCtx(DefaultContext, repoPath) } -// OpenRepository opens the repository at the given path. +// OpenRepositoryCtx opens the repository at the given path with the provided context. func OpenRepositoryCtx(ctx context.Context, repoPath string) (*Repository, error) { repoPath, err := filepath.Abs(repoPath) if err != nil { diff --git a/modules/process/context.go b/modules/process/context.go index f8e4cec7cdd3..93c84ff884bd 100644 --- a/modules/process/context.go +++ b/modules/process/context.go @@ -24,6 +24,7 @@ func (c *Context) GetParent() *Context { return GetContext(c.Context) } +// Value is part of the interface for context.Context. We mostly defer to the internal context - but we return this in response to the ProcessContextKey func (c *Context) Value(key interface{}) interface{} { if key == ProcessContextKey { return c @@ -58,6 +59,7 @@ func GetPID(ctx context.Context) int64 { return pCtx.GetPID() } +// GetParentPID returns the ParentPID for this context func GetParentPID(ctx context.Context) int64 { parentPID := int64(0) if parentProcess := GetContext(ctx); parentProcess != nil { diff --git a/modules/process/manager.go b/modules/process/manager.go index e9d4ada45c04..e9088463dc79 100644 --- a/modules/process/manager.go +++ b/modules/process/manager.go @@ -38,12 +38,23 @@ var ( type Process struct { PID int64 // Process ID, not system one. ParentPID int64 - Children []*Process // FIXME: access may need to be locked! + children []*Process Description string Start time.Time Cancel context.CancelFunc } +// Children gets the children of the process +func (p *Process) Children() []*Process { + var children []*Process + pm := GetManager() + pm.mutex.Lock() + defer pm.mutex.Unlock() + children = make([]*Process, len(p.children)) + copy(children, p.children) + return children +} + // Manager knows about all processes and counts PIDs. type Manager struct { mutex sync.Mutex @@ -117,7 +128,7 @@ func (pm *Manager) Add(parentPID int64, description string, cancel context.Cance } if parent != nil { - parent.Children = append(parent.Children, process) + parent.children = append(parent.children, process) } pm.processes[pid] = process pm.mutex.Unlock() @@ -167,14 +178,14 @@ func (pm *Manager) remove(process *Process) { if p := pm.processes[process.PID]; p == process { delete(pm.processes, process.PID) pm.releasePID(process.PID) - for _, child := range process.Children { + for _, child := range process.children { child.ParentPID = 0 } parent := pm.processes[process.ParentPID] if parent != nil { - for i, child := range parent.Children { + for i, child := range parent.children { if child == process { - parent.Children = append(parent.Children[:i], parent.Children[i+1:]...) + parent.children = append(parent.children[:i], parent.children[i+1:]...) return } } @@ -285,26 +296,6 @@ func (pm *Manager) ExecDirEnvStdIn(timeout time.Duration, dir, desc string, env return stdOut.String(), stdErr.String(), err } -type processList []*Process - -func (l processList) Len() int { - return len(l) -} - -func (l processList) Less(i, j int) bool { - if l[i].ParentPID < l[j].ParentPID { - return true - } - if l[i].ParentPID == l[j].ParentPID { - return l[i].PID < l[j].PID - } - return false -} - -func (l processList) Swap(i, j int) { - l[i], l[j] = l[j], l[i] -} - // Error is a wrapped error describing the error results of Process Execution type Error struct { PID int64 diff --git a/templates/admin/process-row.tmpl b/templates/admin/process-row.tmpl index cba62e503d25..814727e7fae0 100644 --- a/templates/admin/process-row.tmpl +++ b/templates/admin/process-row.tmpl @@ -9,9 +9,10 @@
- {{if .Process.Children}} + {{$children := .Process.Children}} + {{if $children}}
- {{range .Process.Children}} + {{range $children}} {{template "admin/process-row" dict "Process" . "root" $.root}} {{end}}
From 7cf774919169f70b0b2e9521b847752cc46ca2a7 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 10 Oct 2021 18:29:50 +0100 Subject: [PATCH 07/26] separate remove and cancel functions Signed-off-by: Andrew Thornton --- modules/cron/tasks.go | 4 +-- modules/git/blame.go | 12 +++++--- modules/git/command.go | 4 +-- modules/git/diff.go | 4 +-- modules/git/repo_language_stats_nogogit.go | 8 +++++- modules/indexer/stats/db.go | 9 +++++- modules/markup/external/external.go | 4 +-- modules/process/manager.go | 25 ++++++++-------- modules/process/manager_test.go | 33 +++++++++++++++------- modules/task/migrate.go | 4 +-- routers/common/middleware.go | 4 +-- routers/web/repo/http.go | 4 +-- services/gitdiff/gitdiff.go | 4 +-- services/mailer/mailer.go | 6 ++-- 14 files changed, 79 insertions(+), 46 deletions(-) diff --git a/modules/cron/tasks.go b/modules/cron/tasks.go index 1d2b896afcc2..56588d48d511 100644 --- a/modules/cron/tasks.go +++ b/modules/cron/tasks.go @@ -81,8 +81,8 @@ func (t *Task) RunWithUser(doer *models.User, config Config) { }() graceful.GetManager().RunWithShutdownContext(func(baseCtx context.Context) { pm := process.GetManager() - ctx, cancel := pm.AddContext(baseCtx, config.FormatMessage(t.Name, "process", doer)) - defer cancel() + ctx, _, remove := pm.AddContext(baseCtx, config.FormatMessage(t.Name, "process", doer)) + defer remove() if err := t.fun(ctx, doer, config); err != nil { if models.IsErrCancelled(err) { diff --git a/modules/git/blame.go b/modules/git/blame.go index 42597af8dca2..44eca331e5d8 100644 --- a/modules/git/blame.go +++ b/modules/git/blame.go @@ -29,6 +29,7 @@ type BlameReader struct { reader *bufio.Reader lastSha *string cancel context.CancelFunc + remove context.CancelFunc } var shaLineRegex = regexp.MustCompile("^([a-z0-9]{40})") @@ -99,7 +100,8 @@ func (r *BlameReader) NextPart() (*BlamePart, error) { // Close BlameReader - don't run NextPart after invoking that func (r *BlameReader) Close() error { - defer r.cancel() + defer r.remove() + r.cancel() _ = r.output.Close() @@ -123,7 +125,7 @@ func CreateBlameReader(ctx context.Context, repoPath, commitID, file string) (*B func createBlameReader(ctx context.Context, dir string, command ...string) (*BlameReader, error) { // Here we use the provided context - this should be tied to the request performing the blame so that it does not hang around. - ctx, cancel := process.GetManager().AddContext(ctx, fmt.Sprintf("GetBlame [repo_path: %s]", dir)) + ctx, cancel, remove := process.GetManager().AddContext(ctx, fmt.Sprintf("GetBlame [repo_path: %s]", dir)) cmd := exec.CommandContext(ctx, command[0], command[1:]...) cmd.Dir = dir @@ -131,12 +133,13 @@ func createBlameReader(ctx context.Context, dir string, command ...string) (*Bla stdout, err := cmd.StdoutPipe() if err != nil { - defer cancel() + defer remove() return nil, fmt.Errorf("StdoutPipe: %v", err) } if err = cmd.Start(); err != nil { - defer cancel() + defer remove() + _ = stdout.Close() return nil, fmt.Errorf("Start: %v", err) } @@ -148,5 +151,6 @@ func createBlameReader(ctx context.Context, dir string, command ...string) (*Bla reader, nil, cancel, + remove, }, nil } diff --git a/modules/git/command.go b/modules/git/command.go index 6bfa01dc21c1..e06e0f94f5a9 100644 --- a/modules/git/command.go +++ b/modules/git/command.go @@ -148,8 +148,8 @@ func (c *Command) RunWithContext(rc *RunContext) error { desc = fmt.Sprintf("%s %s [repo_path: %s]", c.name, strings.Join(c.args, " "), rc.Dir) } - ctx, cancel := process.GetManager().AddContextTimeout(c.parentContext, rc.Timeout, desc) - defer cancel() + ctx, cancel, remove := process.GetManager().AddContextTimeout(c.parentContext, rc.Timeout, desc) + defer remove() cmd := exec.CommandContext(ctx, c.name, c.args...) if rc.Env == nil { diff --git a/modules/git/diff.go b/modules/git/diff.go index c8bb4fe07056..8261355d6d6c 100644 --- a/modules/git/diff.go +++ b/modules/git/diff.go @@ -56,8 +56,8 @@ func GetRepoRawDiffForFile(repo *Repository, startCommit, endCommit string, diff fileArgs = append(fileArgs, "--", file) } // FIXME: graceful: These commands should have a timeout - ctx, cancel := process.GetManager().AddContext(DefaultContext, fmt.Sprintf("GetRawDiffForFile: [repo_path: %s]", repo.Path)) - defer cancel() + ctx, _, remove := process.GetManager().AddContext(DefaultContext, fmt.Sprintf("GetRawDiffForFile: [repo_path: %s]", repo.Path)) + defer remove() var cmd *exec.Cmd switch diffType { diff --git a/modules/git/repo_language_stats_nogogit.go b/modules/git/repo_language_stats_nogogit.go index 0c4f4aa0f5a6..5c0d39c0ad18 100644 --- a/modules/git/repo_language_stats_nogogit.go +++ b/modules/git/repo_language_stats_nogogit.go @@ -83,7 +83,7 @@ func (repo *Repository) GetLanguageStats(commitID string) (map[string]int64, err IndexFile: indexFilename, WorkTree: tmpWorkTree, } - ctx, cancel := context.WithCancel(DefaultContext) + ctx, cancel := context.WithCancel(repo.Ctx) if err := checker.Init(ctx); err != nil { log.Error("Unable to open checker for %s. Error: %v", commitID, err) } else { @@ -104,6 +104,12 @@ func (repo *Repository) GetLanguageStats(commitID string) (map[string]int64, err var content []byte sizes := make(map[string]int64) for _, f := range entries { + select { + case <-repo.Ctx.Done(): + return sizes, repo.Ctx.Err() + default: + } + contentBuf.Reset() content = contentBuf.Bytes() diff --git a/modules/indexer/stats/db.go b/modules/indexer/stats/db.go index 87e8677a289f..f46be7fc3b56 100644 --- a/modules/indexer/stats/db.go +++ b/modules/indexer/stats/db.go @@ -5,9 +5,13 @@ package stats import ( + "fmt" + "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/graceful" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/process" ) // DBIndexer implements Indexer interface to use database's like search @@ -16,6 +20,9 @@ type DBIndexer struct { // Index repository status function func (db *DBIndexer) Index(id int64) error { + ctx, _, remove := process.GetManager().AddContext(graceful.GetManager().ShutdownContext(), fmt.Sprintf("Stats.DB Index Repo[%d]", id)) + defer remove() + repo, err := models.GetRepositoryByID(id) if err != nil { return err @@ -29,7 +36,7 @@ func (db *DBIndexer) Index(id int64) error { return err } - gitRepo, err := git.OpenRepository(repo.RepoPath()) + gitRepo, err := git.OpenRepositoryCtx(ctx, repo.RepoPath()) if err != nil { return err } diff --git a/modules/markup/external/external.go b/modules/markup/external/external.go index 875e79fa3353..1df13ab17d40 100644 --- a/modules/markup/external/external.go +++ b/modules/markup/external/external.go @@ -106,8 +106,8 @@ func (p *Renderer) Render(ctx *markup.RenderContext, input io.Reader, output io. ctx.Ctx = graceful.GetManager().ShutdownContext() } - processCtx, cancel := process.GetManager().AddContext(ctx.Ctx, fmt.Sprintf("Render [%s] for %s", commands[0], ctx.URLPrefix)) - defer cancel() + processCtx, _, remove := process.GetManager().AddContext(ctx.Ctx, fmt.Sprintf("Render [%s] for %s", commands[0], ctx.URLPrefix)) + defer remove() cmd := exec.CommandContext(processCtx, commands[0], args...) cmd.Env = append( diff --git a/modules/process/manager.go b/modules/process/manager.go index e9088463dc79..117637bc8e32 100644 --- a/modules/process/manager.go +++ b/modules/process/manager.go @@ -78,31 +78,31 @@ func GetManager() *Manager { } // AddContext create a new context and add it as a process. The CancelFunc must always be called even if the context is Done() -func (pm *Manager) AddContext(parent context.Context, description string) (context.Context, context.CancelFunc) { +func (pm *Manager) AddContext(parent context.Context, description string) (ctx context.Context, cancel, remove context.CancelFunc) { parentPID := GetParentPID(parent) - ctx, cancel := context.WithCancel(parent) + ctx, cancel = context.WithCancel(parent) - pid, cancel := pm.Add(parentPID, description, cancel) + pid, remove := pm.Add(parentPID, description, cancel) return &Context{ Context: ctx, pid: pid, - }, cancel + }, cancel, remove } // AddContextTimeout create a new context and add it as a process -func (pm *Manager) AddContextTimeout(parent context.Context, timeout time.Duration, description string) (context.Context, context.CancelFunc) { +func (pm *Manager) AddContextTimeout(parent context.Context, timeout time.Duration, description string) (ctx context.Context, cancel, remove context.CancelFunc) { parentPID := GetParentPID(parent) - ctx, cancel := context.WithTimeout(parent, timeout) + ctx, cancel = context.WithTimeout(parent, timeout) - pid, cancel := pm.Add(parentPID, description, cancel) + pid, remove := pm.Add(parentPID, description, cancel) return &Context{ Context: ctx, pid: pid, - }, cancel + }, cancel, remove } // Add create a new process @@ -120,9 +120,10 @@ func (pm *Manager) Add(parentPID int64, description string, cancel context.Cance ParentPID: parentPID, Description: description, Start: time.Now(), + Cancel: cancel, } - process.Cancel = func() { + remove := func() { cancel() pm.remove(process) } @@ -133,7 +134,7 @@ func (pm *Manager) Add(parentPID int64, description string, cancel context.Cance pm.processes[pid] = process pm.mutex.Unlock() - return pid, process.Cancel + return pid, remove } // nextPID will return the next available PID. pm.mutex should already be locked. @@ -264,8 +265,8 @@ func (pm *Manager) ExecDirEnvStdIn(timeout time.Duration, dir, desc string, env stdOut := new(bytes.Buffer) stdErr := new(bytes.Buffer) - ctx, cancel := pm.AddContextTimeout(DefaultContext, timeout, desc) - defer cancel() + ctx, _, remove := pm.AddContextTimeout(DefaultContext, timeout, desc) + defer remove() cmd := exec.CommandContext(ctx, cmdName, args...) cmd.Dir = dir diff --git a/modules/process/manager_test.go b/modules/process/manager_test.go index 96a2744b4b56..60474b3fef62 100644 --- a/modules/process/manager_test.go +++ b/modules/process/manager_test.go @@ -27,12 +27,12 @@ func TestManager_AddContext(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - p1Ctx, cancel := pm.AddContext(ctx, "foo") - defer cancel() + p1Ctx, _, remove := pm.AddContext(ctx, "foo") + defer remove() assert.Equal(t, int64(1), GetContext(p1Ctx).GetPID(), "expected to get pid 1 got %d", GetContext(p1Ctx).GetPID()) - p2Ctx, cancel := pm.AddContext(p1Ctx, "bar") - defer cancel() + p2Ctx, _, remove := pm.AddContext(p1Ctx, "bar") + defer remove() assert.Equal(t, int64(2), GetContext(p2Ctx).GetPID(), "expected to get pid 2 got %d", GetContext(p2Ctx).GetPID()) assert.Equal(t, int64(1), GetContext(p2Ctx).GetParent().GetPID(), "expected to get pid 1 got %d", GetContext(p2Ctx).GetParent().GetPID()) @@ -41,8 +41,8 @@ func TestManager_AddContext(t *testing.T) { func TestManager_Cancel(t *testing.T) { pm := Manager{processes: make(map[int64]*Process), next: 1, low: 1} - ctx, cancel := pm.AddContext(context.Background(), "foo") - defer cancel() + ctx, _, remove := pm.AddContext(context.Background(), "foo") + defer remove() pm.Cancel(GetPID(ctx)) @@ -51,6 +51,19 @@ func TestManager_Cancel(t *testing.T) { default: assert.Fail(t, "Cancel should cancel the provided context") } + remove() + + ctx, cancel, remove := pm.AddContext(context.Background(), "foo") + defer remove() + + cancel() + + select { + case <-ctx.Done(): + default: + assert.Fail(t, "Cancel should cancel the provided context") + } + remove() } func TestManager_Remove(t *testing.T) { @@ -59,12 +72,12 @@ func TestManager_Remove(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - p1Ctx, cancel := pm.AddContext(ctx, "foo") - defer cancel() + p1Ctx, _, remove := pm.AddContext(ctx, "foo") + defer remove() assert.Equal(t, int64(1), GetContext(p1Ctx).GetPID(), "expected to get pid 1 got %d", GetContext(p1Ctx).GetPID()) - p2Ctx, cancel := pm.AddContext(p1Ctx, "bar") - defer cancel() + p2Ctx, _, remove := pm.AddContext(p1Ctx, "bar") + defer remove() assert.Equal(t, int64(2), GetContext(p2Ctx).GetPID(), "expected to get pid 2 got %d", GetContext(p2Ctx).GetPID()) diff --git a/modules/task/migrate.go b/modules/task/migrate.go index 6a7993e30d79..c3003d894314 100644 --- a/modules/task/migrate.go +++ b/modules/task/migrate.go @@ -94,8 +94,8 @@ func runMigrateTask(t *models.Task) (err error) { opts.MigrateToRepoID = t.RepoID pm := process.GetManager() - ctx, cancel := pm.AddContext(graceful.GetManager().ShutdownContext(), fmt.Sprintf("MigrateTask: %s/%s", t.Owner.Name, opts.RepoName)) - defer cancel() + ctx, _, remove := pm.AddContext(graceful.GetManager().ShutdownContext(), fmt.Sprintf("MigrateTask: %s/%s", t.Owner.Name, opts.RepoName)) + defer remove() t.StartTime = timeutil.TimeStampNow() t.Status = structs.TaskStatusRunning diff --git a/routers/common/middleware.go b/routers/common/middleware.go index e1eebe579ecc..bafe6a4745a6 100644 --- a/routers/common/middleware.go +++ b/routers/common/middleware.go @@ -23,8 +23,8 @@ func Middlewares() []func(http.Handler) http.Handler { var handlers = []func(http.Handler) http.Handler{ func(next http.Handler) http.Handler { return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) { - ctx, cancel := process.GetManager().AddContext(req.Context(), fmt.Sprintf("%s: %s", req.Method, req.RequestURI)) - defer cancel() + ctx, _, remove := process.GetManager().AddContext(req.Context(), fmt.Sprintf("%s: %s", req.Method, req.RequestURI)) + defer remove() next.ServeHTTP(context.NewResponse(resp), req.WithContext(ctx)) }) }, diff --git a/routers/web/repo/http.go b/routers/web/repo/http.go index 858c5f7ca8c0..e15fdc18e5bb 100644 --- a/routers/web/repo/http.go +++ b/routers/web/repo/http.go @@ -482,8 +482,8 @@ func serviceRPC(h serviceHandler, service string) { } // ctx, cancel := gocontext.WithCancel(git.DefaultContext) - ctx, cancel := process.GetManager().AddContext(git.DefaultContext, fmt.Sprintf("%s %s %s [repo_path: %s]", git.GitExecutable, service, "--stateless-rpc", h.dir)) - defer cancel() + ctx, _, remove := process.GetManager().AddContext(git.DefaultContext, fmt.Sprintf("%s %s %s [repo_path: %s]", git.GitExecutable, service, "--stateless-rpc", h.dir)) + defer remove() var stderr bytes.Buffer cmd := exec.CommandContext(ctx, git.GitExecutable, service, "--stateless-rpc", h.dir) diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 763ef40e5a0a..3f35128e5de8 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -1226,8 +1226,8 @@ func GetDiffRangeWithWhitespaceBehavior(gitRepo *git.Repository, beforeCommitID, } timeout := time.Duration(setting.Git.Timeout.Default) * time.Second - ctx, cancel := process.GetManager().AddContextTimeout(git.DefaultContext, timeout, fmt.Sprintf("GetDiffRange [repo_path: %s]", repoPath)) - defer cancel() + ctx, _, remove := process.GetManager().AddContextTimeout(git.DefaultContext, timeout, fmt.Sprintf("GetDiffRange [repo_path: %s]", repoPath)) + defer remove() var cmd *exec.Cmd if (len(beforeCommitID) == 0 || beforeCommitID == git.EmptySHA) && commit.ParentCount() == 0 { diff --git a/services/mailer/mailer.go b/services/mailer/mailer.go index c9d5d254f177..fd7a86936d75 100644 --- a/services/mailer/mailer.go +++ b/services/mailer/mailer.go @@ -248,8 +248,8 @@ func (s *sendmailSender) Send(from string, to []string, msg io.WriterTo) error { desc := fmt.Sprintf("SendMail: %s %v", setting.MailService.SendmailPath, args) - ctx, cancel := process.GetManager().AddContextTimeout(graceful.GetManager().HammerContext(), setting.MailService.SendmailTimeout, desc) - defer cancel() + ctx, cancel, remove := process.GetManager().AddContextTimeout(graceful.GetManager().HammerContext(), setting.MailService.SendmailTimeout, desc) + defer remove() cmd := exec.CommandContext(ctx, setting.MailService.SendmailPath, args...) pipe, err := cmd.StdinPipe() @@ -259,6 +259,8 @@ func (s *sendmailSender) Send(from string, to []string, msg io.WriterTo) error { } if err = cmd.Start(); err != nil { + _ = pipe.Close() + cancel() return err } From a8e228e102b82fde770bd205abad19482541f828 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 10 Oct 2021 20:23:16 +0100 Subject: [PATCH 08/26] associate repo functions with the repo context Signed-off-by: Andrew Thornton --- cmd/hook.go | 2 +- modules/git/repo_attribute.go | 2 +- modules/git/repo_base_gogit.go | 4 +-- modules/git/repo_blame.go | 4 +-- modules/git/repo_branch.go | 25 +++++++++--------- modules/git/repo_branch_nogogit.go | 7 ++--- modules/git/repo_commit.go | 42 +++++++++++++++--------------- modules/git/repo_commit_gogit.go | 2 +- modules/git/repo_commit_nogogit.go | 4 +-- modules/git/repo_compare.go | 20 +++++++------- modules/git/repo_gpg.go | 8 +++--- modules/git/repo_index.go | 14 +++++----- modules/git/repo_object.go | 2 +- modules/git/repo_ref_nogogit.go | 2 +- modules/git/repo_stats.go | 4 +-- modules/git/repo_tag.go | 19 +++++++------- modules/git/repo_tag_nogogit.go | 2 +- modules/git/repo_tree.go | 2 +- modules/git/repo_tree_gogit.go | 2 +- modules/repository/branch.go | 2 +- routers/api/v1/repo/branch.go | 2 +- routers/web/repo/issue.go | 2 +- routers/web/repo/pull.go | 2 +- services/pull/commit_status.go | 2 +- services/pull/pull.go | 2 +- services/pull/temp_repo.go | 2 +- services/wiki/wiki.go | 2 +- 27 files changed, 93 insertions(+), 90 deletions(-) diff --git a/cmd/hook.go b/cmd/hook.go index fb43add8d4ff..2307596f288d 100644 --- a/cmd/hook.go +++ b/cmd/hook.go @@ -310,7 +310,7 @@ func runHookPostReceive(c *cli.Context) error { defer cancel() // First of all run update-server-info no matter what - if _, err := git.NewCommand("update-server-info").SetParentContext(ctx).Run(); err != nil { + if _, err := git.NewCommandContext(ctx, "update-server-info").Run(); err != nil { return fmt.Errorf("Failed to call 'git update-server-info': %v", err) } diff --git a/modules/git/repo_attribute.go b/modules/git/repo_attribute.go index aace64425388..cca4c46841fb 100644 --- a/modules/git/repo_attribute.go +++ b/modules/git/repo_attribute.go @@ -59,7 +59,7 @@ func (repo *Repository) CheckAttribute(opts CheckAttributeOpts) (map[string]map[ } } - cmd := NewCommand(cmdArgs...) + cmd := NewCommandContext(repo.Ctx, cmdArgs...) if err := cmd.RunInDirPipeline(repo.Path, stdOut, stdErr); err != nil { return nil, fmt.Errorf("failed to run check-attr: %v\n%s\n%s", err, stdOut.String(), stdErr.String()) diff --git a/modules/git/repo_base_gogit.go b/modules/git/repo_base_gogit.go index d4aef56746ae..72995265622e 100644 --- a/modules/git/repo_base_gogit.go +++ b/modules/git/repo_base_gogit.go @@ -32,7 +32,7 @@ type Repository struct { gogitStorage *filesystem.Storage gpgSettings *GPGSettings - ctx context.Context + Ctx context.Context } // OpenRepository opens the repository at the given path. @@ -68,7 +68,7 @@ func OpenRepositoryCtx(ctx context.Context, repoPath string) (*Repository, error gogitRepo: gogitRepo, gogitStorage: storage, tagCache: newObjectCache(), - ctx: ctx, + Ctx: ctx, }, nil } diff --git a/modules/git/repo_blame.go b/modules/git/repo_blame.go index 5c023554f17b..4ca05e3ba421 100644 --- a/modules/git/repo_blame.go +++ b/modules/git/repo_blame.go @@ -8,12 +8,12 @@ import "fmt" // FileBlame return the Blame object of file func (repo *Repository) FileBlame(revision, path, file string) ([]byte, error) { - return NewCommand("blame", "--root", "--", file).RunInDirBytes(path) + return NewCommandContext(repo.Ctx, "blame", "--root", "--", file).RunInDirBytes(path) } // LineBlame returns the latest commit at the given line func (repo *Repository) LineBlame(revision, path, file string, line uint) (*Commit, error) { - res, err := NewCommand("blame", fmt.Sprintf("-L %d,%d", line, line), "-p", revision, "--", file).RunInDir(path) + res, err := NewCommandContext(repo.Ctx, "blame", fmt.Sprintf("-L %d,%d", line, line), "-p", revision, "--", file).RunInDir(path) if err != nil { return nil, err } diff --git a/modules/git/repo_branch.go b/modules/git/repo_branch.go index 96f692826ec9..98b1bc8ae7c7 100644 --- a/modules/git/repo_branch.go +++ b/modules/git/repo_branch.go @@ -6,6 +6,7 @@ package git import ( + "context" "fmt" "strings" ) @@ -22,14 +23,14 @@ const PullRequestPrefix = "refs/for/" // TODO: /refs/for-review for suggest change interface // IsReferenceExist returns true if given reference exists in the repository. -func IsReferenceExist(repoPath, name string) bool { - _, err := NewCommand("show-ref", "--verify", "--", name).RunInDir(repoPath) +func IsReferenceExist(ctx context.Context, repoPath, name string) bool { + _, err := NewCommandContext(ctx, "show-ref", "--verify", "--", name).RunInDir(repoPath) return err == nil } // IsBranchExist returns true if given branch exists in the repository. -func IsBranchExist(repoPath, name string) bool { - return IsReferenceExist(repoPath, BranchPrefix+name) +func IsBranchExist(ctx context.Context, repoPath, name string) bool { + return IsReferenceExist(ctx, repoPath, BranchPrefix+name) } // Branch represents a Git branch. @@ -45,7 +46,7 @@ func (repo *Repository) GetHEADBranch() (*Branch, error) { if repo == nil { return nil, fmt.Errorf("nil repo") } - stdout, err := NewCommand("symbolic-ref", "HEAD").RunInDir(repo.Path) + stdout, err := NewCommandContext(repo.Ctx, "symbolic-ref", "HEAD").RunInDir(repo.Path) if err != nil { return nil, err } @@ -64,13 +65,13 @@ func (repo *Repository) GetHEADBranch() (*Branch, error) { // SetDefaultBranch sets default branch of repository. func (repo *Repository) SetDefaultBranch(name string) error { - _, err := NewCommand("symbolic-ref", "HEAD", BranchPrefix+name).RunInDir(repo.Path) + _, err := NewCommandContext(repo.Ctx, "symbolic-ref", "HEAD", BranchPrefix+name).RunInDir(repo.Path) return err } // GetDefaultBranch gets default branch of repository. func (repo *Repository) GetDefaultBranch() (string, error) { - return NewCommand("symbolic-ref", "HEAD").RunInDir(repo.Path) + return NewCommandContext(repo.Ctx, "symbolic-ref", "HEAD").RunInDir(repo.Path) } // GetBranch returns a branch by it's name @@ -118,7 +119,7 @@ type DeleteBranchOptions struct { // DeleteBranch delete a branch by name on repository. func (repo *Repository) DeleteBranch(name string, opts DeleteBranchOptions) error { - cmd := NewCommand("branch") + cmd := NewCommandContext(repo.Ctx, "branch") if opts.Force { cmd.AddArguments("-D") @@ -134,7 +135,7 @@ func (repo *Repository) DeleteBranch(name string, opts DeleteBranchOptions) erro // CreateBranch create a new branch func (repo *Repository) CreateBranch(branch, oldbranchOrCommit string) error { - cmd := NewCommand("branch") + cmd := NewCommandContext(repo.Ctx, "branch") cmd.AddArguments("--", branch, oldbranchOrCommit) _, err := cmd.RunInDir(repo.Path) @@ -144,7 +145,7 @@ func (repo *Repository) CreateBranch(branch, oldbranchOrCommit string) error { // AddRemote adds a new remote to repository. func (repo *Repository) AddRemote(name, url string, fetch bool) error { - cmd := NewCommand("remote", "add") + cmd := NewCommandContext(repo.Ctx, "remote", "add") if fetch { cmd.AddArguments("-f") } @@ -156,7 +157,7 @@ func (repo *Repository) AddRemote(name, url string, fetch bool) error { // RemoveRemote removes a remote from repository. func (repo *Repository) RemoveRemote(name string) error { - _, err := NewCommand("remote", "rm", name).RunInDir(repo.Path) + _, err := NewCommandContext(repo.Ctx, "remote", "rm", name).RunInDir(repo.Path) return err } @@ -167,6 +168,6 @@ func (branch *Branch) GetCommit() (*Commit, error) { // RenameBranch rename a branch func (repo *Repository) RenameBranch(from, to string) error { - _, err := NewCommand("branch", "-m", from, to).RunInDir(repo.Path) + _, err := NewCommandContext(repo.Ctx, "branch", "-m", from, to).RunInDir(repo.Path) return err } diff --git a/modules/git/repo_branch_nogogit.go b/modules/git/repo_branch_nogogit.go index 22383201813b..1928c7515bca 100644 --- a/modules/git/repo_branch_nogogit.go +++ b/modules/git/repo_branch_nogogit.go @@ -11,6 +11,7 @@ package git import ( "bufio" "bytes" + "context" "io" "strings" @@ -63,11 +64,11 @@ func (repo *Repository) IsBranchExist(name string) bool { // GetBranches returns branches from the repository, skipping skip initial branches and // returning at most limit branches, or all branches if limit is 0. func (repo *Repository) GetBranches(skip, limit int) ([]string, int, error) { - return callShowRef(repo.Path, BranchPrefix, "--heads", skip, limit) + return callShowRef(repo.Ctx, repo.Path, BranchPrefix, "--heads", skip, limit) } // callShowRef return refs, if limit = 0 it will not limit -func callShowRef(repoPath, prefix, arg string, skip, limit int) (branchNames []string, countAll int, err error) { +func callShowRef(ctx context.Context, repoPath, prefix, arg string, skip, limit int) (branchNames []string, countAll int, err error) { stdoutReader, stdoutWriter := io.Pipe() defer func() { _ = stdoutReader.Close() @@ -76,7 +77,7 @@ func callShowRef(repoPath, prefix, arg string, skip, limit int) (branchNames []s go func() { stderrBuilder := &strings.Builder{} - err := NewCommand("show-ref", arg).RunInDirPipeline(repoPath, stdoutWriter, stderrBuilder) + err := NewCommandContext(ctx, "show-ref", arg).RunInDirPipeline(repoPath, stdoutWriter, stderrBuilder) if err != nil { if stderrBuilder.Len() == 0 { _ = stdoutWriter.Close() diff --git a/modules/git/repo_commit.go b/modules/git/repo_commit.go index 25060f56da62..3afabac27b51 100644 --- a/modules/git/repo_commit.go +++ b/modules/git/repo_commit.go @@ -58,7 +58,7 @@ func (repo *Repository) getCommitByPathWithID(id SHA1, relpath string) (*Commit, relpath = `\` + relpath } - stdout, err := NewCommand("log", "-1", prettyLogFormat, id.String(), "--", relpath).RunInDir(repo.Path) + stdout, err := NewCommandContext(repo.Ctx, "log", "-1", prettyLogFormat, id.String(), "--", relpath).RunInDir(repo.Path) if err != nil { return nil, err } @@ -73,7 +73,7 @@ func (repo *Repository) getCommitByPathWithID(id SHA1, relpath string) (*Commit, // GetCommitByPath returns the last commit of relative path. func (repo *Repository) GetCommitByPath(relpath string) (*Commit, error) { - stdout, err := NewCommand("log", "-1", prettyLogFormat, "--", relpath).RunInDirBytes(repo.Path) + stdout, err := NewCommandContext(repo.Ctx, "log", "-1", prettyLogFormat, "--", relpath).RunInDirBytes(repo.Path) if err != nil { return nil, err } @@ -86,7 +86,7 @@ func (repo *Repository) GetCommitByPath(relpath string) (*Commit, error) { } func (repo *Repository) commitsByRange(id SHA1, page, pageSize int) ([]*Commit, error) { - stdout, err := NewCommand("log", id.String(), "--skip="+strconv.Itoa((page-1)*pageSize), + stdout, err := NewCommandContext(repo.Ctx, "log", id.String(), "--skip="+strconv.Itoa((page-1)*pageSize), "--max-count="+strconv.Itoa(pageSize), prettyLogFormat).RunInDirBytes(repo.Path) if err != nil { @@ -97,7 +97,7 @@ func (repo *Repository) commitsByRange(id SHA1, page, pageSize int) ([]*Commit, func (repo *Repository) searchCommits(id SHA1, opts SearchCommitsOptions) ([]*Commit, error) { // create new git log command with limit of 100 commis - cmd := NewCommand("log", id.String(), "-100", prettyLogFormat) + cmd := NewCommandContext(repo.Ctx, "log", id.String(), "-100", prettyLogFormat) // ignore case args := []string{"-i"} @@ -155,7 +155,7 @@ func (repo *Repository) searchCommits(id SHA1, opts SearchCommitsOptions) ([]*Co // ignore anything below 4 characters as too unspecific if len(v) >= 4 { // create new git log command with 1 commit limit - hashCmd := NewCommand("log", "-1", prettyLogFormat) + hashCmd := NewCommandContext(repo.Ctx, "log", "-1", prettyLogFormat) // add previous arguments except for --grep and --all hashCmd.AddArguments(args...) // add keyword as @@ -176,7 +176,7 @@ func (repo *Repository) searchCommits(id SHA1, opts SearchCommitsOptions) ([]*Co } func (repo *Repository) getFilesChanged(id1, id2 string) ([]string, error) { - stdout, err := NewCommand("diff", "--name-only", id1, id2).RunInDirBytes(repo.Path) + stdout, err := NewCommandContext(repo.Ctx, "diff", "--name-only", id1, id2).RunInDirBytes(repo.Path) if err != nil { return nil, err } @@ -186,7 +186,7 @@ func (repo *Repository) getFilesChanged(id1, id2 string) ([]string, error) { // FileChangedBetweenCommits Returns true if the file changed between commit IDs id1 and id2 // You must ensure that id1 and id2 are valid commit ids. func (repo *Repository) FileChangedBetweenCommits(filename, id1, id2 string) (bool, error) { - stdout, err := NewCommand("diff", "--name-only", "-z", id1, id2, "--", filename).RunInDirBytes(repo.Path) + stdout, err := NewCommandContext(repo.Ctx, "diff", "--name-only", "-z", id1, id2, "--", filename).RunInDirBytes(repo.Path) if err != nil { return false, err } @@ -209,7 +209,7 @@ func (repo *Repository) CommitsByFileAndRange(revision, file string, page int) ( }() go func() { stderr := strings.Builder{} - err := NewCommand("log", revision, "--follow", + err := NewCommandContext(repo.Ctx, "log", revision, "--follow", "--max-count="+strconv.Itoa(setting.Git.CommitsRangeSize*page), prettyLogFormat, "--", file). RunInDirPipeline(repo.Path, stdoutWriter, &stderr) @@ -240,7 +240,7 @@ func (repo *Repository) CommitsByFileAndRange(revision, file string, page int) ( // CommitsByFileAndRangeNoFollow return the commits according revision file and the page func (repo *Repository) CommitsByFileAndRangeNoFollow(revision, file string, page int) ([]*Commit, error) { - stdout, err := NewCommand("log", revision, "--skip="+strconv.Itoa((page-1)*50), + stdout, err := NewCommandContext(repo.Ctx, "log", revision, "--skip="+strconv.Itoa((page-1)*50), "--max-count="+strconv.Itoa(setting.Git.CommitsRangeSize), prettyLogFormat, "--", file).RunInDirBytes(repo.Path) if err != nil { return nil, err @@ -250,11 +250,11 @@ func (repo *Repository) CommitsByFileAndRangeNoFollow(revision, file string, pag // FilesCountBetween return the number of files changed between two commits func (repo *Repository) FilesCountBetween(startCommitID, endCommitID string) (int, error) { - stdout, err := NewCommand("diff", "--name-only", startCommitID+"..."+endCommitID).RunInDir(repo.Path) + stdout, err := NewCommandContext(repo.Ctx, "diff", "--name-only", startCommitID+"..."+endCommitID).RunInDir(repo.Path) if err != nil && strings.Contains(err.Error(), "no merge base") { // git >= 2.28 now returns an error if startCommitID and endCommitID have become unrelated. // previously it would return the results of git diff --name-only startCommitID endCommitID so let's try that... - stdout, err = NewCommand("diff", "--name-only", startCommitID, endCommitID).RunInDir(repo.Path) + stdout, err = NewCommandContext(repo.Ctx, "diff", "--name-only", startCommitID, endCommitID).RunInDir(repo.Path) } if err != nil { return 0, err @@ -268,13 +268,13 @@ func (repo *Repository) CommitsBetween(last *Commit, before *Commit) ([]*Commit, var stdout []byte var err error if before == nil { - stdout, err = NewCommand("rev-list", last.ID.String()).RunInDirBytes(repo.Path) + stdout, err = NewCommandContext(repo.Ctx, "rev-list", last.ID.String()).RunInDirBytes(repo.Path) } else { - stdout, err = NewCommand("rev-list", before.ID.String()+".."+last.ID.String()).RunInDirBytes(repo.Path) + stdout, err = NewCommandContext(repo.Ctx, "rev-list", before.ID.String()+".."+last.ID.String()).RunInDirBytes(repo.Path) if err != nil && strings.Contains(err.Error(), "no merge base") { // future versions of git >= 2.28 are likely to return an error if before and last have become unrelated. // previously it would return the results of git rev-list before last so let's try that... - stdout, err = NewCommand("rev-list", before.ID.String(), last.ID.String()).RunInDirBytes(repo.Path) + stdout, err = NewCommandContext(repo.Ctx, "rev-list", before.ID.String(), last.ID.String()).RunInDirBytes(repo.Path) } } if err != nil { @@ -288,13 +288,13 @@ func (repo *Repository) CommitsBetweenLimit(last *Commit, before *Commit, limit, var stdout []byte var err error if before == nil { - stdout, err = NewCommand("rev-list", "--max-count", strconv.Itoa(limit), "--skip", strconv.Itoa(skip), last.ID.String()).RunInDirBytes(repo.Path) + stdout, err = NewCommandContext(repo.Ctx, "rev-list", "--max-count", strconv.Itoa(limit), "--skip", strconv.Itoa(skip), last.ID.String()).RunInDirBytes(repo.Path) } else { - stdout, err = NewCommand("rev-list", "--max-count", strconv.Itoa(limit), "--skip", strconv.Itoa(skip), before.ID.String()+".."+last.ID.String()).RunInDirBytes(repo.Path) + stdout, err = NewCommandContext(repo.Ctx, "rev-list", "--max-count", strconv.Itoa(limit), "--skip", strconv.Itoa(skip), before.ID.String()+".."+last.ID.String()).RunInDirBytes(repo.Path) if err != nil && strings.Contains(err.Error(), "no merge base") { // future versions of git >= 2.28 are likely to return an error if before and last have become unrelated. // previously it would return the results of git rev-list --max-count n before last so let's try that... - stdout, err = NewCommand("rev-list", "--max-count", strconv.Itoa(limit), "--skip", strconv.Itoa(skip), before.ID.String(), last.ID.String()).RunInDirBytes(repo.Path) + stdout, err = NewCommandContext(repo.Ctx, "rev-list", "--max-count", strconv.Itoa(limit), "--skip", strconv.Itoa(skip), before.ID.String(), last.ID.String()).RunInDirBytes(repo.Path) } } if err != nil { @@ -333,7 +333,7 @@ func (repo *Repository) CommitsCountBetween(start, end string) (int64, error) { // commitsBefore the limit is depth, not total number of returned commits. func (repo *Repository) commitsBefore(id SHA1, limit int) ([]*Commit, error) { - cmd := NewCommand("log") + cmd := NewCommandContext(repo.Ctx, "log") if limit > 0 { cmd.AddArguments("-"+strconv.Itoa(limit), prettyLogFormat, id.String()) } else { @@ -377,7 +377,7 @@ func (repo *Repository) getCommitsBeforeLimit(id SHA1, num int) ([]*Commit, erro func (repo *Repository) getBranches(commit *Commit, limit int) ([]string, error) { if CheckGitVersionAtLeast("2.7.0") == nil { - stdout, err := NewCommand("for-each-ref", "--count="+strconv.Itoa(limit), "--format=%(refname:strip=2)", "--contains", commit.ID.String(), BranchPrefix).RunInDir(repo.Path) + stdout, err := NewCommandContext(repo.Ctx, "for-each-ref", "--count="+strconv.Itoa(limit), "--format=%(refname:strip=2)", "--contains", commit.ID.String(), BranchPrefix).RunInDir(repo.Path) if err != nil { return nil, err } @@ -386,7 +386,7 @@ func (repo *Repository) getBranches(commit *Commit, limit int) ([]string, error) return branches, nil } - stdout, err := NewCommand("branch", "--contains", commit.ID.String()).RunInDir(repo.Path) + stdout, err := NewCommandContext(repo.Ctx, "branch", "--contains", commit.ID.String()).RunInDir(repo.Path) if err != nil { return nil, err } @@ -425,7 +425,7 @@ func (repo *Repository) GetCommitsFromIDs(commitIDs []string) []*Commit { // IsCommitInBranch check if the commit is on the branch func (repo *Repository) IsCommitInBranch(commitID, branch string) (r bool, err error) { - stdout, err := NewCommand("branch", "--contains", commitID, branch).RunInDir(repo.Path) + stdout, err := NewCommandContext(repo.Ctx, "branch", "--contains", commitID, branch).RunInDir(repo.Path) if err != nil { return false, err } diff --git a/modules/git/repo_commit_gogit.go b/modules/git/repo_commit_gogit.go index 175b6e644660..f00b340d154e 100644 --- a/modules/git/repo_commit_gogit.go +++ b/modules/git/repo_commit_gogit.go @@ -40,7 +40,7 @@ func (repo *Repository) ConvertToSHA1(commitID string) (SHA1, error) { } } - actualCommitID, err := NewCommand("rev-parse", "--verify", commitID).RunInDir(repo.Path) + actualCommitID, err := NewCommandContext(repo.Ctx, "rev-parse", "--verify", commitID).RunInDir(repo.Path) if err != nil { if strings.Contains(err.Error(), "unknown revision or path") || strings.Contains(err.Error(), "fatal: Needed a single revision") { diff --git a/modules/git/repo_commit_nogogit.go b/modules/git/repo_commit_nogogit.go index 88cf56845e90..d86e7d32680b 100644 --- a/modules/git/repo_commit_nogogit.go +++ b/modules/git/repo_commit_nogogit.go @@ -18,7 +18,7 @@ import ( // ResolveReference resolves a name to a reference func (repo *Repository) ResolveReference(name string) (string, error) { - stdout, err := NewCommand("show-ref", "--hash", name).RunInDir(repo.Path) + stdout, err := NewCommandContext(repo.Ctx, "show-ref", "--hash", name).RunInDir(repo.Path) if err != nil { if strings.Contains(err.Error(), "not a valid ref") { return "", ErrNotExist{name, ""} @@ -48,7 +48,7 @@ func (repo *Repository) GetRefCommitID(name string) (string, error) { // IsCommitExist returns true if given commit exists in current repository. func (repo *Repository) IsCommitExist(name string) bool { - _, err := NewCommand("cat-file", "-e", name).RunInDir(repo.Path) + _, err := NewCommandContext(repo.Ctx, "cat-file", "-e", name).RunInDir(repo.Path) return err == nil } diff --git a/modules/git/repo_compare.go b/modules/git/repo_compare.go index 50e900551113..191de6e4a3ac 100644 --- a/modules/git/repo_compare.go +++ b/modules/git/repo_compare.go @@ -35,13 +35,13 @@ func (repo *Repository) GetMergeBase(tmpRemote string, base, head string) (strin if tmpRemote != "origin" { tmpBaseName := "refs/remotes/" + tmpRemote + "/tmp_" + base // Fetch commit into a temporary branch in order to be able to handle commits and tags - _, err := NewCommand("fetch", tmpRemote, base+":"+tmpBaseName).RunInDir(repo.Path) + _, err := NewCommandContext(repo.Ctx, "fetch", tmpRemote, base+":"+tmpBaseName).RunInDir(repo.Path) if err == nil { base = tmpBaseName } } - stdout, err := NewCommand("merge-base", "--", base, head).RunInDir(repo.Path) + stdout, err := NewCommandContext(repo.Ctx, "merge-base", "--", base, head).RunInDir(repo.Path) return strings.TrimSpace(stdout), base, err } @@ -87,7 +87,7 @@ func (repo *Repository) GetCompareInfo(basePath, baseBranch, headBranch string, } // We have a common base - therefore we know that ... should work - logs, err := NewCommand("log", baseCommitID+separator+headBranch, prettyLogFormat).RunInDirBytes(repo.Path) + logs, err := NewCommandContext(repo.Ctx, "log", baseCommitID+separator+headBranch, prettyLogFormat).RunInDirBytes(repo.Path) if err != nil { return nil, err } @@ -137,14 +137,14 @@ func (repo *Repository) GetDiffNumChangedFiles(base, head string, directComparis separator = ".." } - if err := NewCommand("diff", "-z", "--name-only", base+separator+head). + if err := NewCommandContext(repo.Ctx, "diff", "-z", "--name-only", base+separator+head). RunInDirPipeline(repo.Path, w, stderr); err != nil { if strings.Contains(stderr.String(), "no merge base") { // git >= 2.28 now returns an error if base and head have become unrelated. // previously it would return the results of git diff -z --name-only base head so let's try that... w = &lineCountWriter{} stderr.Reset() - if err = NewCommand("diff", "-z", "--name-only", base, head).RunInDirPipeline(repo.Path, w, stderr); err == nil { + if err = NewCommandContext(repo.Ctx, "diff", "-z", "--name-only", base, head).RunInDirPipeline(repo.Path, w, stderr); err == nil { return w.numLines, nil } } @@ -227,23 +227,23 @@ func (repo *Repository) GetDiffOrPatch(base, head string, w io.Writer, patch, bi // GetDiff generates and returns patch data between given revisions, optimized for human readability func (repo *Repository) GetDiff(base, head string, w io.Writer) error { - return NewCommand("diff", "-p", base, head). + return NewCommandContext(repo.Ctx, "diff", "-p", base, head). RunInDirPipeline(repo.Path, w, nil) } // GetDiffBinary generates and returns patch data between given revisions, including binary diffs. func (repo *Repository) GetDiffBinary(base, head string, w io.Writer) error { - return NewCommand("diff", "-p", "--binary", base, head). + return NewCommandContext(repo.Ctx, "diff", "-p", "--binary", base, head). RunInDirPipeline(repo.Path, w, nil) } // GetPatch generates and returns format-patch data between given revisions, able to be used with `git apply` func (repo *Repository) GetPatch(base, head string, w io.Writer) error { stderr := new(bytes.Buffer) - err := NewCommand("format-patch", "--binary", "--stdout", base+"..."+head). + err := NewCommandContext(repo.Ctx, "format-patch", "--binary", "--stdout", base+"..."+head). RunInDirPipeline(repo.Path, w, stderr) if err != nil && bytes.Contains(stderr.Bytes(), []byte("no merge base")) { - return NewCommand("format-patch", "--binary", "--stdout", base, head). + return NewCommandContext(repo.Ctx, "format-patch", "--binary", "--stdout", base, head). RunInDirPipeline(repo.Path, w, nil) } return err @@ -252,7 +252,7 @@ func (repo *Repository) GetPatch(base, head string, w io.Writer) error { // GetDiffFromMergeBase generates and return patch data from merge base to head func (repo *Repository) GetDiffFromMergeBase(base, head string, w io.Writer) error { stderr := new(bytes.Buffer) - err := NewCommand("diff", "-p", "--binary", base+"..."+head). + err := NewCommandContext(repo.Ctx, "diff", "-p", "--binary", base+"..."+head). RunInDirPipeline(repo.Path, w, stderr) if err != nil && bytes.Contains(stderr.Bytes(), []byte("no merge base")) { return repo.GetDiffBinary(base, head, w) diff --git a/modules/git/repo_gpg.go b/modules/git/repo_gpg.go index b4c3f3b4314a..addf6a6b6296 100644 --- a/modules/git/repo_gpg.go +++ b/modules/git/repo_gpg.go @@ -34,7 +34,7 @@ func (repo *Repository) GetDefaultPublicGPGKey(forceUpdate bool) (*GPGSettings, Sign: true, } - value, _ := NewCommand("config", "--get", "commit.gpgsign").RunInDir(repo.Path) + value, _ := NewCommandContext(repo.Ctx, "config", "--get", "commit.gpgsign").RunInDir(repo.Path) sign, valid := ParseBool(strings.TrimSpace(value)) if !sign || !valid { gpgSettings.Sign = false @@ -42,13 +42,13 @@ func (repo *Repository) GetDefaultPublicGPGKey(forceUpdate bool) (*GPGSettings, return gpgSettings, nil } - signingKey, _ := NewCommand("config", "--get", "user.signingkey").RunInDir(repo.Path) + signingKey, _ := NewCommandContext(repo.Ctx, "config", "--get", "user.signingkey").RunInDir(repo.Path) gpgSettings.KeyID = strings.TrimSpace(signingKey) - defaultEmail, _ := NewCommand("config", "--get", "user.email").RunInDir(repo.Path) + defaultEmail, _ := NewCommandContext(repo.Ctx, "config", "--get", "user.email").RunInDir(repo.Path) gpgSettings.Email = strings.TrimSpace(defaultEmail) - defaultName, _ := NewCommand("config", "--get", "user.name").RunInDir(repo.Path) + defaultName, _ := NewCommandContext(repo.Ctx, "config", "--get", "user.name").RunInDir(repo.Path) gpgSettings.Name = strings.TrimSpace(defaultName) if err := gpgSettings.LoadPublicKeyContent(); err != nil { diff --git a/modules/git/repo_index.go b/modules/git/repo_index.go index 27cb7fbebe19..d08ddc8657a6 100644 --- a/modules/git/repo_index.go +++ b/modules/git/repo_index.go @@ -17,7 +17,7 @@ import ( // ReadTreeToIndex reads a treeish to the index func (repo *Repository) ReadTreeToIndex(treeish string, indexFilename ...string) error { if len(treeish) != 40 { - res, err := NewCommand("rev-parse", "--verify", treeish).RunInDir(repo.Path) + res, err := NewCommandContext(repo.Ctx, "rev-parse", "--verify", treeish).RunInDir(repo.Path) if err != nil { return err } @@ -37,7 +37,7 @@ func (repo *Repository) readTreeToIndex(id SHA1, indexFilename ...string) error if len(indexFilename) > 0 { env = append(os.Environ(), "GIT_INDEX_FILE="+indexFilename[0]) } - _, err := NewCommand("read-tree", id.String()).RunInDirWithEnv(repo.Path, env) + _, err := NewCommandContext(repo.Ctx, "read-tree", id.String()).RunInDirWithEnv(repo.Path, env) if err != nil { return err } @@ -67,13 +67,13 @@ func (repo *Repository) ReadTreeToTemporaryIndex(treeish string) (filename strin // EmptyIndex empties the index func (repo *Repository) EmptyIndex() error { - _, err := NewCommand("read-tree", "--empty").RunInDir(repo.Path) + _, err := NewCommandContext(repo.Ctx, "read-tree", "--empty").RunInDir(repo.Path) return err } // LsFiles checks if the given filenames are in the index func (repo *Repository) LsFiles(filenames ...string) ([]string, error) { - cmd := NewCommand("ls-files", "-z", "--") + cmd := NewCommandContext(repo.Ctx, "ls-files", "-z", "--") for _, arg := range filenames { if arg != "" { cmd.AddArguments(arg) @@ -93,7 +93,7 @@ func (repo *Repository) LsFiles(filenames ...string) ([]string, error) { // RemoveFilesFromIndex removes given filenames from the index - it does not check whether they are present. func (repo *Repository) RemoveFilesFromIndex(filenames ...string) error { - cmd := NewCommand("update-index", "--remove", "-z", "--index-info") + cmd := NewCommandContext(repo.Ctx, "update-index", "--remove", "-z", "--index-info") stdout := new(bytes.Buffer) stderr := new(bytes.Buffer) buffer := new(bytes.Buffer) @@ -109,14 +109,14 @@ func (repo *Repository) RemoveFilesFromIndex(filenames ...string) error { // AddObjectToIndex adds the provided object hash to the index at the provided filename func (repo *Repository) AddObjectToIndex(mode string, object SHA1, filename string) error { - cmd := NewCommand("update-index", "--add", "--replace", "--cacheinfo", mode, object.String(), filename) + cmd := NewCommandContext(repo.Ctx, "update-index", "--add", "--replace", "--cacheinfo", mode, object.String(), filename) _, err := cmd.RunInDir(repo.Path) return err } // WriteTree writes the current index as a tree to the object db and returns its hash func (repo *Repository) WriteTree() (*Tree, error) { - res, err := NewCommand("write-tree").RunInDir(repo.Path) + res, err := NewCommandContext(repo.Ctx, "write-tree").RunInDir(repo.Path) if err != nil { return nil, err } diff --git a/modules/git/repo_object.go b/modules/git/repo_object.go index f054c349029e..3921e6a1d41c 100644 --- a/modules/git/repo_object.go +++ b/modules/git/repo_object.go @@ -42,7 +42,7 @@ func (repo *Repository) HashObject(reader io.Reader) (SHA1, error) { } func (repo *Repository) hashObject(reader io.Reader) (string, error) { - cmd := NewCommand("hash-object", "-w", "--stdin") + cmd := NewCommandContext(repo.Ctx, "hash-object", "-w", "--stdin") stdout := new(bytes.Buffer) stderr := new(bytes.Buffer) err := cmd.RunInDirFullPipeline(repo.Path, stdout, stderr, reader) diff --git a/modules/git/repo_ref_nogogit.go b/modules/git/repo_ref_nogogit.go index ec0c5ec4cad3..790b717d384f 100644 --- a/modules/git/repo_ref_nogogit.go +++ b/modules/git/repo_ref_nogogit.go @@ -23,7 +23,7 @@ func (repo *Repository) GetRefsFiltered(pattern string) ([]*Reference, error) { go func() { stderrBuilder := &strings.Builder{} - err := NewCommand("for-each-ref").RunInDirPipeline(repo.Path, stdoutWriter, stderrBuilder) + err := NewCommandContext(repo.Ctx, "for-each-ref").RunInDirPipeline(repo.Path, stdoutWriter, stderrBuilder) if err != nil { _ = stdoutWriter.CloseWithError(ConcatenateError(err, stderrBuilder.String())) } else { diff --git a/modules/git/repo_stats.go b/modules/git/repo_stats.go index aca5ab21ccb2..caf2caabcc3b 100644 --- a/modules/git/repo_stats.go +++ b/modules/git/repo_stats.go @@ -39,7 +39,7 @@ func (repo *Repository) GetCodeActivityStats(fromTime time.Time, branch string) since := fromTime.Format(time.RFC3339) - stdout, err := NewCommand("rev-list", "--count", "--no-merges", "--branches=*", "--date=iso", fmt.Sprintf("--since='%s'", since)).RunInDirBytes(repo.Path) + stdout, err := NewCommandContext(repo.Ctx, "rev-list", "--count", "--no-merges", "--branches=*", "--date=iso", fmt.Sprintf("--since='%s'", since)).RunInDirBytes(repo.Path) if err != nil { return nil, err } @@ -67,7 +67,7 @@ func (repo *Repository) GetCodeActivityStats(fromTime time.Time, branch string) } stderr := new(strings.Builder) - err = NewCommand(args...).RunInDirTimeoutEnvFullPipelineFunc( + err = NewCommandContext(repo.Ctx, args...).RunInDirTimeoutEnvFullPipelineFunc( nil, -1, repo.Path, stdoutWriter, stderr, nil, func(ctx context.Context, cancel context.CancelFunc) error { diff --git a/modules/git/repo_tag.go b/modules/git/repo_tag.go index 44d7a048bc76..4262e0804f14 100644 --- a/modules/git/repo_tag.go +++ b/modules/git/repo_tag.go @@ -6,6 +6,7 @@ package git import ( + "context" "fmt" "strings" @@ -17,19 +18,19 @@ import ( const TagPrefix = "refs/tags/" // IsTagExist returns true if given tag exists in the repository. -func IsTagExist(repoPath, name string) bool { - return IsReferenceExist(repoPath, TagPrefix+name) +func IsTagExist(ctx context.Context, repoPath, name string) bool { + return IsReferenceExist(ctx, repoPath, TagPrefix+name) } // CreateTag create one tag in the repository func (repo *Repository) CreateTag(name, revision string) error { - _, err := NewCommand("tag", "--", name, revision).RunInDir(repo.Path) + _, err := NewCommandContext(repo.Ctx, "tag", "--", name, revision).RunInDir(repo.Path) return err } // CreateAnnotatedTag create one annotated tag in the repository func (repo *Repository) CreateAnnotatedTag(name, message, revision string) error { - _, err := NewCommand("tag", "-a", "-m", message, "--", name, revision).RunInDir(repo.Path) + _, err := NewCommandContext(repo.Ctx, "tag", "-a", "-m", message, "--", name, revision).RunInDir(repo.Path) return err } @@ -79,7 +80,7 @@ func (repo *Repository) getTag(tagID SHA1, name string) (*Tag, error) { } // The tag is an annotated tag with a message. - data, err := NewCommand("cat-file", "-p", tagID.String()).RunInDirBytes(repo.Path) + data, err := NewCommandContext(repo.Ctx, "cat-file", "-p", tagID.String()).RunInDirBytes(repo.Path) if err != nil { return nil, err } @@ -104,7 +105,7 @@ func (repo *Repository) GetTagNameBySHA(sha string) (string, error) { return "", fmt.Errorf("SHA is too short: %s", sha) } - stdout, err := NewCommand("show-ref", "--tags", "-d").RunInDir(repo.Path) + stdout, err := NewCommandContext(repo.Ctx, "show-ref", "--tags", "-d").RunInDir(repo.Path) if err != nil { return "", err } @@ -127,7 +128,7 @@ func (repo *Repository) GetTagNameBySHA(sha string) (string, error) { // GetTagID returns the object ID for a tag (annotated tags have both an object SHA AND a commit SHA) func (repo *Repository) GetTagID(name string) (string, error) { - stdout, err := NewCommand("show-ref", "--tags", "--", name).RunInDir(repo.Path) + stdout, err := NewCommandContext(repo.Ctx, "show-ref", "--tags", "--", name).RunInDir(repo.Path) if err != nil { return "", err } @@ -163,7 +164,7 @@ func (repo *Repository) GetTag(name string) (*Tag, error) { // GetTagInfos returns all tag infos of the repository. func (repo *Repository) GetTagInfos(page, pageSize int) ([]*Tag, int, error) { // TODO this a slow implementation, makes one git command per tag - stdout, err := NewCommand("tag").RunInDir(repo.Path) + stdout, err := NewCommandContext(repo.Ctx, "tag").RunInDir(repo.Path) if err != nil { return nil, 0, err } @@ -196,7 +197,7 @@ func (repo *Repository) GetTagInfos(page, pageSize int) ([]*Tag, int, error) { // GetTagType gets the type of the tag, either commit (simple) or tag (annotated) func (repo *Repository) GetTagType(id SHA1) (string, error) { // Get tag type - stdout, err := NewCommand("cat-file", "-t", id.String()).RunInDir(repo.Path) + stdout, err := NewCommandContext(repo.Ctx, "cat-file", "-t", id.String()).RunInDir(repo.Path) if err != nil { return "", err } diff --git a/modules/git/repo_tag_nogogit.go b/modules/git/repo_tag_nogogit.go index 172b6fd66cbb..1a23755aa666 100644 --- a/modules/git/repo_tag_nogogit.go +++ b/modules/git/repo_tag_nogogit.go @@ -20,6 +20,6 @@ func (repo *Repository) IsTagExist(name string) bool { // GetTags returns all tags of the repository. // returning at most limit tags, or all if limit is 0. func (repo *Repository) GetTags(skip, limit int) (tags []string, err error) { - tags, _, err = callShowRef(repo.Path, TagPrefix, "--tags", skip, limit) + tags, _, err = callShowRef(repo.Ctx, repo.Path, TagPrefix, "--tags", skip, limit) return } diff --git a/modules/git/repo_tree.go b/modules/git/repo_tree.go index 2053b6a1de17..f57c26ffee95 100644 --- a/modules/git/repo_tree.go +++ b/modules/git/repo_tree.go @@ -40,7 +40,7 @@ func (repo *Repository) CommitTree(author *Signature, committer *Signature, tree "GIT_COMMITTER_EMAIL="+committer.Email, "GIT_COMMITTER_DATE="+commitTimeStr, ) - cmd := NewCommand("commit-tree", tree.ID.String()) + cmd := NewCommandContext(repo.Ctx, "commit-tree", tree.ID.String()) for _, parent := range opts.Parents { cmd.AddArguments("-p", parent) diff --git a/modules/git/repo_tree_gogit.go b/modules/git/repo_tree_gogit.go index 2ddffcf79b67..5a90cbe802ef 100644 --- a/modules/git/repo_tree_gogit.go +++ b/modules/git/repo_tree_gogit.go @@ -22,7 +22,7 @@ func (repo *Repository) getTree(id SHA1) (*Tree, error) { // GetTree find the tree object in the repository. func (repo *Repository) GetTree(idStr string) (*Tree, error) { if len(idStr) != 40 { - res, err := NewCommand("rev-parse", "--verify", idStr).RunInDir(repo.Path) + res, err := NewCommandContext(repo.Ctx, "rev-parse", "--verify", idStr).RunInDir(repo.Path) if err != nil { return nil, err } diff --git a/modules/repository/branch.go b/modules/repository/branch.go index 275bae91e3f9..2ef676c12f07 100644 --- a/modules/repository/branch.go +++ b/modules/repository/branch.go @@ -73,7 +73,7 @@ func CreateNewBranch(doer *models.User, repo *models.Repository, oldBranchName, return err } - if !git.IsBranchExist(repo.RepoPath(), oldBranchName) { + if !git.IsBranchExist(git.DefaultContext, repo.RepoPath(), oldBranchName) { return models.ErrBranchDoesNotExist{ BranchName: oldBranchName, } diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index c57075e3b84c..db36ae410a3a 100644 --- a/routers/api/v1/repo/branch.go +++ b/routers/api/v1/repo/branch.go @@ -405,7 +405,7 @@ func CreateBranchProtection(ctx *context.APIContext) { repo := ctx.Repo.Repository // Currently protection must match an actual branch - if !git.IsBranchExist(ctx.Repo.Repository.RepoPath(), form.BranchName) { + if !git.IsBranchExist(ctx.Req.Context(), ctx.Repo.Repository.RepoPath(), form.BranchName) { ctx.NotFound() return } diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 1aaa27c2b00e..b1f16bbf13c5 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -1573,7 +1573,7 @@ func ViewIssue(ctx *context.Context) { } ctx.Data["IsPullBranchDeletable"] = canDelete && pull.HeadRepo != nil && - git.IsBranchExist(pull.HeadRepo.RepoPath(), pull.HeadBranch) && + git.IsBranchExist(ctx, pull.HeadRepo.RepoPath(), pull.HeadBranch) && (!pull.HasMerged || ctx.Data["HeadBranchCommitID"] == ctx.Data["PullHeadCommitID"]) stillCanManualMerge := func() bool { diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index dde556135177..407e5d990b55 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -434,7 +434,7 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare if pull.Flow == models.PullRequestFlowGithub { headBranchExist = headGitRepo.IsBranchExist(pull.HeadBranch) } else { - headBranchExist = git.IsReferenceExist(baseGitRepo.Path, pull.GetGitRefName()) + headBranchExist = git.IsReferenceExist(ctx, baseGitRepo.Path, pull.GetGitRefName()) } if headBranchExist { diff --git a/services/pull/commit_status.go b/services/pull/commit_status.go index f1f351138b8e..2b834c25f1c8 100644 --- a/services/pull/commit_status.go +++ b/services/pull/commit_status.go @@ -112,7 +112,7 @@ func GetPullRequestCommitStatusState(pr *models.PullRequest) (structs.CommitStat if pr.Flow == models.PullRequestFlowGithub && !headGitRepo.IsBranchExist(pr.HeadBranch) { return "", errors.New("Head branch does not exist, can not merge") } - if pr.Flow == models.PullRequestFlowAGit && !git.IsReferenceExist(headGitRepo.Path, pr.GetGitRefName()) { + if pr.Flow == models.PullRequestFlowAGit && !git.IsReferenceExist(headGitRepo.Ctx, headGitRepo.Path, pr.GetGitRefName()) { return "", errors.New("Head branch does not exist, can not merge") } diff --git a/services/pull/pull.go b/services/pull/pull.go index f7d154cfd023..3b9e70c45c42 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -312,7 +312,7 @@ func AddTestPullRequestTask(doer *models.User, repoID int64, branch string, isSy for _, pr := range prs { divergence, err := GetDiverging(pr) if err != nil { - if models.IsErrBranchDoesNotExist(err) && !git.IsBranchExist(pr.HeadRepo.RepoPath(), pr.HeadBranch) { + if models.IsErrBranchDoesNotExist(err) && !git.IsBranchExist(ctx, pr.HeadRepo.RepoPath(), pr.HeadBranch) { log.Warn("Cannot test PR %s/%d: head_branch %s no longer exists", pr.BaseRepo.Name, pr.IssueID, pr.HeadBranch) } else { log.Error("GetDiverging: %v", err) diff --git a/services/pull/temp_repo.go b/services/pull/temp_repo.go index 54d09c815852..e30dba7add60 100644 --- a/services/pull/temp_repo.go +++ b/services/pull/temp_repo.go @@ -152,7 +152,7 @@ func createTemporaryRepo(pr *models.PullRequest) (string, error) { if err := models.RemoveTemporaryPath(tmpBasePath); err != nil { log.Error("CreateTempRepo: RemoveTemporaryPath: %s", err) } - if !git.IsBranchExist(pr.HeadRepo.RepoPath(), pr.HeadBranch) { + if !git.IsBranchExist(git.DefaultContext, pr.HeadRepo.RepoPath(), pr.HeadBranch) { return "", models.ErrBranchDoesNotExist{ BranchName: pr.HeadBranch, } diff --git a/services/wiki/wiki.go b/services/wiki/wiki.go index 5acb23ac78a2..9859e8b8b51b 100644 --- a/services/wiki/wiki.go +++ b/services/wiki/wiki.go @@ -124,7 +124,7 @@ func updateWikiPage(doer *models.User, repo *models.Repository, oldWikiName, new return fmt.Errorf("InitWiki: %v", err) } - hasMasterBranch := git.IsBranchExist(repo.WikiPath(), "master") + hasMasterBranch := git.IsBranchExist(git.DefaultContext, repo.WikiPath(), "master") basePath, err := models.CreateTemporaryPath("update-wiki") if err != nil { From 08b77d1a2236176bcd141888697566114f5d97e3 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 13 Oct 2021 17:15:20 +0100 Subject: [PATCH 09/26] fix lint Signed-off-by: Andrew Thornton --- routers/api/v1/repo/branch.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index db36ae410a3a..f8209ee9978d 100644 --- a/routers/api/v1/repo/branch.go +++ b/routers/api/v1/repo/branch.go @@ -405,7 +405,7 @@ func CreateBranchProtection(ctx *context.APIContext) { repo := ctx.Repo.Repository // Currently protection must match an actual branch - if !git.IsBranchExist(ctx.Req.Context(), ctx.Repo.Repository.RepoPath(), form.BranchName) { + if !git.IsBranchExist(ctx.Req.Context(), ctx.Repo.Repository.RepoPath(), form.BranchName) { ctx.NotFound() return } From 9bb820bb2d337941bea33314eb1bd026b8d0cf5a Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Fri, 15 Oct 2021 05:05:09 +0100 Subject: [PATCH 10/26] Simplify PID to strings using the time of start plus/minus a counter Signed-off-by: Andrew Thornton --- modules/process/context.go | 12 ++--- modules/process/manager.go | 78 +++++++++++++-------------------- modules/process/manager_test.go | 6 +-- routers/web/admin/admin.go | 4 +- 4 files changed, 42 insertions(+), 58 deletions(-) diff --git a/modules/process/context.go b/modules/process/context.go index 93c84ff884bd..f8d18bc3645d 100644 --- a/modules/process/context.go +++ b/modules/process/context.go @@ -11,11 +11,11 @@ import ( // Context is a wrapper around context.Context for having the current pid for this context type Context struct { context.Context - pid int64 + pid IDType } // GetPID returns the PID for this context -func (c *Context) GetPID() int64 { +func (c *Context) GetPID() IDType { return c.pid } @@ -51,17 +51,17 @@ func GetContext(ctx context.Context) *Context { } // GetPID returns the PID for this context -func GetPID(ctx context.Context) int64 { +func GetPID(ctx context.Context) IDType { pCtx := GetContext(ctx) if pCtx == nil { - return 0 + return "" } return pCtx.GetPID() } // GetParentPID returns the ParentPID for this context -func GetParentPID(ctx context.Context) int64 { - parentPID := int64(0) +func GetParentPID(ctx context.Context) IDType { + var parentPID IDType if parentProcess := GetContext(ctx); parentProcess != nil { parentPID = parentProcess.GetPID() } diff --git a/modules/process/manager.go b/modules/process/manager.go index 117637bc8e32..2b9ff8031d5c 100644 --- a/modules/process/manager.go +++ b/modules/process/manager.go @@ -12,6 +12,7 @@ import ( "io" "os/exec" "sort" + "strconv" "sync" "time" ) @@ -26,18 +27,15 @@ var ( // DefaultContext is the default context to run processing commands in DefaultContext = context.Background() - - // RecyclePID is the PID number at which we will attempt to recycle PIDs - RecyclePID int64 = 1 << 16 - - // HuntSize is the size of the hunt for the lowest free PID - HuntSize int64 = 512 ) +// IDType is a pid type +type IDType string + // Process represents a working process inheriting from Gitea. type Process struct { - PID int64 // Process ID, not system one. - ParentPID int64 + PID IDType // Process ID, not system one. + ParentPID IDType children []*Process Description string Start time.Time @@ -59,19 +57,18 @@ func (p *Process) Children() []*Process { type Manager struct { mutex sync.Mutex - next int64 - low int64 + next int64 + lastTime time.Time - processes map[int64]*Process + processes map[IDType]*Process } // GetManager returns a Manager and initializes one as singleton if there's none yet func GetManager() *Manager { managerInit.Do(func() { manager = &Manager{ - processes: make(map[int64]*Process), + processes: make(map[IDType]*Process), next: 1, - low: 1, } }) return manager @@ -106,20 +103,20 @@ func (pm *Manager) AddContextTimeout(parent context.Context, timeout time.Durati } // Add create a new process -func (pm *Manager) Add(parentPID int64, description string, cancel context.CancelFunc) (int64, context.CancelFunc) { +func (pm *Manager) Add(parentPID IDType, description string, cancel context.CancelFunc) (IDType, context.CancelFunc) { pm.mutex.Lock() - pid := pm.nextPID() + start, pid := pm.nextPID() parent := pm.processes[parentPID] if parent == nil { - parentPID = 0 + parentPID = "" } process := &Process{ PID: pid, ParentPID: parentPID, Description: description, - Start: time.Now(), + Start: start, Cancel: cancel, } @@ -138,38 +135,26 @@ func (pm *Manager) Add(parentPID int64, description string, cancel context.Cance } // nextPID will return the next available PID. pm.mutex should already be locked. -func (pm *Manager) nextPID() int64 { - if pm.next > RecyclePID { - for i := int64(0); i < HuntSize; i++ { - if pm.low >= pm.next { - pm.low = 1 - break - } - if _, ok := pm.processes[pm.low]; !ok { - next := pm.low - pm.low++ - return next - } - pm.low++ - } +func (pm *Manager) nextPID() (start time.Time, pid IDType) { + start = time.Now() + if pm.lastTime == start { + pm.next++ + } else { + pm.next = 1 } - next := pm.next - pm.next++ - return next -} + pid = IDType(strconv.FormatInt(start.Unix(), 16)) -// releasePID will release the PID. pm.mutex should already be locked. -func (pm *Manager) releasePID(pid int64) { - if pid < pm.low+RecyclePID { - pm.low = pid + if pm.next == 1 { + return } + pid += IDType("-" + strconv.FormatInt(pm.next, 10)) + return } // Remove a process from the ProcessManager. -func (pm *Manager) Remove(pid int64) { +func (pm *Manager) Remove(pid IDType) { pm.mutex.Lock() delete(pm.processes, pid) - pm.releasePID(pid) pm.mutex.Unlock() } @@ -178,9 +163,8 @@ func (pm *Manager) remove(process *Process) { defer pm.mutex.Unlock() if p := pm.processes[process.PID]; p == process { delete(pm.processes, process.PID) - pm.releasePID(process.PID) for _, child := range process.children { - child.ParentPID = 0 + child.ParentPID = "" } parent := pm.processes[process.ParentPID] if parent != nil { @@ -195,7 +179,7 @@ func (pm *Manager) remove(process *Process) { } // Cancel a process in the ProcessManager. -func (pm *Manager) Cancel(pid int64) { +func (pm *Manager) Cancel(pid IDType) { pm.mutex.Lock() process, ok := pm.processes[pid] pm.mutex.Unlock() @@ -210,7 +194,7 @@ func (pm *Manager) Processes(onlyRoots bool) []*Process { processes := make([]*Process, 0, len(pm.processes)) if onlyRoots { for _, process := range pm.processes { - if process.ParentPID == 0 { + if process.ParentPID == "" { processes = append(processes, process) } } @@ -299,7 +283,7 @@ func (pm *Manager) ExecDirEnvStdIn(timeout time.Duration, dir, desc string, env // Error is a wrapped error describing the error results of Process Execution type Error struct { - PID int64 + PID IDType Description string Err error CtxErr error @@ -308,7 +292,7 @@ type Error struct { } func (err *Error) Error() string { - return fmt.Sprintf("exec(%d:%s) failed: %v(%v) stdout: %s stderr: %s", err.PID, err.Description, err.Err, err.CtxErr, err.Stdout, err.Stderr) + return fmt.Sprintf("exec(%s:%s) failed: %v(%v) stdout: %s stderr: %s", err.PID, err.Description, err.Err, err.CtxErr, err.Stdout, err.Stderr) } // Unwrap implements the unwrappable implicit interface for go1.13 Unwrap() diff --git a/modules/process/manager_test.go b/modules/process/manager_test.go index 60474b3fef62..8baf993de3d2 100644 --- a/modules/process/manager_test.go +++ b/modules/process/manager_test.go @@ -22,7 +22,7 @@ func TestGetManager(t *testing.T) { } func TestManager_AddContext(t *testing.T) { - pm := Manager{processes: make(map[int64]*Process), next: 1, low: 1} + pm := Manager{processes: make(map[IDType]*Process), next: 1} ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -39,7 +39,7 @@ func TestManager_AddContext(t *testing.T) { } func TestManager_Cancel(t *testing.T) { - pm := Manager{processes: make(map[int64]*Process), next: 1, low: 1} + pm := Manager{processes: make(map[IDType]*Process), next: 1} ctx, _, remove := pm.AddContext(context.Background(), "foo") defer remove() @@ -67,7 +67,7 @@ func TestManager_Cancel(t *testing.T) { } func TestManager_Remove(t *testing.T) { - pm := Manager{processes: make(map[int64]*Process), next: 1, low: 1} + pm := Manager{processes: make(map[IDType]*Process), next: 1} ctx, cancel := context.WithCancel(context.Background()) defer cancel() diff --git a/routers/web/admin/admin.go b/routers/web/admin/admin.go index 0b98987be51c..036218a356ed 100644 --- a/routers/web/admin/admin.go +++ b/routers/web/admin/admin.go @@ -331,8 +331,8 @@ func Monitor(ctx *context.Context) { // MonitorCancel cancels a process func MonitorCancel(ctx *context.Context) { - pid := ctx.ParamsInt64("pid") - process.GetManager().Cancel(pid) + pid := ctx.Params("pid") + process.GetManager().Cancel(process.IDType(pid)) ctx.JSON(http.StatusOK, map[string]interface{}{ "redirect": setting.AppSubURL + "/admin/monitor", }) From 9895680f4a8ff01f53e1431aedf793298118d004 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Fri, 15 Oct 2021 05:46:54 +0100 Subject: [PATCH 11/26] extract process out of manager.go Signed-off-by: Andrew Thornton --- modules/process/manager.go | 50 +++++++---------------------- modules/process/process.go | 66 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 38 deletions(-) create mode 100644 modules/process/process.go diff --git a/modules/process/manager.go b/modules/process/manager.go index 2b9ff8031d5c..777451af636e 100644 --- a/modules/process/manager.go +++ b/modules/process/manager.go @@ -32,27 +32,6 @@ var ( // IDType is a pid type type IDType string -// Process represents a working process inheriting from Gitea. -type Process struct { - PID IDType // Process ID, not system one. - ParentPID IDType - children []*Process - Description string - Start time.Time - Cancel context.CancelFunc -} - -// Children gets the children of the process -func (p *Process) Children() []*Process { - var children []*Process - pm := GetManager() - pm.mutex.Lock() - defer pm.mutex.Unlock() - children = make([]*Process, len(p.children)) - copy(children, p.children) - return children -} - // Manager knows about all processes and counts PIDs. type Manager struct { mutex sync.Mutex @@ -74,7 +53,7 @@ func GetManager() *Manager { return manager } -// AddContext create a new context and add it as a process. The CancelFunc must always be called even if the context is Done() +// AddContext create a new context and add it as a process. The remove CancelFunc must always be called even if the context is Done() func (pm *Manager) AddContext(parent context.Context, description string) (ctx context.Context, cancel, remove context.CancelFunc) { parentPID := GetParentPID(parent) @@ -88,7 +67,7 @@ func (pm *Manager) AddContext(parent context.Context, description string) (ctx c }, cancel, remove } -// AddContextTimeout create a new context and add it as a process +// AddContextTimeout create a new context and add it as a process. The remove CancelFunc must always be called even if the context is Done() func (pm *Manager) AddContextTimeout(parent context.Context, timeout time.Duration, description string) (ctx context.Context, cancel, remove context.CancelFunc) { parentPID := GetParentPID(parent) @@ -126,7 +105,7 @@ func (pm *Manager) Add(parentPID IDType, description string, cancel context.Canc } if parent != nil { - parent.children = append(parent.children, process) + parent.AddChild(process) } pm.processes[pid] = process pm.mutex.Unlock() @@ -160,22 +139,17 @@ func (pm *Manager) Remove(pid IDType) { func (pm *Manager) remove(process *Process) { pm.mutex.Lock() - defer pm.mutex.Unlock() if p := pm.processes[process.PID]; p == process { delete(pm.processes, process.PID) - for _, child := range process.children { - child.ParentPID = "" - } - parent := pm.processes[process.ParentPID] - if parent != nil { - for i, child := range parent.children { - if child == process { - parent.children = append(parent.children[:i], parent.children[i+1:]...) - return - } - } - } } + parent := pm.processes[process.ParentPID] + pm.mutex.Unlock() + + if parent == nil { + return + } + + parent.RemoveChild(process) } // Cancel a process in the ProcessManager. @@ -194,7 +168,7 @@ func (pm *Manager) Processes(onlyRoots bool) []*Process { processes := make([]*Process, 0, len(pm.processes)) if onlyRoots { for _, process := range pm.processes { - if process.ParentPID == "" { + if _, has := pm.processes[process.ParentPID]; !has { processes = append(processes, process) } } diff --git a/modules/process/process.go b/modules/process/process.go new file mode 100644 index 000000000000..662f878d7f3d --- /dev/null +++ b/modules/process/process.go @@ -0,0 +1,66 @@ +// Copyright 2021 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 process + +import ( + "context" + "sync" + "time" +) + +// Process represents a working process inheriting from Gitea. +type Process struct { + PID IDType // Process ID, not system one. + ParentPID IDType + Description string + Start time.Time + Cancel context.CancelFunc + + lock sync.Mutex + children []*Process +} + +// Children gets the children of the process +// Note: this function will behave nicely even if p is nil +func (p *Process) Children() (children []*Process) { + if p == nil { + return + } + + p.lock.Lock() + defer p.lock.Unlock() + children = make([]*Process, len(p.children)) + copy(children, p.children) + return children +} + +// AddChild adds a child process +// Note: this function will behave nicely even if p is nil +func (p *Process) AddChild(child *Process) { + if p == nil { + return + } + + p.lock.Lock() + defer p.lock.Unlock() + p.children = append(p.children, child) +} + +// RemoveChild removes a child process +// Note: this function will behave nicely even if p is nil +func (p *Process) RemoveChild(process *Process) { + if p == nil { + return + } + + p.lock.Lock() + defer p.lock.Unlock() + for i, child := range p.children { + if child == process { + p.children = append(p.children[:i], p.children[i+1:]...) + return + } + } +} From afc5b417c14628331e81235662160e34c46a4b9b Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Fri, 15 Oct 2021 06:06:41 +0100 Subject: [PATCH 12/26] fix test Signed-off-by: Andrew Thornton --- modules/process/manager.go | 8 +++++--- modules/process/manager_test.go | 12 +++++++----- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/modules/process/manager.go b/modules/process/manager.go index 777451af636e..15bef5463e4c 100644 --- a/modules/process/manager.go +++ b/modules/process/manager.go @@ -37,7 +37,7 @@ type Manager struct { mutex sync.Mutex next int64 - lastTime time.Time + lastTime int64 processes map[IDType]*Process } @@ -116,17 +116,19 @@ func (pm *Manager) Add(parentPID IDType, description string, cancel context.Canc // nextPID will return the next available PID. pm.mutex should already be locked. func (pm *Manager) nextPID() (start time.Time, pid IDType) { start = time.Now() - if pm.lastTime == start { + startUnix := start.Unix() + if pm.lastTime == startUnix { pm.next++ } else { pm.next = 1 } + pm.lastTime = startUnix pid = IDType(strconv.FormatInt(start.Unix(), 16)) if pm.next == 1 { return } - pid += IDType("-" + strconv.FormatInt(pm.next, 10)) + pid = IDType(string(pid) + "-" + strconv.FormatInt(pm.next, 10)) return } diff --git a/modules/process/manager_test.go b/modules/process/manager_test.go index 8baf993de3d2..8493776bb583 100644 --- a/modules/process/manager_test.go +++ b/modules/process/manager_test.go @@ -29,13 +29,15 @@ func TestManager_AddContext(t *testing.T) { p1Ctx, _, remove := pm.AddContext(ctx, "foo") defer remove() - assert.Equal(t, int64(1), GetContext(p1Ctx).GetPID(), "expected to get pid 1 got %d", GetContext(p1Ctx).GetPID()) + assert.NotEmpty(t, GetContext(p1Ctx).GetPID(), "expected to get non-empty pid") p2Ctx, _, remove := pm.AddContext(p1Ctx, "bar") defer remove() - assert.Equal(t, int64(2), GetContext(p2Ctx).GetPID(), "expected to get pid 2 got %d", GetContext(p2Ctx).GetPID()) - assert.Equal(t, int64(1), GetContext(p2Ctx).GetParent().GetPID(), "expected to get pid 1 got %d", GetContext(p2Ctx).GetParent().GetPID()) + assert.NotEmpty(t, GetContext(p2Ctx).GetPID(), "expected to get non-empty pid") + + assert.NotEqual(t, GetContext(p1Ctx).GetPID(), GetContext(p2Ctx).GetPID(), "expected to get different pids %s == %s", GetContext(p2Ctx).GetPID(), GetContext(p1Ctx).GetPID()) + assert.Equal(t, GetContext(p1Ctx).GetPID(), GetContext(p2Ctx).GetParent().GetPID(), "expected to get pid %s got %s", GetContext(p1Ctx).GetPID(), GetContext(p2Ctx).GetParent().GetPID()) } func TestManager_Cancel(t *testing.T) { @@ -74,12 +76,12 @@ func TestManager_Remove(t *testing.T) { p1Ctx, _, remove := pm.AddContext(ctx, "foo") defer remove() - assert.Equal(t, int64(1), GetContext(p1Ctx).GetPID(), "expected to get pid 1 got %d", GetContext(p1Ctx).GetPID()) + assert.NotEmpty(t, GetContext(p1Ctx).GetPID(), "expected to have non-empty PID") p2Ctx, _, remove := pm.AddContext(p1Ctx, "bar") defer remove() - assert.Equal(t, int64(2), GetContext(p2Ctx).GetPID(), "expected to get pid 2 got %d", GetContext(p2Ctx).GetPID()) + assert.NotEqual(t, GetContext(p1Ctx).GetPID(), GetContext(p2Ctx).GetPID(), "expected to get different pids got %s == %s", GetContext(p2Ctx).GetPID(), GetContext(p1Ctx).GetPID()) pm.Remove(GetPID(p2Ctx)) From 4ce4614a3c88b94a0c4bef99f2de3c57fb0c0ec2 Mon Sep 17 00:00:00 2001 From: zeripath Date: Sun, 17 Oct 2021 12:43:25 +0100 Subject: [PATCH 13/26] Make the Mirror Queue a queue (#17326) Convert the old mirror syncing queue to the more modern queue format. Fix a bug in the from the repo-archive queue PR - the assumption was made that uniqueness could be enforced with by checking equality in a map in channel unique queues - however this only works for primitive types - which was the initial intention but is an imperfect. This is fixed by marshalling the data and placing the martialled data in the unique map instead. The documentation is also updated to add information about the deprecated configuration values. Signed-off-by: Andrew Thornton --- custom/conf/app.example.ini | 4 +- .../doc/advanced/config-cheat-sheet.en-us.md | 39 +++++++- modules/queue/unique_queue_channel.go | 29 ++++-- modules/setting/mailer.go | 2 - modules/setting/queue.go | 64 +++++++----- modules/setting/repository.go | 4 - services/mirror/mirror.go | 98 ++++++++++++------- 7 files changed, 164 insertions(+), 76 deletions(-) diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index bdc42480e443..1753ed233070 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -769,10 +769,10 @@ PATH = ;; Global limit of repositories per user, applied at creation time. -1 means no limit ;MAX_CREATION_LIMIT = -1 ;; -;; Mirror sync queue length, increase if mirror syncing starts hanging +;; Mirror sync queue length, increase if mirror syncing starts hanging (DEPRECATED: please use [queue.mirror] LENGTH instead) ;MIRROR_QUEUE_LENGTH = 1000 ;; -;; Patch test queue length, increase if pull request patch testing starts hanging +;; Patch test queue length, increase if pull request patch testing starts hanging (DEPRECATED: please use [queue.pr_patch_checker] LENGTH instead) ;PULL_REQUEST_QUEUE_LENGTH = 1000 ;; ;; Preferred Licenses to place at the top of the List diff --git a/docs/content/doc/advanced/config-cheat-sheet.en-us.md b/docs/content/doc/advanced/config-cheat-sheet.en-us.md index 251f6bd51a9d..91c62dbec34a 100644 --- a/docs/content/doc/advanced/config-cheat-sheet.en-us.md +++ b/docs/content/doc/advanced/config-cheat-sheet.en-us.md @@ -54,10 +54,10 @@ Values containing `#` or `;` must be quoted using `` ` `` or `"""`. - `DEFAULT_PUSH_CREATE_PRIVATE`: **true**: Default private when creating a new repository with push-to-create. - `MAX_CREATION_LIMIT`: **-1**: Global maximum creation limit of repositories per user, `-1` means no limit. -- `PULL_REQUEST_QUEUE_LENGTH`: **1000**: Length of pull request patch test queue, make it +- `PULL_REQUEST_QUEUE_LENGTH`: **1000**: Length of pull request patch test queue, make it. **DEPRECATED** use `LENGTH` in `[queue.pr_patch_checker]`. as large as possible. Use caution when editing this value. - `MIRROR_QUEUE_LENGTH`: **1000**: Patch test queue length, increase if pull request patch - testing starts hanging. + testing starts hanging. **DEPRECATED** use `LENGTH` in `[queue.mirror]`. - `PREFERRED_LICENSES`: **Apache License 2.0,MIT License**: Preferred Licenses to place at the top of the list. Name must match file name in options/license or custom/options/license. - `DISABLE_HTTP_GIT`: **false**: Disable the ability to interact with repositories over the @@ -382,6 +382,8 @@ relation to port exhaustion. ## Queue (`queue` and `queue.*`) +Configuration at `[queue]` will set defaults for queues with overrides for individual queues at `[queue.*]`. (However see below.) + - `TYPE`: **persistable-channel**: General queue type, currently support: `persistable-channel` (uses a LevelDB internally), `channel`, `level`, `redis`, `dummy` - `DATADIR`: **queues/**: Base DataDir for storing persistent and level queues. `DATADIR` for individual queues can be set in `queue.name` sections but will default to `DATADIR/`**`common`**. (Previously each queue would default to `DATADIR/`**`name`**.) - `LENGTH`: **20**: Maximal queue size before channel queues block @@ -400,6 +402,37 @@ relation to port exhaustion. - `BOOST_TIMEOUT`: **5m**: Boost workers will timeout after this long. - `BOOST_WORKERS`: **1** (v1.14 and before: **5**): This many workers will be added to the worker pool if there is a boost. +Gitea creates the following non-unique queues: + +- `code_indexer` +- `issue_indexer` +- `notification-service` +- `task` +- `mail` +- `push_update` + +And the following unique queues: + +- `repo_stats_update` +- `repo-archive` +- `mirror` +- `pr_patch_checker` + +Certain queues have defaults that override the defaults set in `[queue]` (this occurs mostly to support older configuration): + +- `[queue.issue_indexer]` + - `TYPE` this will default to `[queue]` `TYPE` if it is set but if not it will appropriately convert `[indexer]` `ISSUE_INDEXER_QUEUE_TYPE` if that is set. + - `LENGTH` will default to `[indexer]` `UPDATE_BUFFER_LEN` if that is set. + - `BATCH_LENGTH` will default to `[indexer]` `ISSUE_INDEXER_QUEUE_BATCH_NUMBER` if that is set. + - `DATADIR` will default to `[indexer]` `ISSUE_INDEXER_QUEUE_DIR` if that is set. + - `CONN_STR` will default to `[indexer]` `ISSUE_INDEXER_QUEUE_CONN_STR` if that is set. +- `[queue.mailer]` + - `LENGTH` will default to **100** or whatever `[mailer]` `SEND_BUFFER_LEN` is. +- `[queue.pr_patch_checker]` + - `LENGTH` will default to **1000** or whatever `[repository]` `PULL_REQUEST_QUEUE_LENGTH` is. +- `[queue.mirror]` + - `LENGTH` will default to **1000** or whatever `[repository]` `MIRROR_QUEUE_LENGTH` is. + ## Admin (`admin`) - `DEFAULT_EMAIL_NOTIFICATIONS`: **enabled**: Default configuration for email notifications for users (user configurable). Options: enabled, onmention, disabled @@ -588,7 +621,7 @@ Define allowed algorithms and their minimum key length (use -1 to disable a type command or full path). - `SENDMAIL_ARGS`: **_empty_**: Specify any extra sendmail arguments. - `SENDMAIL_TIMEOUT`: **5m**: default timeout for sending email through sendmail -- `SEND_BUFFER_LEN`: **100**: Buffer length of mailing queue. +- `SEND_BUFFER_LEN`: **100**: Buffer length of mailing queue. **DEPRECATED** use `LENGTH` in `[queue.mailer]` ## Cache (`cache`) diff --git a/modules/queue/unique_queue_channel.go b/modules/queue/unique_queue_channel.go index 5bec67c4d355..f617595c0438 100644 --- a/modules/queue/unique_queue_channel.go +++ b/modules/queue/unique_queue_channel.go @@ -9,6 +9,7 @@ import ( "fmt" "sync" + "code.gitea.io/gitea/modules/json" "code.gitea.io/gitea/modules/log" ) @@ -29,7 +30,7 @@ type ChannelUniqueQueueConfiguration ChannelQueueConfiguration type ChannelUniqueQueue struct { *WorkerPool lock sync.Mutex - table map[Data]bool + table map[string]bool shutdownCtx context.Context shutdownCtxCancel context.CancelFunc terminateCtx context.Context @@ -54,7 +55,7 @@ func NewChannelUniqueQueue(handle HandlerFunc, cfg, exemplar interface{}) (Queue shutdownCtx, shutdownCtxCancel := context.WithCancel(terminateCtx) queue := &ChannelUniqueQueue{ - table: map[Data]bool{}, + table: map[string]bool{}, shutdownCtx: shutdownCtx, shutdownCtxCancel: shutdownCtxCancel, terminateCtx: terminateCtx, @@ -65,9 +66,13 @@ func NewChannelUniqueQueue(handle HandlerFunc, cfg, exemplar interface{}) (Queue } queue.WorkerPool = NewWorkerPool(func(data ...Data) { for _, datum := range data { + // No error is possible here because PushFunc ensures that this can be marshalled + bs, _ := json.Marshal(datum) + queue.lock.Lock() - delete(queue.table, datum) + delete(queue.table, string(bs)) queue.lock.Unlock() + handle(datum) } }, config.WorkerPoolConfiguration) @@ -94,6 +99,11 @@ func (q *ChannelUniqueQueue) PushFunc(data Data, fn func() error) error { if !assignableTo(data, q.exemplar) { return fmt.Errorf("Unable to assign data: %v to same type as exemplar: %v in queue: %s", data, q.exemplar, q.name) } + + bs, err := json.Marshal(data) + if err != nil { + return err + } q.lock.Lock() locked := true defer func() { @@ -101,16 +111,16 @@ func (q *ChannelUniqueQueue) PushFunc(data Data, fn func() error) error { q.lock.Unlock() } }() - if _, ok := q.table[data]; ok { + if _, ok := q.table[string(bs)]; ok { return ErrAlreadyInQueue } // FIXME: We probably need to implement some sort of limit here // If the downstream queue blocks this table will grow without limit - q.table[data] = true + q.table[string(bs)] = true if fn != nil { err := fn() if err != nil { - delete(q.table, data) + delete(q.table, string(bs)) return err } } @@ -122,9 +132,14 @@ func (q *ChannelUniqueQueue) PushFunc(data Data, fn func() error) error { // Has checks if the data is in the queue func (q *ChannelUniqueQueue) Has(data Data) (bool, error) { + bs, err := json.Marshal(data) + if err != nil { + return false, err + } + q.lock.Lock() defer q.lock.Unlock() - _, has := q.table[data] + _, has := q.table[string(bs)] return has, nil } diff --git a/modules/setting/mailer.go b/modules/setting/mailer.go index a2228e938ba0..d2fac440ac08 100644 --- a/modules/setting/mailer.go +++ b/modules/setting/mailer.go @@ -16,7 +16,6 @@ import ( // Mailer represents mail service. type Mailer struct { // Mailer - QueueLength int Name string From string FromName string @@ -54,7 +53,6 @@ func newMailService() { } MailService = &Mailer{ - QueueLength: sec.Key("SEND_BUFFER_LEN").MustInt(100), Name: sec.Key("NAME").MustString(AppName), SendAsPlainText: sec.Key("SEND_AS_PLAIN_TEXT").MustBool(false), MailerType: sec.Key("MAILER_TYPE").In("", []string{"smtp", "sendmail", "dummy"}), diff --git a/modules/setting/queue.go b/modules/setting/queue.go index 76b7dc1faf70..1668cc63a34d 100644 --- a/modules/setting/queue.go +++ b/modules/setting/queue.go @@ -5,11 +5,12 @@ package setting import ( - "fmt" "path/filepath" + "strconv" "time" "code.gitea.io/gitea/modules/log" + ini "gopkg.in/ini.v1" ) // QueueSettings represent the settings for a queue from the ini @@ -106,11 +107,8 @@ func NewQueueService() { // Now handle the old issue_indexer configuration section := Cfg.Section("queue.issue_indexer") - sectionMap := map[string]bool{} - for _, key := range section.Keys() { - sectionMap[key.Name()] = true - } - if _, ok := sectionMap["TYPE"]; !ok && defaultType == "" { + directlySet := toDirectlySetKeysMap(section) + if !directlySet["TYPE"] && defaultType == "" { switch Indexer.IssueQueueType { case LevelQueueType: _, _ = section.NewKey("TYPE", "level") @@ -125,37 +123,53 @@ func NewQueueService() { Indexer.IssueQueueType) } } - if _, ok := sectionMap["LENGTH"]; !ok && Indexer.UpdateQueueLength != 0 { - _, _ = section.NewKey("LENGTH", fmt.Sprintf("%d", Indexer.UpdateQueueLength)) + if !directlySet["LENGTH"] && Indexer.UpdateQueueLength != 0 { + _, _ = section.NewKey("LENGTH", strconv.Itoa(Indexer.UpdateQueueLength)) } - if _, ok := sectionMap["BATCH_LENGTH"]; !ok && Indexer.IssueQueueBatchNumber != 0 { - _, _ = section.NewKey("BATCH_LENGTH", fmt.Sprintf("%d", Indexer.IssueQueueBatchNumber)) + if !directlySet["BATCH_LENGTH"] && Indexer.IssueQueueBatchNumber != 0 { + _, _ = section.NewKey("BATCH_LENGTH", strconv.Itoa(Indexer.IssueQueueBatchNumber)) } - if _, ok := sectionMap["DATADIR"]; !ok && Indexer.IssueQueueDir != "" { + if !directlySet["DATADIR"] && Indexer.IssueQueueDir != "" { _, _ = section.NewKey("DATADIR", Indexer.IssueQueueDir) } - if _, ok := sectionMap["CONN_STR"]; !ok && Indexer.IssueQueueConnStr != "" { + if !directlySet["CONN_STR"] && Indexer.IssueQueueConnStr != "" { _, _ = section.NewKey("CONN_STR", Indexer.IssueQueueConnStr) } // Handle the old mailer configuration - section = Cfg.Section("queue.mailer") - sectionMap = map[string]bool{} - for _, key := range section.Keys() { - sectionMap[key.Name()] = true - } - if _, ok := sectionMap["LENGTH"]; !ok { - _, _ = section.NewKey("LENGTH", fmt.Sprintf("%d", Cfg.Section("mailer").Key("SEND_BUFFER_LEN").MustInt(100))) - } + handleOldLengthConfiguration("mailer", Cfg.Section("mailer").Key("SEND_BUFFER_LEN").MustInt(100)) // Handle the old test pull requests configuration // Please note this will be a unique queue - section = Cfg.Section("queue.pr_patch_checker") - sectionMap = map[string]bool{} + handleOldLengthConfiguration("pr_patch_checker", Cfg.Section("repository").Key("PULL_REQUEST_QUEUE_LENGTH").MustInt(1000)) + + // Handle the old mirror queue configuration + // Please note this will be a unique queue + handleOldLengthConfiguration("mirror", Cfg.Section("repository").Key("MIRROR_QUEUE_LENGTH").MustInt(1000)) +} + +// handleOldLengthConfiguration allows fallback to older configuration. `[queue.name]` `LENGTH` will override this configuration, but +// if that is left unset then we should fallback to the older configuration. (Except where the new length woul be <=0) +func handleOldLengthConfiguration(queueName string, value int) { + // Don't override with 0 + if value <= 0 { + return + } + + section := Cfg.Section("queue." + queueName) + directlySet := toDirectlySetKeysMap(section) + if !directlySet["LENGTH"] { + _, _ = section.NewKey("LENGTH", strconv.Itoa(value)) + } +} + +// toDirectlySetKeysMap returns a bool map of keys directly set by this section +// Note: we cannot use section.HasKey(...) as that will immediately set the Key if a parent section has the Key +// but this section does not. +func toDirectlySetKeysMap(section *ini.Section) map[string]bool { + sectionMap := map[string]bool{} for _, key := range section.Keys() { sectionMap[key.Name()] = true } - if _, ok := sectionMap["LENGTH"]; !ok { - _, _ = section.NewKey("LENGTH", fmt.Sprintf("%d", Repository.PullRequestQueueLength)) - } + return sectionMap } diff --git a/modules/setting/repository.go b/modules/setting/repository.go index de57eb91401a..0791602efb6b 100644 --- a/modules/setting/repository.go +++ b/modules/setting/repository.go @@ -29,8 +29,6 @@ var ( DefaultPrivate string DefaultPushCreatePrivate bool MaxCreationLimit int - MirrorQueueLength int - PullRequestQueueLength int PreferredLicenses []string DisableHTTPGit bool AccessControlAllowOrigin string @@ -142,8 +140,6 @@ var ( DefaultPrivate: RepoCreatingLastUserVisibility, DefaultPushCreatePrivate: true, MaxCreationLimit: -1, - MirrorQueueLength: 1000, - PullRequestQueueLength: 1000, PreferredLicenses: []string{"Apache License 2.0", "MIT License"}, DisableHTTPGit: false, AccessControlAllowOrigin: "", diff --git a/services/mirror/mirror.go b/services/mirror/mirror.go index 7a3e37d99359..eb37639beff8 100644 --- a/services/mirror/mirror.go +++ b/services/mirror/mirror.go @@ -7,18 +7,43 @@ package mirror import ( "context" "fmt" - "strconv" - "strings" "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/graceful" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/queue" "code.gitea.io/gitea/modules/setting" - "code.gitea.io/gitea/modules/sync" ) -// mirrorQueue holds an UniqueQueue object of the mirror -var mirrorQueue = sync.NewUniqueQueue(setting.Repository.MirrorQueueLength) +var mirrorQueue queue.UniqueQueue + +// SyncType type of sync request +type SyncType int + +const ( + // PullMirrorType for pull mirrors + PullMirrorType SyncType = iota + // PushMirrorType for push mirrors + PushMirrorType +) + +// SyncRequest for the mirror queue +type SyncRequest struct { + Type SyncType + RepoID int64 +} + +// doMirrorSync causes this request to mirror itself +func doMirrorSync(ctx context.Context, req *SyncRequest) { + switch req.Type { + case PushMirrorType: + _ = SyncPushMirror(ctx, req.RepoID) + case PullMirrorType: + _ = SyncPullMirror(ctx, req.RepoID) + default: + log.Error("Unknown Request type in queue: %v for RepoID[%d]", req.Type, req.RepoID) + } +} // Update checks and updates mirror repositories. func Update(ctx context.Context) error { @@ -29,19 +54,25 @@ func Update(ctx context.Context) error { log.Trace("Doing: Update") handler := func(idx int, bean interface{}) error { - var item string + var item SyncRequest if m, ok := bean.(*models.Mirror); ok { if m.Repo == nil { log.Error("Disconnected mirror found: %d", m.ID) return nil } - item = fmt.Sprintf("pull %d", m.RepoID) + item = SyncRequest{ + Type: PullMirrorType, + RepoID: m.RepoID, + } } else if m, ok := bean.(*models.PushMirror); ok { if m.Repo == nil { log.Error("Disconnected push-mirror found: %d", m.ID) return nil } - item = fmt.Sprintf("push %d", m.ID) + item = SyncRequest{ + Type: PushMirrorType, + RepoID: m.RepoID, + } } else { log.Error("Unknown bean: %v", bean) return nil @@ -51,8 +82,7 @@ func Update(ctx context.Context) error { case <-ctx.Done(): return fmt.Errorf("Aborted") default: - mirrorQueue.Add(item) - return nil + return mirrorQueue.Push(&item) } } @@ -68,26 +98,10 @@ func Update(ctx context.Context) error { return nil } -// syncMirrors checks and syncs mirrors. -// FIXME: graceful: this should be a persistable queue -func syncMirrors(ctx context.Context) { - // Start listening on new sync requests. - for { - select { - case <-ctx.Done(): - mirrorQueue.Close() - return - case item := <-mirrorQueue.Queue(): - id, _ := strconv.ParseInt(item[5:], 10, 64) - if strings.HasPrefix(item, "pull") { - _ = SyncPullMirror(ctx, id) - } else if strings.HasPrefix(item, "push") { - _ = SyncPushMirror(ctx, id) - } else { - log.Error("Unknown item in queue: %v", item) - } - mirrorQueue.Remove(item) - } +func queueHandle(data ...queue.Data) { + for _, datum := range data { + req := datum.(*SyncRequest) + doMirrorSync(graceful.GetManager().ShutdownContext(), req) } } @@ -96,7 +110,9 @@ func InitSyncMirrors() { if !setting.Mirror.Enabled { return } - go graceful.GetManager().RunWithShutdownContext(syncMirrors) + mirrorQueue = queue.CreateUniqueQueue("mirror", queueHandle, new(SyncRequest)) + + go graceful.GetManager().RunWithShutdownFns(mirrorQueue.Run) } // StartToMirror adds repoID to mirror queue @@ -104,7 +120,15 @@ func StartToMirror(repoID int64) { if !setting.Mirror.Enabled { return } - go mirrorQueue.Add(fmt.Sprintf("pull %d", repoID)) + go func() { + err := mirrorQueue.Push(&SyncRequest{ + Type: PullMirrorType, + RepoID: repoID, + }) + if err != nil { + log.Error("Unable to push sync request for to the queue for push mirror repo[%d]: Error: %v", repoID, err) + } + }() } // AddPushMirrorToQueue adds the push mirror to the queue @@ -112,5 +136,13 @@ func AddPushMirrorToQueue(mirrorID int64) { if !setting.Mirror.Enabled { return } - go mirrorQueue.Add(fmt.Sprintf("push %d", mirrorID)) + go func() { + err := mirrorQueue.Push(&SyncRequest{ + Type: PushMirrorType, + RepoID: mirrorID, + }) + if err != nil { + log.Error("Unable to push sync request to the queue for pull mirror repo[%d]: Error: %v", mirrorID, err) + } + }() } From 633b041e22afd78640a8226410fbe5790fd7acb7 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 17 Oct 2021 13:50:22 +0100 Subject: [PATCH 14/26] make mirroring a process Signed-off-by: Andrew Thornton --- services/mirror/mirror_pull.go | 6 +++++- services/mirror/mirror_push.go | 7 ++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/services/mirror/mirror_pull.go b/services/mirror/mirror_pull.go index c2b413131d19..84b6609a7135 100644 --- a/services/mirror/mirror_pull.go +++ b/services/mirror/mirror_pull.go @@ -17,6 +17,7 @@ import ( "code.gitea.io/gitea/modules/lfs" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/notification" + "code.gitea.io/gitea/modules/process" repo_module "code.gitea.io/gitea/modules/repository" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/timeutil" @@ -273,6 +274,9 @@ func SyncPullMirror(ctx context.Context, repoID int64) bool { return false } + ctx, _, remove := process.GetManager().AddContext(ctx, fmt.Sprintf("Syncing Mirror %s/%s", m.Repo.OwnerName, m.Repo.Name)) + defer remove() + log.Trace("SyncMirrors [repo: %-v]: Running Sync", m.Repo) results, ok := runSync(ctx, m) if !ok { @@ -291,7 +295,7 @@ func SyncPullMirror(ctx context.Context, repoID int64) bool { log.Trace("SyncMirrors [repo: %-v]: no branches updated", m.Repo) } else { log.Trace("SyncMirrors [repo: %-v]: %d branches updated", m.Repo, len(results)) - gitRepo, err = git.OpenRepository(m.Repo.RepoPath()) + gitRepo, err = git.OpenRepositoryCtx(ctx, m.Repo.RepoPath()) if err != nil { log.Error("OpenRepository [%d]: %v", m.RepoID, err) return false diff --git a/services/mirror/mirror_push.go b/services/mirror/mirror_push.go index c1f53196e39e..caae51e3b6e4 100644 --- a/services/mirror/mirror_push.go +++ b/services/mirror/mirror_push.go @@ -7,6 +7,7 @@ package mirror import ( "context" "errors" + "fmt" "io" "net/url" "regexp" @@ -16,6 +17,7 @@ import ( "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/lfs" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/process" "code.gitea.io/gitea/modules/repository" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/timeutil" @@ -93,6 +95,9 @@ func SyncPushMirror(ctx context.Context, mirrorID int64) bool { m.LastError = "" + ctx, _, remove := process.GetManager().AddContext(ctx, fmt.Sprintf("Syncing PushMirror %s/%s to %s", m.Repo.OwnerName, m.Repo.Name, m.RemoteName)) + defer remove() + log.Trace("SyncPushMirror [mirror: %d][repo: %-v]: Running Sync", m.ID, m.Repo) err = runPushSync(ctx, m) if err != nil { @@ -126,7 +131,7 @@ func runPushSync(ctx context.Context, m *models.PushMirror) error { if setting.LFS.StartServer { log.Trace("SyncMirrors [repo: %-v]: syncing LFS objects...", m.Repo) - gitRepo, err := git.OpenRepository(path) + gitRepo, err := git.OpenRepositoryCtx(ctx, path) if err != nil { log.Error("OpenRepository: %v", err) return errors.New("Unexpected error") From 377a384f632346fb3ffb198231a1e05aca438300 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 17 Oct 2021 13:59:01 +0100 Subject: [PATCH 15/26] Ensure that mirrors are al within the same context Signed-off-by: Andrew Thornton --- modules/git/remote.go | 11 +++++++---- modules/git/repo.go | 4 ++-- modules/repofiles/temp_repo.go | 2 +- modules/repository/branch.go | 4 ++-- modules/templates/helper.go | 2 +- routers/web/repo/branch.go | 2 +- routers/web/repo/setting.go | 2 +- services/mirror/mirror_pull.go | 4 ++-- services/mirror/mirror_push.go | 6 +++--- services/pull/pull.go | 2 +- services/wiki/wiki.go | 4 ++-- 11 files changed, 23 insertions(+), 20 deletions(-) diff --git a/modules/git/remote.go b/modules/git/remote.go index 7ba2b35a5ed3..5ed02300d420 100644 --- a/modules/git/remote.go +++ b/modules/git/remote.go @@ -4,19 +4,22 @@ package git -import "net/url" +import ( + "context" + "net/url" +) // GetRemoteAddress returns the url of a specific remote of the repository. -func GetRemoteAddress(repoPath, remoteName string) (*url.URL, error) { +func GetRemoteAddress(ctx context.Context, repoPath, remoteName string) (*url.URL, error) { err := LoadGitVersion() if err != nil { return nil, err } var cmd *Command if CheckGitVersionAtLeast("2.7") == nil { - cmd = NewCommand("remote", "get-url", remoteName) + cmd = NewCommandContext(ctx, "remote", "get-url", remoteName) } else { - cmd = NewCommand("config", "--get", "remote."+remoteName+".url") + cmd = NewCommandContext(ctx, "config", "--get", "remote."+remoteName+".url") } result, err := cmd.RunInDir(repoPath) diff --git a/modules/git/repo.go b/modules/git/repo.go index 89af7aa9e1da..3ff2b6fe2d06 100644 --- a/modules/git/repo.go +++ b/modules/git/repo.go @@ -211,8 +211,8 @@ type PushOptions struct { } // Push pushs local commits to given remote branch. -func Push(repoPath string, opts PushOptions) error { - cmd := NewCommand("push") +func Push(ctx context.Context, repoPath string, opts PushOptions) error { + cmd := NewCommandContext(ctx, "push") if opts.Force { cmd.AddArguments("-f") } diff --git a/modules/repofiles/temp_repo.go b/modules/repofiles/temp_repo.go index 820f5f25421e..c7d689db6ed3 100644 --- a/modules/repofiles/temp_repo.go +++ b/modules/repofiles/temp_repo.go @@ -263,7 +263,7 @@ func (t *TemporaryUploadRepository) CommitTreeWithDate(author, committer *models func (t *TemporaryUploadRepository) Push(doer *models.User, commitHash string, branch string) error { // Because calls hooks we need to pass in the environment env := models.PushingEnvironment(doer, t.repo) - if err := git.Push(t.basePath, git.PushOptions{ + if err := git.Push(t.gitRepo.Ctx, t.basePath, git.PushOptions{ Remote: t.repo.RepoPath(), Branch: strings.TrimSpace(commitHash) + ":refs/heads/" + strings.TrimSpace(branch), Env: env, diff --git a/modules/repository/branch.go b/modules/repository/branch.go index 2ef676c12f07..ea002467cd19 100644 --- a/modules/repository/branch.go +++ b/modules/repository/branch.go @@ -79,7 +79,7 @@ func CreateNewBranch(doer *models.User, repo *models.Repository, oldBranchName, } } - if err := git.Push(repo.RepoPath(), git.PushOptions{ + if err := git.Push(git.DefaultContext, repo.RepoPath(), git.PushOptions{ Remote: repo.RepoPath(), Branch: fmt.Sprintf("%s:%s%s", oldBranchName, git.BranchPrefix, branchName), Env: models.PushingEnvironment(doer, repo), @@ -100,7 +100,7 @@ func CreateNewBranchFromCommit(doer *models.User, repo *models.Repository, commi return err } - if err := git.Push(repo.RepoPath(), git.PushOptions{ + if err := git.Push(git.DefaultContext, repo.RepoPath(), git.PushOptions{ Remote: repo.RepoPath(), Branch: fmt.Sprintf("%s:%s%s", commit, git.BranchPrefix, branchName), Env: models.PushingEnvironment(doer, repo), diff --git a/modules/templates/helper.go b/modules/templates/helper.go index 79195863599e..44162efea26e 100644 --- a/modules/templates/helper.go +++ b/modules/templates/helper.go @@ -951,7 +951,7 @@ type remoteAddress struct { func mirrorRemoteAddress(m models.RemoteMirrorer) remoteAddress { a := remoteAddress{} - u, err := git.GetRemoteAddress(m.GetRepository().RepoPath(), m.GetRemoteName()) + u, err := git.GetRemoteAddress(git.DefaultContext, m.GetRepository().RepoPath(), m.GetRemoteName()) if err != nil { log.Error("GetRemoteAddress %v", err) return a diff --git a/routers/web/repo/branch.go b/routers/web/repo/branch.go index 32cfc2adff32..d5dff35b3b13 100644 --- a/routers/web/repo/branch.go +++ b/routers/web/repo/branch.go @@ -122,7 +122,7 @@ func RestoreBranchPost(ctx *context.Context) { return } - if err := git.Push(ctx.Repo.Repository.RepoPath(), git.PushOptions{ + if err := git.Push(ctx, ctx.Repo.Repository.RepoPath(), git.PushOptions{ Remote: ctx.Repo.Repository.RepoPath(), Branch: fmt.Sprintf("%s:%s%s", deletedBranch.Commit, git.BranchPrefix, deletedBranch.Name), Env: models.PushingEnvironment(ctx.User, ctx.Repo.Repository), diff --git a/routers/web/repo/setting.go b/routers/web/repo/setting.go index e71a5bf482bb..984efbef48f7 100644 --- a/routers/web/repo/setting.go +++ b/routers/web/repo/setting.go @@ -175,7 +175,7 @@ func SettingsPost(ctx *context.Context) { } } - u, _ := git.GetRemoteAddress(ctx.Repo.Repository.RepoPath(), ctx.Repo.Mirror.GetRemoteName()) + u, _ := git.GetRemoteAddress(ctx, ctx.Repo.Repository.RepoPath(), ctx.Repo.Mirror.GetRemoteName()) if u.User != nil && form.MirrorPassword == "" && form.MirrorUsername == u.User.Username() { form.MirrorPassword, _ = u.User.Password() } diff --git a/services/mirror/mirror_pull.go b/services/mirror/mirror_pull.go index 84b6609a7135..ef11e1d8278e 100644 --- a/services/mirror/mirror_pull.go +++ b/services/mirror/mirror_pull.go @@ -155,7 +155,7 @@ func runSync(ctx context.Context, m *models.Mirror) ([]*mirrorSyncResult, bool) } gitArgs = append(gitArgs, m.GetRemoteName()) - remoteAddr, remoteErr := git.GetRemoteAddress(repoPath, m.GetRemoteName()) + remoteAddr, remoteErr := git.GetRemoteAddress(ctx, repoPath, m.GetRemoteName()) if remoteErr != nil { log.Error("GetRemoteAddress Error %v", remoteErr) } @@ -222,7 +222,7 @@ func runSync(ctx context.Context, m *models.Mirror) ([]*mirrorSyncResult, bool) // sanitize the output, since it may contain the remote address, which may // contain a password - remoteAddr, remoteErr := git.GetRemoteAddress(wikiPath, m.GetRemoteName()) + remoteAddr, remoteErr := git.GetRemoteAddress(ctx, wikiPath, m.GetRemoteName()) if remoteErr != nil { log.Error("GetRemoteAddress Error %v", remoteErr) } diff --git a/services/mirror/mirror_push.go b/services/mirror/mirror_push.go index caae51e3b6e4..9f567d4c2a49 100644 --- a/services/mirror/mirror_push.go +++ b/services/mirror/mirror_push.go @@ -122,7 +122,7 @@ func runPushSync(ctx context.Context, m *models.PushMirror) error { timeout := time.Duration(setting.Git.Timeout.Mirror) * time.Second performPush := func(path string) error { - remoteAddr, err := git.GetRemoteAddress(path, m.RemoteName) + remoteAddr, err := git.GetRemoteAddress(ctx, path, m.RemoteName) if err != nil { log.Error("GetRemoteAddress(%s) Error %v", path, err) return errors.New("Unexpected error") @@ -146,7 +146,7 @@ func runPushSync(ctx context.Context, m *models.PushMirror) error { log.Trace("Pushing %s mirror[%d] remote %s", path, m.ID, m.RemoteName) - if err := git.Push(path, git.PushOptions{ + if err := git.Push(ctx, path, git.PushOptions{ Remote: m.RemoteName, Force: true, Mirror: true, @@ -167,7 +167,7 @@ func runPushSync(ctx context.Context, m *models.PushMirror) error { if m.Repo.HasWiki() { wikiPath := m.Repo.WikiPath() - _, err := git.GetRemoteAddress(wikiPath, m.RemoteName) + _, err := git.GetRemoteAddress(ctx, wikiPath, m.RemoteName) if err == nil { err := performPush(wikiPath) if err != nil { diff --git a/services/pull/pull.go b/services/pull/pull.go index 41006636c1e2..22badaf080de 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -430,7 +430,7 @@ func pushToBaseRepoHelper(pr *models.PullRequest, prefixHeadBranch string) (err gitRefName := pr.GetGitRefName() - if err := git.Push(headRepoPath, git.PushOptions{ + if err := git.Push(git.DefaultContext, headRepoPath, git.PushOptions{ Remote: baseRepoPath, Branch: prefixHeadBranch + pr.HeadBranch + ":" + gitRefName, Force: true, diff --git a/services/wiki/wiki.go b/services/wiki/wiki.go index 9859e8b8b51b..bafb7ac743ff 100644 --- a/services/wiki/wiki.go +++ b/services/wiki/wiki.go @@ -239,7 +239,7 @@ func updateWikiPage(doer *models.User, repo *models.Repository, oldWikiName, new return err } - if err := git.Push(basePath, git.PushOptions{ + if err := git.Push(gitRepo.Ctx, basePath, git.PushOptions{ Remote: "origin", Branch: fmt.Sprintf("%s:%s%s", commitHash.String(), git.BranchPrefix, "master"), Env: models.FullPushingEnvironment( @@ -353,7 +353,7 @@ func DeleteWikiPage(doer *models.User, repo *models.Repository, wikiName string) return err } - if err := git.Push(basePath, git.PushOptions{ + if err := git.Push(gitRepo.Ctx, basePath, git.PushOptions{ Remote: "origin", Branch: fmt.Sprintf("%s:%s%s", commitHash.String(), git.BranchPrefix, "master"), Env: models.PushingEnvironment(doer, repo), From 71dec9979e9a03c7487c529360cd1fc69482e0d6 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 19 Oct 2021 16:25:25 +0100 Subject: [PATCH 16/26] add clarity to the difference between cancel and remove Signed-off-by: Andrew Thornton --- modules/process/manager.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/modules/process/manager.go b/modules/process/manager.go index 15bef5463e4c..87e44f7d336f 100644 --- a/modules/process/manager.go +++ b/modules/process/manager.go @@ -54,6 +54,12 @@ func GetManager() *Manager { } // AddContext create a new context and add it as a process. The remove CancelFunc must always be called even if the context is Done() +// +// cancel should be used to cancel the returned context, however it will not remove the process from the process table. +// remove will cancel the returned context and remove it from the process table. +// +// 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) AddContext(parent context.Context, description string) (ctx context.Context, cancel, remove context.CancelFunc) { parentPID := GetParentPID(parent) @@ -68,6 +74,12 @@ func (pm *Manager) AddContext(parent context.Context, description string) (ctx c } // AddContextTimeout create a new context and add it as a process. The remove CancelFunc must always be called even if the context is Done() +// +// cancel should be used to cancel the returned context, however it will not remove the process from the process table. +// remove will cancel the returned context and remove it from the process table. +// +// 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, remove context.CancelFunc) { parentPID := GetParentPID(parent) From 9e95fdbdf2ad6362e1be49095970103e324b855b Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 19 Oct 2021 16:38:46 +0100 Subject: [PATCH 17/26] add explanatory notes for remove and close Signed-off-by: Andrew Thornton --- modules/git/blame.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/modules/git/blame.go b/modules/git/blame.go index 44eca331e5d8..5afe3a8e196f 100644 --- a/modules/git/blame.go +++ b/modules/git/blame.go @@ -28,8 +28,8 @@ type BlameReader struct { output io.ReadCloser reader *bufio.Reader lastSha *string - cancel context.CancelFunc - remove context.CancelFunc + cancel context.CancelFunc // Cancels the context that this reader runs in + remove context.CancelFunc // Tells the process manager to remove the associated process from the process table } var shaLineRegex = regexp.MustCompile("^([a-z0-9]{40})") @@ -100,8 +100,8 @@ func (r *BlameReader) NextPart() (*BlamePart, error) { // Close BlameReader - don't run NextPart after invoking that func (r *BlameReader) Close() error { - defer r.remove() - r.cancel() + defer r.remove() // Only remove the process from the process table when the underlying command is closed + r.cancel() // However, first cancel our own context early _ = r.output.Close() @@ -114,7 +114,7 @@ func (r *BlameReader) Close() error { // CreateBlameReader creates reader for given repository, commit and file func CreateBlameReader(ctx context.Context, repoPath, commitID, file string) (*BlameReader, error) { - gitRepo, err := OpenRepository(repoPath) + gitRepo, err := OpenRepositoryCtx(ctx, repoPath) if err != nil { return nil, err } From 9cc97d0b24e6e80c07deba752ebef52deda1eaa4 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 19 Oct 2021 16:44:34 +0100 Subject: [PATCH 18/26] explicitly name the arguments in the blame reader Signed-off-by: Andrew Thornton --- modules/git/blame.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/modules/git/blame.go b/modules/git/blame.go index 5afe3a8e196f..7769eb054cc0 100644 --- a/modules/git/blame.go +++ b/modules/git/blame.go @@ -146,11 +146,10 @@ func createBlameReader(ctx context.Context, dir string, command ...string) (*Bla reader := bufio.NewReader(stdout) return &BlameReader{ - cmd, - stdout, - reader, - nil, - cancel, - remove, + cmd: cmd, + output: stdout, + reader: reader, + cancel: cancel, + remove: remove, }, nil } From e4aebfb38a6c4bd29a070e190b2254057eca03a7 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 20 Oct 2021 17:38:14 +0100 Subject: [PATCH 19/26] Change remove to finished Signed-off-by: Andrew Thornton --- modules/cron/tasks.go | 4 ++-- modules/git/blame.go | 32 ++++++++++++++--------------- modules/git/command.go | 4 ++-- modules/git/diff.go | 4 ++-- modules/indexer/stats/db.go | 4 ++-- modules/markup/external/external.go | 4 ++-- modules/process/manager.go | 30 ++++++++++++++++----------- modules/process/manager_test.go | 28 ++++++++++++------------- modules/task/migrate.go | 4 ++-- routers/common/middleware.go | 4 ++-- routers/web/repo/http.go | 4 ++-- services/gitdiff/gitdiff.go | 4 ++-- services/mailer/mailer.go | 4 ++-- services/mirror/mirror_pull.go | 4 ++-- services/mirror/mirror_push.go | 4 ++-- 15 files changed, 72 insertions(+), 66 deletions(-) diff --git a/modules/cron/tasks.go b/modules/cron/tasks.go index 56588d48d511..6ceb1fcd5f46 100644 --- a/modules/cron/tasks.go +++ b/modules/cron/tasks.go @@ -81,8 +81,8 @@ func (t *Task) RunWithUser(doer *models.User, config Config) { }() graceful.GetManager().RunWithShutdownContext(func(baseCtx context.Context) { pm := process.GetManager() - ctx, _, remove := pm.AddContext(baseCtx, config.FormatMessage(t.Name, "process", doer)) - defer remove() + ctx, _, finished := pm.AddContext(baseCtx, config.FormatMessage(t.Name, "process", doer)) + defer finished() if err := t.fun(ctx, doer, config); err != nil { if models.IsErrCancelled(err) { diff --git a/modules/git/blame.go b/modules/git/blame.go index 7769eb054cc0..49cbbf5c82cd 100644 --- a/modules/git/blame.go +++ b/modules/git/blame.go @@ -24,12 +24,12 @@ type BlamePart struct { // BlameReader returns part of file blame one by one type BlameReader struct { - cmd *exec.Cmd - output io.ReadCloser - reader *bufio.Reader - lastSha *string - cancel context.CancelFunc // Cancels the context that this reader runs in - remove context.CancelFunc // Tells the process manager to remove the associated process from the process table + cmd *exec.Cmd + output io.ReadCloser + reader *bufio.Reader + lastSha *string + cancel context.CancelFunc // Cancels the context that this reader runs in + finished context.CancelFunc // Tells the process manager to remove the associated process from the process table } var shaLineRegex = regexp.MustCompile("^([a-z0-9]{40})") @@ -100,8 +100,8 @@ func (r *BlameReader) NextPart() (*BlamePart, error) { // Close BlameReader - don't run NextPart after invoking that func (r *BlameReader) Close() error { - defer r.remove() // Only remove the process from the process table when the underlying command is closed - r.cancel() // However, first cancel our own context early + defer r.finished() // Only remove the process from the process table when the underlying command is closed + r.cancel() // However, first cancel our own context early _ = r.output.Close() @@ -125,7 +125,7 @@ func CreateBlameReader(ctx context.Context, repoPath, commitID, file string) (*B func createBlameReader(ctx context.Context, dir string, command ...string) (*BlameReader, error) { // Here we use the provided context - this should be tied to the request performing the blame so that it does not hang around. - ctx, cancel, remove := process.GetManager().AddContext(ctx, fmt.Sprintf("GetBlame [repo_path: %s]", dir)) + ctx, cancel, finished := process.GetManager().AddContext(ctx, fmt.Sprintf("GetBlame [repo_path: %s]", dir)) cmd := exec.CommandContext(ctx, command[0], command[1:]...) cmd.Dir = dir @@ -133,12 +133,12 @@ func createBlameReader(ctx context.Context, dir string, command ...string) (*Bla stdout, err := cmd.StdoutPipe() if err != nil { - defer remove() + defer finished() return nil, fmt.Errorf("StdoutPipe: %v", err) } if err = cmd.Start(); err != nil { - defer remove() + defer finished() _ = stdout.Close() return nil, fmt.Errorf("Start: %v", err) } @@ -146,10 +146,10 @@ func createBlameReader(ctx context.Context, dir string, command ...string) (*Bla reader := bufio.NewReader(stdout) return &BlameReader{ - cmd: cmd, - output: stdout, - reader: reader, - cancel: cancel, - remove: remove, + cmd: cmd, + output: stdout, + reader: reader, + cancel: cancel, + finished: finished, }, nil } diff --git a/modules/git/command.go b/modules/git/command.go index e06e0f94f5a9..3cf85c2d8520 100644 --- a/modules/git/command.go +++ b/modules/git/command.go @@ -148,8 +148,8 @@ func (c *Command) RunWithContext(rc *RunContext) error { desc = fmt.Sprintf("%s %s [repo_path: %s]", c.name, strings.Join(c.args, " "), rc.Dir) } - ctx, cancel, remove := process.GetManager().AddContextTimeout(c.parentContext, rc.Timeout, desc) - defer remove() + ctx, cancel, finished := process.GetManager().AddContextTimeout(c.parentContext, rc.Timeout, desc) + defer finished() cmd := exec.CommandContext(ctx, c.name, c.args...) if rc.Env == nil { diff --git a/modules/git/diff.go b/modules/git/diff.go index 8261355d6d6c..40d589d7f383 100644 --- a/modules/git/diff.go +++ b/modules/git/diff.go @@ -56,8 +56,8 @@ func GetRepoRawDiffForFile(repo *Repository, startCommit, endCommit string, diff fileArgs = append(fileArgs, "--", file) } // FIXME: graceful: These commands should have a timeout - ctx, _, remove := process.GetManager().AddContext(DefaultContext, fmt.Sprintf("GetRawDiffForFile: [repo_path: %s]", repo.Path)) - defer remove() + ctx, _, finished := process.GetManager().AddContext(DefaultContext, fmt.Sprintf("GetRawDiffForFile: [repo_path: %s]", repo.Path)) + defer finished() var cmd *exec.Cmd switch diffType { diff --git a/modules/indexer/stats/db.go b/modules/indexer/stats/db.go index f46be7fc3b56..9e251d0f69f2 100644 --- a/modules/indexer/stats/db.go +++ b/modules/indexer/stats/db.go @@ -20,8 +20,8 @@ type DBIndexer struct { // Index repository status function func (db *DBIndexer) Index(id int64) error { - ctx, _, remove := process.GetManager().AddContext(graceful.GetManager().ShutdownContext(), fmt.Sprintf("Stats.DB Index Repo[%d]", id)) - defer remove() + ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().ShutdownContext(), fmt.Sprintf("Stats.DB Index Repo[%d]", id)) + defer finished() repo, err := models.GetRepositoryByID(id) if err != nil { diff --git a/modules/markup/external/external.go b/modules/markup/external/external.go index 1df13ab17d40..3acb601067df 100644 --- a/modules/markup/external/external.go +++ b/modules/markup/external/external.go @@ -106,8 +106,8 @@ func (p *Renderer) Render(ctx *markup.RenderContext, input io.Reader, output io. ctx.Ctx = graceful.GetManager().ShutdownContext() } - processCtx, _, remove := process.GetManager().AddContext(ctx.Ctx, fmt.Sprintf("Render [%s] for %s", commands[0], ctx.URLPrefix)) - defer remove() + processCtx, _, finished := process.GetManager().AddContext(ctx.Ctx, fmt.Sprintf("Render [%s] for %s", commands[0], ctx.URLPrefix)) + defer finished() cmd := exec.CommandContext(processCtx, commands[0], args...) cmd.Env = append( diff --git a/modules/process/manager.go b/modules/process/manager.go index 87e44f7d336f..53d0d563a954 100644 --- a/modules/process/manager.go +++ b/modules/process/manager.go @@ -32,6 +32,10 @@ var ( // IDType is a pid type type IDType string +// FinishedFunc is a function that marks that the process is finished and can be removed from the process table +// - it is simply an alias for context.CancelFunc and is only for documentary purpose +type FinishedFunc = context.CancelFunc + // Manager knows about all processes and counts PIDs. type Manager struct { mutex sync.Mutex @@ -53,44 +57,46 @@ func GetManager() *Manager { return manager } -// AddContext create a new context and add it as a process. The remove CancelFunc must always be called even if the context is Done() +// AddContext create a new context and add it as a process. Once the process is finished, finished must be called in order that the process +// can be removed from the process table. It should not be called until the process is finished but must always be called. // // cancel should be used to cancel the returned context, however it will not remove the process from the process table. -// remove will cancel the returned context and remove it from the process table. +// finished will cancel the returned context and remove it from the process table. // // 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) AddContext(parent context.Context, description string) (ctx context.Context, cancel, remove context.CancelFunc) { +func (pm *Manager) AddContext(parent context.Context, description string) (ctx context.Context, cancel context.CancelFunc, finished FinishedFunc) { parentPID := GetParentPID(parent) ctx, cancel = context.WithCancel(parent) - pid, remove := pm.Add(parentPID, description, cancel) + pid, finished := pm.Add(parentPID, description, cancel) return &Context{ Context: ctx, pid: pid, - }, cancel, remove + }, cancel, finished } -// AddContextTimeout create a new context and add it as a process. The remove CancelFunc must always be called even if the context is Done() +// AddContextTimeout create a new context and add it as a process. Once the process is finished, finished must be called in order that the process +// can be removed from the process table. It should not be called until the process is finsihed but must always be called. // // cancel should be used to cancel the returned context, however it will not remove the process from the process table. -// remove will cancel the returned context and remove it from the process table. +// finished will cancel the returned context and remove it from the process table. // // 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, remove context.CancelFunc) { +func (pm *Manager) AddContextTimeout(parent context.Context, timeout time.Duration, description string) (ctx context.Context, cancel context.CancelFunc, finshed FinishedFunc) { parentPID := GetParentPID(parent) ctx, cancel = context.WithTimeout(parent, timeout) - pid, remove := pm.Add(parentPID, description, cancel) + pid, finshed := pm.Add(parentPID, description, cancel) return &Context{ Context: ctx, pid: pid, - }, cancel, remove + }, cancel, finshed } // Add create a new process @@ -237,8 +243,8 @@ func (pm *Manager) ExecDirEnvStdIn(timeout time.Duration, dir, desc string, env stdOut := new(bytes.Buffer) stdErr := new(bytes.Buffer) - ctx, _, remove := pm.AddContextTimeout(DefaultContext, timeout, desc) - defer remove() + ctx, _, finished := pm.AddContextTimeout(DefaultContext, timeout, desc) + defer finished() cmd := exec.CommandContext(ctx, cmdName, args...) cmd.Dir = dir diff --git a/modules/process/manager_test.go b/modules/process/manager_test.go index 8493776bb583..eb4228e72cc3 100644 --- a/modules/process/manager_test.go +++ b/modules/process/manager_test.go @@ -27,12 +27,12 @@ func TestManager_AddContext(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - p1Ctx, _, remove := pm.AddContext(ctx, "foo") - defer remove() + p1Ctx, _, finished := pm.AddContext(ctx, "foo") + defer finished() assert.NotEmpty(t, GetContext(p1Ctx).GetPID(), "expected to get non-empty pid") - p2Ctx, _, remove := pm.AddContext(p1Ctx, "bar") - defer remove() + p2Ctx, _, finished := pm.AddContext(p1Ctx, "bar") + defer finished() assert.NotEmpty(t, GetContext(p2Ctx).GetPID(), "expected to get non-empty pid") @@ -43,8 +43,8 @@ func TestManager_AddContext(t *testing.T) { func TestManager_Cancel(t *testing.T) { pm := Manager{processes: make(map[IDType]*Process), next: 1} - ctx, _, remove := pm.AddContext(context.Background(), "foo") - defer remove() + ctx, _, finished := pm.AddContext(context.Background(), "foo") + defer finished() pm.Cancel(GetPID(ctx)) @@ -53,10 +53,10 @@ func TestManager_Cancel(t *testing.T) { default: assert.Fail(t, "Cancel should cancel the provided context") } - remove() + finished() - ctx, cancel, remove := pm.AddContext(context.Background(), "foo") - defer remove() + ctx, cancel, finished := pm.AddContext(context.Background(), "foo") + defer finished() cancel() @@ -65,7 +65,7 @@ func TestManager_Cancel(t *testing.T) { default: assert.Fail(t, "Cancel should cancel the provided context") } - remove() + finished() } func TestManager_Remove(t *testing.T) { @@ -74,12 +74,12 @@ func TestManager_Remove(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - p1Ctx, _, remove := pm.AddContext(ctx, "foo") - defer remove() + p1Ctx, _, finished := pm.AddContext(ctx, "foo") + defer finished() assert.NotEmpty(t, GetContext(p1Ctx).GetPID(), "expected to have non-empty PID") - p2Ctx, _, remove := pm.AddContext(p1Ctx, "bar") - defer remove() + p2Ctx, _, finished := pm.AddContext(p1Ctx, "bar") + defer finished() assert.NotEqual(t, GetContext(p1Ctx).GetPID(), GetContext(p2Ctx).GetPID(), "expected to get different pids got %s == %s", GetContext(p2Ctx).GetPID(), GetContext(p1Ctx).GetPID()) diff --git a/modules/task/migrate.go b/modules/task/migrate.go index c3003d894314..a5d771db710b 100644 --- a/modules/task/migrate.go +++ b/modules/task/migrate.go @@ -94,8 +94,8 @@ func runMigrateTask(t *models.Task) (err error) { opts.MigrateToRepoID = t.RepoID pm := process.GetManager() - ctx, _, remove := pm.AddContext(graceful.GetManager().ShutdownContext(), fmt.Sprintf("MigrateTask: %s/%s", t.Owner.Name, opts.RepoName)) - defer remove() + ctx, _, finished := pm.AddContext(graceful.GetManager().ShutdownContext(), fmt.Sprintf("MigrateTask: %s/%s", t.Owner.Name, opts.RepoName)) + defer finished() t.StartTime = timeutil.TimeStampNow() t.Status = structs.TaskStatusRunning diff --git a/routers/common/middleware.go b/routers/common/middleware.go index bafe6a4745a6..7fea20338ffa 100644 --- a/routers/common/middleware.go +++ b/routers/common/middleware.go @@ -23,8 +23,8 @@ func Middlewares() []func(http.Handler) http.Handler { var handlers = []func(http.Handler) http.Handler{ func(next http.Handler) http.Handler { return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) { - ctx, _, remove := process.GetManager().AddContext(req.Context(), fmt.Sprintf("%s: %s", req.Method, req.RequestURI)) - defer remove() + ctx, _, finished := process.GetManager().AddContext(req.Context(), fmt.Sprintf("%s: %s", req.Method, req.RequestURI)) + defer finished() next.ServeHTTP(context.NewResponse(resp), req.WithContext(ctx)) }) }, diff --git a/routers/web/repo/http.go b/routers/web/repo/http.go index e15fdc18e5bb..b37c7351620c 100644 --- a/routers/web/repo/http.go +++ b/routers/web/repo/http.go @@ -482,8 +482,8 @@ func serviceRPC(h serviceHandler, service string) { } // ctx, cancel := gocontext.WithCancel(git.DefaultContext) - ctx, _, remove := process.GetManager().AddContext(git.DefaultContext, fmt.Sprintf("%s %s %s [repo_path: %s]", git.GitExecutable, service, "--stateless-rpc", h.dir)) - defer remove() + ctx, _, finished := process.GetManager().AddContext(git.DefaultContext, fmt.Sprintf("%s %s %s [repo_path: %s]", git.GitExecutable, service, "--stateless-rpc", h.dir)) + defer finished() var stderr bytes.Buffer cmd := exec.CommandContext(ctx, git.GitExecutable, service, "--stateless-rpc", h.dir) diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index e119ff96b70e..da8c000b8919 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -1230,8 +1230,8 @@ func GetDiffRangeWithWhitespaceBehavior(gitRepo *git.Repository, beforeCommitID, } timeout := time.Duration(setting.Git.Timeout.Default) * time.Second - ctx, _, remove := process.GetManager().AddContextTimeout(git.DefaultContext, timeout, fmt.Sprintf("GetDiffRange [repo_path: %s]", repoPath)) - defer remove() + ctx, _, finished := process.GetManager().AddContextTimeout(git.DefaultContext, timeout, fmt.Sprintf("GetDiffRange [repo_path: %s]", repoPath)) + defer finished() argsLength := 6 if len(whitespaceBehavior) > 0 { diff --git a/services/mailer/mailer.go b/services/mailer/mailer.go index fd7a86936d75..351383748109 100644 --- a/services/mailer/mailer.go +++ b/services/mailer/mailer.go @@ -248,8 +248,8 @@ func (s *sendmailSender) Send(from string, to []string, msg io.WriterTo) error { desc := fmt.Sprintf("SendMail: %s %v", setting.MailService.SendmailPath, args) - ctx, cancel, remove := process.GetManager().AddContextTimeout(graceful.GetManager().HammerContext(), setting.MailService.SendmailTimeout, desc) - defer remove() + ctx, cancel, finished := process.GetManager().AddContextTimeout(graceful.GetManager().HammerContext(), setting.MailService.SendmailTimeout, desc) + defer finished() cmd := exec.CommandContext(ctx, setting.MailService.SendmailPath, args...) pipe, err := cmd.StdinPipe() diff --git a/services/mirror/mirror_pull.go b/services/mirror/mirror_pull.go index ef11e1d8278e..133ffe99d36e 100644 --- a/services/mirror/mirror_pull.go +++ b/services/mirror/mirror_pull.go @@ -274,8 +274,8 @@ func SyncPullMirror(ctx context.Context, repoID int64) bool { return false } - ctx, _, remove := process.GetManager().AddContext(ctx, fmt.Sprintf("Syncing Mirror %s/%s", m.Repo.OwnerName, m.Repo.Name)) - defer remove() + ctx, _, finished := process.GetManager().AddContext(ctx, fmt.Sprintf("Syncing Mirror %s/%s", m.Repo.OwnerName, m.Repo.Name)) + defer finished() log.Trace("SyncMirrors [repo: %-v]: Running Sync", m.Repo) results, ok := runSync(ctx, m) diff --git a/services/mirror/mirror_push.go b/services/mirror/mirror_push.go index 9f567d4c2a49..22c7cfa7dd35 100644 --- a/services/mirror/mirror_push.go +++ b/services/mirror/mirror_push.go @@ -95,8 +95,8 @@ func SyncPushMirror(ctx context.Context, mirrorID int64) bool { m.LastError = "" - ctx, _, remove := process.GetManager().AddContext(ctx, fmt.Sprintf("Syncing PushMirror %s/%s to %s", m.Repo.OwnerName, m.Repo.Name, m.RemoteName)) - defer remove() + ctx, _, finished := process.GetManager().AddContext(ctx, fmt.Sprintf("Syncing PushMirror %s/%s to %s", m.Repo.OwnerName, m.Repo.Name, m.RemoteName)) + defer finished() log.Trace("SyncPushMirror [mirror: %d][repo: %-v]: Running Sync", m.ID, m.Repo) err = runPushSync(ctx, m) From 217fbf7181924dd8ff597e322c193364b24464a0 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 20 Oct 2021 17:41:23 +0100 Subject: [PATCH 20/26] update blame documentation Signed-off-by: Andrew Thornton --- modules/git/blame.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/git/blame.go b/modules/git/blame.go index 49cbbf5c82cd..b30124594dc2 100644 --- a/modules/git/blame.go +++ b/modules/git/blame.go @@ -28,8 +28,8 @@ type BlameReader struct { output io.ReadCloser reader *bufio.Reader lastSha *string - cancel context.CancelFunc // Cancels the context that this reader runs in - finished context.CancelFunc // Tells the process manager to remove the associated process from the process table + cancel context.CancelFunc // Cancels the context that this reader runs in + finished process.FinishedFunc // Tells the process manager we're finished and it can remove the associated process from the process table } var shaLineRegex = regexp.MustCompile("^([a-z0-9]{40})") From 6b6ac80b0bd2f9f564e6473e9c1b7bf011b68de6 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 20 Oct 2021 18:22:05 +0100 Subject: [PATCH 21/26] as per review Signed-off-by: Andrew Thornton --- modules/process/manager.go | 6 +++--- services/mailer/mailer.go | 3 +-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/modules/process/manager.go b/modules/process/manager.go index 53d0d563a954..b632d4d00abb 100644 --- a/modules/process/manager.go +++ b/modules/process/manager.go @@ -100,7 +100,7 @@ func (pm *Manager) AddContextTimeout(parent context.Context, timeout time.Durati } // Add create a new process -func (pm *Manager) Add(parentPID IDType, description string, cancel context.CancelFunc) (IDType, context.CancelFunc) { +func (pm *Manager) Add(parentPID IDType, description string, cancel context.CancelFunc) (IDType, FinishedFunc) { pm.mutex.Lock() start, pid := pm.nextPID() @@ -117,7 +117,7 @@ func (pm *Manager) Add(parentPID IDType, description string, cancel context.Canc Cancel: cancel, } - remove := func() { + finished := func() { cancel() pm.remove(process) } @@ -128,7 +128,7 @@ func (pm *Manager) Add(parentPID IDType, description string, cancel context.Canc pm.processes[pid] = process pm.mutex.Unlock() - return pid, remove + return pid, finished } // nextPID will return the next available PID. pm.mutex should already be locked. diff --git a/services/mailer/mailer.go b/services/mailer/mailer.go index 351383748109..c95b41343d18 100644 --- a/services/mailer/mailer.go +++ b/services/mailer/mailer.go @@ -248,7 +248,7 @@ func (s *sendmailSender) Send(from string, to []string, msg io.WriterTo) error { desc := fmt.Sprintf("SendMail: %s %v", setting.MailService.SendmailPath, args) - ctx, cancel, finished := process.GetManager().AddContextTimeout(graceful.GetManager().HammerContext(), setting.MailService.SendmailTimeout, desc) + ctx, _, finished := process.GetManager().AddContextTimeout(graceful.GetManager().HammerContext(), setting.MailService.SendmailTimeout, desc) defer finished() cmd := exec.CommandContext(ctx, setting.MailService.SendmailPath, args...) @@ -260,7 +260,6 @@ func (s *sendmailSender) Send(from string, to []string, msg io.WriterTo) error { if err = cmd.Start(); err != nil { _ = pipe.Close() - cancel() return err } From e06216bd687ccd37101a27108ee19e6a619703ea Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 27 Oct 2021 19:42:28 +0100 Subject: [PATCH 22/26] Close the cat-file batch and checks after the context cancellation Signed-off-by: Andrew Thornton --- modules/git/batch_reader.go | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/modules/git/batch_reader.go b/modules/git/batch_reader.go index 324b069512c7..71045adbc9bf 100644 --- a/modules/git/batch_reader.go +++ b/modules/git/batch_reader.go @@ -34,11 +34,9 @@ func CatFileBatchCheck(ctx context.Context, repoPath string) (WriteCloserError, ctx, ctxCancel := context.WithCancel(ctx) closed := make(chan struct{}) cancel := func() { - _ = batchStdinReader.Close() - _ = batchStdinWriter.Close() - _ = batchStdoutReader.Close() - _ = batchStdoutWriter.Close() ctxCancel() + _ = batchStdoutReader.Close() + _ = batchStdinWriter.Close() <-closed } @@ -75,11 +73,9 @@ func CatFileBatch(ctx context.Context, repoPath string) (WriteCloserError, *bufi ctx, ctxCancel := context.WithCancel(ctx) closed := make(chan struct{}) cancel := func() { - _ = batchStdinReader.Close() + ctxCancel() _ = batchStdinWriter.Close() _ = batchStdoutReader.Close() - _ = batchStdoutWriter.Close() - ctxCancel() <-closed } From 1203fa9338b5a6ff9e71747a27ff9b2cd3c8a11e Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 30 Nov 2021 19:00:02 +0000 Subject: [PATCH 23/26] Ensure that http requests use the same context as the request Signed-off-by: Andrew Thornton --- routers/web/repo/http.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/web/repo/http.go b/routers/web/repo/http.go index caad3e8f93bd..3aa8e84f57fa 100644 --- a/routers/web/repo/http.go +++ b/routers/web/repo/http.go @@ -485,7 +485,7 @@ func serviceRPC(h serviceHandler, service string) { } // ctx, cancel := gocontext.WithCancel(git.DefaultContext) - ctx, _, finished := process.GetManager().AddContext(git.DefaultContext, fmt.Sprintf("%s %s %s [repo_path: %s]", git.GitExecutable, service, "--stateless-rpc", h.dir)) + ctx, _, finished := process.GetManager().AddContext(h.r.Context(), fmt.Sprintf("%s %s %s [repo_path: %s]", git.GitExecutable, service, "--stateless-rpc", h.dir)) defer finished() var stderr bytes.Buffer From 772d31d2f68c3d2dd574e73c0fa237425f90a578 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 30 Nov 2021 19:05:00 +0000 Subject: [PATCH 24/26] use the repo context in the diff Signed-off-by: Andrew Thornton --- modules/git/diff.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/git/diff.go b/modules/git/diff.go index 40d589d7f383..3a82cda1ce03 100644 --- a/modules/git/diff.go +++ b/modules/git/diff.go @@ -56,7 +56,7 @@ func GetRepoRawDiffForFile(repo *Repository, startCommit, endCommit string, diff fileArgs = append(fileArgs, "--", file) } // FIXME: graceful: These commands should have a timeout - ctx, _, finished := process.GetManager().AddContext(DefaultContext, fmt.Sprintf("GetRawDiffForFile: [repo_path: %s]", repo.Path)) + ctx, _, finished := process.GetManager().AddContext(repo.Ctx, fmt.Sprintf("GetRawDiffForFile: [repo_path: %s]", repo.Path)) defer finished() var cmd *exec.Cmd From 37f0716207c319dd84d3035f4fadb86bbaff9522 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 30 Nov 2021 19:23:14 +0000 Subject: [PATCH 25/26] improve code documentation Signed-off-by: Andrew Thornton --- modules/process/context.go | 4 ++-- modules/process/manager.go | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/modules/process/context.go b/modules/process/context.go index f8d18bc3645d..6df5bc1513f5 100644 --- a/modules/process/context.go +++ b/modules/process/context.go @@ -8,7 +8,7 @@ import ( "context" ) -// Context is a wrapper around context.Context for having the current pid for this context +// Context is a wrapper around context.Context and contains the current pid for this context type Context struct { context.Context pid IDType @@ -19,7 +19,7 @@ func (c *Context) GetPID() IDType { return c.pid } -// GetParent returns the parent process context if any +// GetParent returns the parent process context (if any) func (c *Context) GetParent() *Context { return GetContext(c.Context) } diff --git a/modules/process/manager.go b/modules/process/manager.go index b632d4d00abb..10a89d04ddb4 100644 --- a/modules/process/manager.go +++ b/modules/process/manager.go @@ -33,10 +33,10 @@ var ( type IDType string // FinishedFunc is a function that marks that the process is finished and can be removed from the process table -// - it is simply an alias for context.CancelFunc and is only for documentary purpose +// - it is simply an alias for context.CancelFunc and is only for documentary purposes type FinishedFunc = context.CancelFunc -// Manager knows about all processes and counts PIDs. +// Manager manages all processes and counts PIDs. type Manager struct { mutex sync.Mutex @@ -57,8 +57,8 @@ func GetManager() *Manager { return manager } -// AddContext create a new context and add it as a process. Once the process is finished, finished must be called in order that the process -// can be removed from the process table. It should not be called until the process is finished but must always be called. +// AddContext creates a new context and adds it as a process. Once the process is finished, finished must be called +// to remove the process from the process table. It should not be called until the process is finished but must always be called. // // cancel should be used to cancel the returned context, however it will not remove the process from the process table. // finished will cancel the returned context and remove it from the process table. @@ -78,8 +78,8 @@ func (pm *Manager) AddContext(parent context.Context, description string) (ctx c }, cancel, finished } -// AddContextTimeout create a new context and add it as a process. Once the process is finished, finished must be called in order that the process -// can be removed from the process table. It should not be called until the process is finsihed but must always be called. +// AddContextTimeout creates a new context and add it as a process. Once the process is finished, finished must be called +// to remove the process from the process table. It should not be called until the process is finsihed but must always be called. // // cancel should be used to cancel the returned context, however it will not remove the process from the process table. // finished will cancel the returned context and remove it from the process table. From ec6b66321edc31ad8ebcaab8fe07c1786d803929 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 30 Nov 2021 19:26:22 +0000 Subject: [PATCH 26/26] use the gitrepo context Signed-off-by: Andrew Thornton --- services/gitdiff/gitdiff.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 8ca5bc2bac80..166660b87e18 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -1304,7 +1304,7 @@ func GetDiff(gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff } timeout := time.Duration(setting.Git.Timeout.Default) * time.Second - ctx, _, finished := process.GetManager().AddContextTimeout(git.DefaultContext, timeout, fmt.Sprintf("GetDiffRange [repo_path: %s]", repoPath)) + ctx, _, finished := process.GetManager().AddContextTimeout(gitRepo.Ctx, timeout, fmt.Sprintf("GetDiffRange [repo_path: %s]", repoPath)) defer finished() argsLength := 6