From: zeripath <art27@cantab.net>
Date: Fri, 10 Sep 2021 09:46:06 +0000 (+0100)
Subject: Fix missing close in WalkGitLog (#17008) (#17009)
X-Git-Tag: v1.15.3~8
X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=659b946edaff08d38067a13b9d3157b12b1131d2;p=gitea.git

Fix missing close in WalkGitLog (#17008) (#17009)

Backport #17008

When the external context is cancelled it is possible for the
GitLogReader to not itself be Closed.

This PR does three things:

1. Instead of adding a plain defer it wraps the `g.Close` in a func as
`g` may change.
2. It adds the missing explicit g.Close - although the defer fix makes
this unnecessary.
3. It passes down the external context as the base context for the
GitLogReader meaning that the cancellation of the external context will
pass down automatically.

Fix #17007

Signed-off-by: Andrew Thornton <art27@cantab.net>
---

diff --git a/modules/git/log_name_status.go b/modules/git/log_name_status.go
index 00a1859f7a..7d83ac7270 100644
--- a/modules/git/log_name_status.go
+++ b/modules/git/log_name_status.go
@@ -18,11 +18,16 @@ import (
 )
 
 // LogNameStatusRepo opens git log --raw in the provided repo and returns a stdin pipe, a stdout reader and cancel function
-func LogNameStatusRepo(repository, head, treepath string, paths ...string) (*bufio.Reader, func()) {
+func LogNameStatusRepo(ctx context.Context, repository, head, treepath string, paths ...string) (*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
 	stdoutReader, stdoutWriter := nio.Pipe(buffer.New(32 * 1024))
+
+	// Lets also create a context so that we can absolutely ensure that the command should die when we're done
+	ctx, ctxCancel := context.WithCancel(ctx)
+
 	cancel := func() {
+		ctxCancel()
 		_ = stdoutReader.Close()
 		_ = stdoutWriter.Close()
 	}
@@ -50,7 +55,7 @@ func LogNameStatusRepo(repository, head, treepath string, paths ...string) (*buf
 
 	go func() {
 		stderr := strings.Builder{}
-		err := NewCommand(args...).RunInDirFullPipeline(repository, stdoutWriter, &stderr, nil)
+		err := NewCommandContext(ctx, args...).RunInDirFullPipeline(repository, stdoutWriter, &stderr, nil)
 		if err != nil {
 			_ = stdoutWriter.CloseWithError(ConcatenateError(err, (&stderr).String()))
 		} else {
@@ -75,8 +80,8 @@ type LogNameStatusRepoParser struct {
 }
 
 // NewLogNameStatusRepoParser returns a new parser for a git log raw output
-func NewLogNameStatusRepoParser(repository, head, treepath string, paths ...string) *LogNameStatusRepoParser {
-	rd, cancel := LogNameStatusRepo(repository, head, treepath, paths...)
+func NewLogNameStatusRepoParser(ctx context.Context, repository, head, treepath string, paths ...string) *LogNameStatusRepoParser {
+	rd, cancel := LogNameStatusRepo(ctx, repository, head, treepath, paths...)
 	return &LogNameStatusRepoParser{
 		treepath: treepath,
 		paths:    paths,
@@ -311,8 +316,11 @@ func WalkGitLog(ctx context.Context, repo *Repository, head *Commit, treepath st
 		}
 	}
 
-	g := NewLogNameStatusRepoParser(repo.Path, head.ID.String(), treepath, paths...)
-	defer g.Close()
+	g := NewLogNameStatusRepoParser(ctx, repo.Path, head.ID.String(), treepath, paths...)
+	// don't use defer g.Close() here as g may change its value - instead wrap in a func
+	defer func() {
+		g.Close()
+	}()
 
 	results := make([]string, len(paths))
 	remaining := len(paths)
@@ -331,6 +339,7 @@ heaploop:
 	for {
 		select {
 		case <-ctx.Done():
+			g.Close()
 			return nil, ctx.Err()
 		default:
 		}
@@ -380,7 +389,7 @@ heaploop:
 						remainingPaths = append(remainingPaths, pth)
 					}
 				}
-				g = NewLogNameStatusRepoParser(repo.Path, lastEmptyParent, treepath, remainingPaths...)
+				g = NewLogNameStatusRepoParser(ctx, repo.Path, lastEmptyParent, treepath, remainingPaths...)
 				parentRemaining = map[string]bool{}
 				nextRestart = (remaining * 3) / 4
 				continue heaploop