From 66d222a931cca395ce24dc90ab3141d5a62e0c16 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 25 Apr 2022 22:03:00 +0100 Subject: [PATCH 1/6] 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 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 059f75fb3c0e7..8ae22881bceff 100644 --- a/modules/git/repo_base_gogit.go +++ b/modules/git/repo_base_gogit.go @@ -63,13 +63,17 @@ func OpenRepository(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 c4a0e82c89df1..6e7e2511abcc0 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 OpenRepository(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 4eddcefea0c05bb837b26ec75d1f5be3ab3a86fa Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 26 Apr 2022 07:58:07 +0100 Subject: [PATCH 2/6] 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 8ae22881bceff..14bf4a9317112 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 aa8eeed54e1b080680b83a141e29e19d144bddff Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 26 Apr 2022 21:33:17 +0100 Subject: [PATCH 3/6] Add finalizer warnings Signed-off-by: Andrew Thornton --- modules/git/blob_nogogit.go | 55 ++++++++++++++++++++++++++----- modules/git/repo_base_gogit.go | 38 ++++++++++++++++++++-- modules/git/repo_base_nogogit.go | 56 ++++++++++++++++++++++++++++++-- 3 files changed, 135 insertions(+), 14 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 14bf4a9317112..5b3c4e180e748 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,14 +78,24 @@ func OpenRepository(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() (err error) { - if repo == nil || repo.gogitStorage == nil { + 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 { @@ -88,6 +104,24 @@ func (repo *Repository) Close() (err error) { 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 func (repo *Repository) GoGitRepo() *gogit.Repository { return repo.gogitRepo diff --git a/modules/git/repo_base_nogogit.go b/modules/git/repo_base_nogogit.go index 6e7e2511abcc0..b120e6824e6ae 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 OpenRepository(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() (err error) { + repo.lock.Lock() + defer repo.lock.Unlock() + + return repo.close() +} + +func (repo *Repository) close() (err error) { if repo == nil { return } @@ -105,5 +134,26 @@ func (repo *Repository) Close() (err error) { 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 98f7f7b132830be57acfe8222ba1d568bc151726 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 26 Apr 2022 22:01:59 +0100 Subject: [PATCH 4/6] pass go-git-close error back Signed-off-by: Andrew Thornton --- modules/git/repo_base_gogit.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/modules/git/repo_base_gogit.go b/modules/git/repo_base_gogit.go index 5b3c4e180e748..14725233bba7c 100644 --- a/modules/git/repo_base_gogit.go +++ b/modules/git/repo_base_gogit.go @@ -98,7 +98,8 @@ func (repo *Repository) close() (err error) { 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 From a773621f4c72e60925a62a202e7b84682f97923f Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 26 Apr 2022 22:18:29 +0100 Subject: [PATCH 5/6] 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 b120e6824e6ae..5eab55c9095c7 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() From 849b839ff1befe6ac79fc6fe27e5451ec1f1768f Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 8 May 2022 17:25:18 +0100 Subject: [PATCH 6/6] remove finalizer on close - except for gogit variant which I think can reopen Signed-off-by: Andrew Thornton --- modules/git/blob_nogogit.go | 5 ++++- modules/git/repo_base_nogogit.go | 1 + 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/modules/git/blob_nogogit.go b/modules/git/blob_nogogit.go index b55c4d0c8afad..d46db8f6e949b 100644 --- a/modules/git/blob_nogogit.go +++ b/modules/git/blob_nogogit.go @@ -124,7 +124,10 @@ func (b *blobReader) Close() (err error) { } func (b *blobReader) close() (err error) { - defer b.cancel() + defer func() { + b.cancel() + runtime.SetFinalizer(b, nil) + }() b.closed = true if b.n > 0 { var n int diff --git a/modules/git/repo_base_nogogit.go b/modules/git/repo_base_nogogit.go index 1670fa84dcf86..d7e289b32959d 100644 --- a/modules/git/repo_base_nogogit.go +++ b/modules/git/repo_base_nogogit.go @@ -137,6 +137,7 @@ func (repo *Repository) close() (err error) { repo.checkWriter = nil } repo.closed = true + runtime.SetFinalizer(repo, nil) return }