From 858c35b73184aa94242076d417ad09a74c55aebf Mon Sep 17 00:00:00 2001 From: zeripath Date: Wed, 1 Jul 2020 14:01:17 +0100 Subject: [PATCH] Ensure BlameReaders close at end of request (#12102) #11716 reports multiple git blame processes hanging around this was thought to be due to timeouts, however on closer look this appears to be due to the Close() function of the BlameReader hanging with a blocked stdout pipe. This PR fixes this Close function to: * Cancel the context of the cmd * Close the StdoutReader - ensuring that the output pipe is closed Further it makes the context of the `git blame` command a child of the request context - ensuring that even if Close() is not called, on cancellation of the Request the blame is command will also be cancelled. Fixes #11716 Closes #11727 Signed-off-by: Andrew Thornton --- modules/git/blame.go | 14 ++++++++------ modules/git/blame_test.go | 5 ++++- routers/repo/blame.go | 2 +- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/modules/git/blame.go b/modules/git/blame.go index 5a9ae9a74f..9aa77dc65b 100644 --- a/modules/git/blame.go +++ b/modules/git/blame.go @@ -79,7 +79,9 @@ 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) - defer r.cancel() + r.cancel() + + _ = r.output.Close() if err := r.cmd.Wait(); err != nil { return fmt.Errorf("Wait: %v", err) @@ -89,19 +91,19 @@ func (r *BlameReader) Close() error { } // CreateBlameReader creates reader for given repository, commit and file -func CreateBlameReader(repoPath, commitID, file string) (*BlameReader, error) { +func CreateBlameReader(ctx context.Context, repoPath, commitID, file string) (*BlameReader, error) { gitRepo, err := OpenRepository(repoPath) if err != nil { return nil, err } gitRepo.Close() - return createBlameReader(repoPath, GitExecutable, "blame", commitID, "--porcelain", "--", file) + return createBlameReader(ctx, repoPath, GitExecutable, "blame", commitID, "--porcelain", "--", file) } -func createBlameReader(dir string, command ...string) (*BlameReader, error) { - // FIXME: graceful: This should have a timeout - ctx, cancel := context.WithCancel(DefaultContext) +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) cmd := exec.CommandContext(ctx, command[0], command[1:]...) cmd.Dir = dir cmd.Stderr = os.Stderr diff --git a/modules/git/blame_test.go b/modules/git/blame_test.go index 1752312d81..734d63ee14 100644 --- a/modules/git/blame_test.go +++ b/modules/git/blame_test.go @@ -5,6 +5,7 @@ package git import ( + "context" "io/ioutil" "testing" @@ -93,8 +94,10 @@ func TestReadingBlameOutput(t *testing.T) { if _, err = tempFile.WriteString(exampleBlame); err != nil { panic(err) } + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() - blameReader, err := createBlameReader("", "cat", tempFile.Name()) + blameReader, err := createBlameReader(ctx, "", "cat", tempFile.Name()) if err != nil { panic(err) } diff --git a/routers/repo/blame.go b/routers/repo/blame.go index 9d4179abf7..69becc50d4 100644 --- a/routers/repo/blame.go +++ b/routers/repo/blame.go @@ -123,7 +123,7 @@ func RefBlame(ctx *context.Context) { return } - blameReader, err := git.CreateBlameReader(models.RepoPath(userName, repoName), commitID, fileName) + blameReader, err := git.CreateBlameReader(ctx.Req.Context(), models.RepoPath(userName, repoName), commitID, fileName) if err != nil { ctx.NotFound("CreateBlameReader", err) return