catfile: Deduplicate logic to tear down processes

Both the `CatfileReader` and `CatfileInfoReader` have the same logic to
tear down the process when the context gets cancelled. This logic can
easily be unified into the cache: it already got a Goroutine that waits
for the context to get cancelled to decrement the process counter, so it
is a perfect fit to also host the other logic.

Refactor the code to do this. While this reduces complexity, it also
cuts the number of Goroutines in half.

3 files changed, 7 insertions, 19 deletions

diff --git a/internal/git/catfile/cache.go b/internal/git/catfile/cache.go
index f63fd36ec..94f163d0f 100644
--- a/internal/git/catfile/cache.go
+++ b/internal/git/catfile/cache.go
@@ -178,7 +178,7 @@ func (c *ProcessCache) Stop() {
 func (c *ProcessCache) ObjectReader(ctx context.Context, repo git.RepositoryExecutor) (ObjectReader, error) {
 	cacheable, err := c.getOrCreateProcess(ctx, repo, &c.objectReaders, func(ctx context.Context) (cacheable, error) {
 		return newObjectReader(ctx, repo, c.catfileLookupCounter)
-	})
+	}, "catfile.ObjectReader")
 	if err != nil {
 		return nil, err
 	}
@@ -195,7 +195,7 @@ func (c *ProcessCache) ObjectReader(ctx context.Context, repo git.RepositoryExec
 func (c *ProcessCache) ObjectInfoReader(ctx context.Context, repo git.RepositoryExecutor) (ObjectInfoReader, error) {
 	cacheable, err := c.getOrCreateProcess(ctx, repo, &c.objectInfoReaders, func(ctx context.Context) (cacheable, error) {
 		return newObjectInfoReader(ctx, repo, c.catfileLookupCounter)
-	})
+	}, "catfile.ObjectInfoReader")
 	if err != nil {
 		return nil, err
 	}
@@ -213,6 +213,7 @@ func (c *ProcessCache) getOrCreateProcess(
 	repo repository.GitRepo,
 	processes *processes,
 	create func(context.Context) (cacheable, error),
+	spanName string,
 ) (_ cacheable, returnedErr error) {
 	requestDone := ctx.Done()
 	if requestDone == nil {
@@ -256,6 +257,8 @@ func (c *ProcessCache) getOrCreateProcess(
 		ctx = opentracing.ContextWithSpan(ctx, nil)
 	}
 
+	span, ctx := opentracing.StartSpanFromContext(ctx, spanName)
+
 	process, err := create(ctx)
 	if err != nil {
 		return nil, err
@@ -272,6 +275,8 @@ func (c *ProcessCache) getOrCreateProcess(
 	c.currentCatfileProcesses.Inc()
 	go func() {
 		<-ctx.Done()
+		process.close()
+		span.Finish()
 		c.currentCatfileProcesses.Dec()
 	}()
 
diff --git a/internal/git/catfile/object_info_reader.go b/internal/git/catfile/object_info_reader.go
index 300459e81..e7773acd2 100644
--- a/internal/git/catfile/object_info_reader.go
+++ b/internal/git/catfile/object_info_reader.go
@@ -8,7 +8,6 @@ import (
 	"strings"
 	"sync/atomic"
 
-	"github.com/opentracing/opentracing-go"
 	"github.com/prometheus/client_golang/prometheus"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/command"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
@@ -139,8 +138,6 @@ func newObjectInfoReader(
 	repo git.RepositoryExecutor,
 	counter *prometheus.CounterVec,
 ) (*objectInfoReader, error) {
-	span, ctx := opentracing.StartSpanFromContext(ctx, "catfile.ObjectInfoReader")
-
 	batchCmd, err := repo.Exec(ctx,
 		git.SubCmd{
 			Name: "cat-file",
@@ -163,12 +160,6 @@ func newObjectInfoReader(
 			stdin:  bufio.NewWriter(batchCmd),
 		},
 	}
-	go func() {
-		<-ctx.Done()
-		// This is crucial to prevent leaking file descriptors.
-		objectInfoReader.close()
-		span.Finish()
-	}()
 
 	return objectInfoReader, nil
 }
diff --git a/internal/git/catfile/object_reader.go b/internal/git/catfile/object_reader.go
index 7020adcbe..8dedb1572 100644
--- a/internal/git/catfile/object_reader.go
+++ b/internal/git/catfile/object_reader.go
@@ -8,7 +8,6 @@ import (
 	"os"
 	"sync/atomic"
 
-	"github.com/opentracing/opentracing-go"
 	"github.com/prometheus/client_golang/prometheus"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/command"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
@@ -61,8 +60,6 @@ func newObjectReader(
 	repo git.RepositoryExecutor,
 	counter *prometheus.CounterVec,
 ) (*objectReader, error) {
-	span, ctx := opentracing.StartSpanFromContext(ctx, "catfile.ObjectReader")
-
 	batchCmd, err := repo.Exec(ctx,
 		git.SubCmd{
 			Name: "cat-file",
@@ -86,11 +83,6 @@ func newObjectReader(
 			stdin:         bufio.NewWriter(batchCmd),
 		},
 	}
-	go func() {
-		<-ctx.Done()
-		objectReader.close()
-		span.Finish()
-	}()
 
 	return objectReader, nil
 }