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>

3 files changed, 19 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 00ef9a99ea..602924ecd6 100644
--- a/routers/repo/blame.go
+++ b/routers/repo/blame.go
@@ -141,7 +141,13 @@ func RefBlame(ctx *context.Context) {
 	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