Ensure BlameReaders close at end of request (#12102) (#12103) Backport #12102 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 <art27@cantab.net>

Changes

3 changed files (+19/-8)

• modules/git/blame.go
  • @@ -79,7 +79,9 @@
    // 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 @@ return nil
    } // 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
• modules/git/blame_test.go
  • @@ -5,6 +5,7 @@
    package git import ( + "context" "io/ioutil" "testing"
  • @@ -93,8 +94,10 @@
    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) }
• routers/repo/blame.go
  • @@ -141,7 +141,13 @@
    ctx.Data["FileSize"] = blob.Size() ctx.Data["FileName"] = blob.Name() - blameReader, err := git.CreateBlameReader(models.RepoPath(userName, repoName), commitID, fileName) + ctx.Data["NumLines"], err = blob.GetBlobLineCount() + if err != nil { + ctx.NotFound("GetBlobLineCount", err) + return + } + + blameReader, err := git.CreateBlameReader(ctx.Req.Context(), models.RepoPath(userName, repoName), commitID, fileName) if err != nil { ctx.NotFound("CreateBlameReader", err) return