From 7730858c629f020ad6904f86385a85ce4bb74001 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 25 Apr 2022 22:03:00 +0100 Subject: [PATCH 1/4] Add finalizers to ensure that repos are closed and blobreaders are closed It may be prudent to add runtime finalizers to the git.Repository and git.blobReader objects to absolutely ensure that these are both properly cancelled, cleaned and closed out. This commit is a backport of an extract from #19448 Signed-off-by: Andrew Thornton --- modules/git/blob_nogogit.go | 8 ++++++-- modules/git/repo_base_gogit.go | 8 ++++++-- modules/git/repo_base_nogogit.go | 3 +++ 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/modules/git/blob_nogogit.go b/modules/git/blob_nogogit.go index aabf1b34aded8..7a36452a70aab 100644 --- a/modules/git/blob_nogogit.go +++ b/modules/git/blob_nogogit.go @@ -12,6 +12,7 @@ import ( "bytes" "io" "math" + "runtime" "code.gitea.io/gitea/modules/log" ) @@ -54,11 +55,14 @@ func (b *Blob) DataAsync() (io.ReadCloser, error) { return io.NopCloser(bytes.NewReader(bs)), err } - return &blobReader{ + br := &blobReader{ rd: rd, n: size, cancel: cancel, - }, nil + } + runtime.SetFinalizer(br, func(br *blobReader) { br.Close() }) + + return br, nil } // Size returns the uncompressed size of the blob diff --git a/modules/git/repo_base_gogit.go b/modules/git/repo_base_gogit.go index 72995265622e5..28a0071f0a959 100644 --- a/modules/git/repo_base_gogit.go +++ b/modules/git/repo_base_gogit.go @@ -63,13 +63,17 @@ func OpenRepositoryCtx(ctx context.Context, repoPath string) (*Repository, error return nil, err } - return &Repository{ + repo := &Repository{ Path: repoPath, gogitRepo: gogitRepo, gogitStorage: storage, tagCache: newObjectCache(), Ctx: ctx, - }, nil + } + + runtime.SetFinalizer(repo, func(repo *Repository) { repo.Close() }) + + return repo, nil } // Close this repository, in particular close the underlying gogitStorage if this is not nil diff --git a/modules/git/repo_base_nogogit.go b/modules/git/repo_base_nogogit.go index e264fd4a14132..31f5de94552e3 100644 --- a/modules/git/repo_base_nogogit.go +++ b/modules/git/repo_base_nogogit.go @@ -13,6 +13,7 @@ import ( "context" "errors" "path/filepath" + "runtime" "code.gitea.io/gitea/modules/log" ) @@ -64,6 +65,8 @@ func OpenRepositoryCtx(ctx context.Context, repoPath string) (*Repository, error repo.batchWriter, repo.batchReader, repo.batchCancel = CatFileBatch(ctx, repoPath) repo.checkWriter, repo.checkReader, repo.checkCancel = CatFileBatchCheck(ctx, repo.Path) + runtime.SetFinalizer(repo, func(repo *Repository) { repo.Close() }) + return repo, nil } From 5671b8fd8edd08d2fe74feffbe40d944eebbd254 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 26 Apr 2022 07:58:07 +0100 Subject: [PATCH 2/4] oops missed runtime import Signed-off-by: Andrew Thornton --- modules/git/repo_base_gogit.go | 1 + 1 file changed, 1 insertion(+) diff --git a/modules/git/repo_base_gogit.go b/modules/git/repo_base_gogit.go index 28a0071f0a959..fedb8d8404a0c 100644 --- a/modules/git/repo_base_gogit.go +++ b/modules/git/repo_base_gogit.go @@ -12,6 +12,7 @@ import ( "context" "errors" "path/filepath" + "runtime" gitealog "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" From 4a09045aba097c06ed0cc35a80725d3350ae9113 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 26 Apr 2022 21:33:17 +0100 Subject: [PATCH 3/4] Add finalizer warnings Signed-off-by: Andrew Thornton --- modules/git/blob_nogogit.go | 55 ++++++++++++++++++++++++----- modules/git/repo_base_gogit.go | 44 +++++++++++++++++++++--- modules/git/repo_base_nogogit.go | 59 +++++++++++++++++++++++++++++--- 3 files changed, 141 insertions(+), 17 deletions(-) diff --git a/modules/git/blob_nogogit.go b/modules/git/blob_nogogit.go index 7a36452a70aab..512dea1eb0b20 100644 --- a/modules/git/blob_nogogit.go +++ b/modules/git/blob_nogogit.go @@ -13,8 +13,10 @@ import ( "io" "math" "runtime" + "sync" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/process" ) // Blob represents a Git object. @@ -56,11 +58,12 @@ func (b *Blob) DataAsync() (io.ReadCloser, error) { } br := &blobReader{ + repo: b.repo, rd: rd, n: size, cancel: cancel, } - runtime.SetFinalizer(br, func(br *blobReader) { br.Close() }) + runtime.SetFinalizer(br, (*blobReader).finalizer) return br, nil } @@ -90,6 +93,10 @@ func (b *Blob) Size() int64 { } type blobReader struct { + lock sync.Mutex + closed bool + + repo *Repository rd *bufio.Reader n int64 cancel func() @@ -108,27 +115,57 @@ func (b *blobReader) Read(p []byte) (n int, err error) { } // Close implements io.Closer -func (b *blobReader) Close() error { +func (b *blobReader) Close() (err error) { + b.lock.Lock() + defer b.lock.Unlock() + if b.closed { + return + } + return b.close() +} + +func (b *blobReader) close() (err error) { defer b.cancel() + b.closed = true if b.n > 0 { + var n int for b.n > math.MaxInt32 { - n, err := b.rd.Discard(math.MaxInt32) + n, err = b.rd.Discard(math.MaxInt32) b.n -= int64(n) if err != nil { - return err + return } b.n -= math.MaxInt32 } - n, err := b.rd.Discard(int(b.n)) + n, err = b.rd.Discard(int(b.n)) b.n -= int64(n) if err != nil { - return err + return } } if b.n == 0 { - _, err := b.rd.Discard(1) + _, err = b.rd.Discard(1) b.n-- - return err + return + } + return +} + +func (b *blobReader) finalizer() error { + if b == nil { + return nil + } + b.lock.Lock() + defer b.lock.Unlock() + if b.closed { + return nil } - return nil + + pid := "" + if b.repo.Ctx != nil { + pid = " from PID: " + string(process.GetPID(b.repo.Ctx)) + } + log.Error("Finalizer running on unclosed blobReader%s: %s%s", pid, b.repo.Path) + + return b.close() } diff --git a/modules/git/repo_base_gogit.go b/modules/git/repo_base_gogit.go index fedb8d8404a0c..183c7750ba77d 100644 --- a/modules/git/repo_base_gogit.go +++ b/modules/git/repo_base_gogit.go @@ -13,8 +13,11 @@ import ( "errors" "path/filepath" "runtime" + "sync" + "code.gitea.io/gitea/modules/log" gitealog "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/process" "code.gitea.io/gitea/modules/setting" "github.com/go-git/go-billy/v5/osfs" @@ -29,6 +32,9 @@ type Repository struct { tagCache *ObjectCache + lock sync.Mutex + closed bool + gogitRepo *gogit.Repository gogitStorage *filesystem.Storage gpgSettings *GPGSettings @@ -72,19 +78,49 @@ func OpenRepositoryCtx(ctx context.Context, repoPath string) (*Repository, error Ctx: ctx, } - runtime.SetFinalizer(repo, func(repo *Repository) { repo.Close() }) + runtime.SetFinalizer(repo, (*Repository).finalizer) return repo, nil } // Close this repository, in particular close the underlying gogitStorage if this is not nil -func (repo *Repository) Close() { - if repo == nil || repo.gogitStorage == nil { +func (repo *Repository) Close() (err error) { + if repo == nil { + return + } + repo.lock.Lock() + defer repo.lock.Unlock() + return repo.close() +} + +func (repo *Repository) close() (err error) { + repo.closed = true + if repo.gogitStorage == nil { return } - if err := repo.gogitStorage.Close(); err != nil { + err = repo.gogitStorage.Close() + if err != nil { gitealog.Error("Error closing storage: %v", err) } + return +} + +func (repo *Repository) finalizer() error { + if repo == nil { + return nil + } + repo.lock.Lock() + defer repo.lock.Unlock() + if !repo.closed { + pid := "" + if repo.Ctx != nil { + pid = " from PID: " + string(process.GetPID(repo.Ctx)) + } + log.Error("Finalizer running on unclosed repository%s: %s%s", pid, repo.Path) + } + + // We still need to run the close fn as it may be possible to reopen the gogitrepo after close + return repo.close() } // GoGitRepo gets the go-git repo representation diff --git a/modules/git/repo_base_nogogit.go b/modules/git/repo_base_nogogit.go index 31f5de94552e3..cc35f38f6fd4b 100644 --- a/modules/git/repo_base_nogogit.go +++ b/modules/git/repo_base_nogogit.go @@ -14,8 +14,10 @@ import ( "errors" "path/filepath" "runtime" + "sync" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/process" ) // Repository represents a Git repository. @@ -26,6 +28,10 @@ type Repository struct { gpgSettings *GPGSettings + lock sync.Mutex + + closed bool + batchCancel context.CancelFunc batchReader *bufio.Reader batchWriter WriteCloserError @@ -65,31 +71,54 @@ func OpenRepositoryCtx(ctx context.Context, repoPath string) (*Repository, error repo.batchWriter, repo.batchReader, repo.batchCancel = CatFileBatch(ctx, repoPath) repo.checkWriter, repo.checkReader, repo.checkCancel = CatFileBatchCheck(ctx, repo.Path) - runtime.SetFinalizer(repo, func(repo *Repository) { repo.Close() }) + runtime.SetFinalizer(repo, (*Repository).finalizer) return repo, nil } // CatFileBatch obtains a CatFileBatch for this repository func (repo *Repository) CatFileBatch(ctx context.Context) (WriteCloserError, *bufio.Reader, func()) { - if repo.batchCancel == nil || repo.batchReader.Buffered() > 0 { + repo.lock.Lock() + defer repo.lock.Unlock() + + if repo.closed || repo.batchReader.Buffered() > 0 { log.Debug("Opening temporary cat file batch for: %s", repo.Path) return CatFileBatch(ctx, repo.Path) } + + if repo.batchCancel == nil { + repo.batchWriter, repo.batchReader, repo.batchCancel = CatFileBatch(ctx, repo.Path) + } + return repo.batchWriter, repo.batchReader, func() {} } // CatFileBatchCheck obtains a CatFileBatchCheck for this repository func (repo *Repository) CatFileBatchCheck(ctx context.Context) (WriteCloserError, *bufio.Reader, func()) { - if repo.checkCancel == nil || repo.checkReader.Buffered() > 0 { + repo.lock.Lock() + defer repo.lock.Unlock() + + if repo.closed || repo.checkReader.Buffered() > 0 { log.Debug("Opening temporary cat file batch-check: %s", repo.Path) return CatFileBatchCheck(ctx, repo.Path) } + + if repo.checkCancel == nil { + repo.checkWriter, repo.checkReader, repo.checkCancel = CatFileBatchCheck(ctx, repo.Path) + } + return repo.checkWriter, repo.checkReader, func() {} } // Close this repository, in particular close the underlying gogitStorage if this is not nil -func (repo *Repository) Close() { +func (repo *Repository) Close() (err error) { + repo.lock.Lock() + defer repo.lock.Unlock() + + return repo.close() +} + +func (repo *Repository) close() (err error) { if repo == nil { return } @@ -105,4 +134,26 @@ func (repo *Repository) Close() { repo.checkReader = nil repo.checkWriter = nil } + repo.closed = true + return +} + +func (repo *Repository) finalizer() (err error) { + if repo == nil { + return + } + repo.lock.Lock() + defer repo.lock.Unlock() + if repo.closed { + return nil + } + + if repo.batchCancel != nil || repo.checkCancel != nil { + pid := "" + if repo.Ctx != nil { + pid = " from PID: " + string(process.GetPID(repo.Ctx)) + } + log.Error("Finalizer running on unclosed repository%s: %s%s", pid, repo.Path) + } + return repo.close() } From a19220a3c4a89b8ef45a698aa1c33e6e8bb6f575 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 26 Apr 2022 22:19:49 +0100 Subject: [PATCH 4/4] prevent NPE Signed-off-by: Andrew Thornton --- modules/git/repo_base_nogogit.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/modules/git/repo_base_nogogit.go b/modules/git/repo_base_nogogit.go index cc35f38f6fd4b..5499edb08b291 100644 --- a/modules/git/repo_base_nogogit.go +++ b/modules/git/repo_base_nogogit.go @@ -112,6 +112,9 @@ func (repo *Repository) CatFileBatchCheck(ctx context.Context) (WriteCloserError // Close this repository, in particular close the underlying gogitStorage if this is not nil func (repo *Repository) Close() (err error) { + if repo == nil { + return + } repo.lock.Lock() defer repo.lock.Unlock()