diff options
| author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2022-05-30 11:08:27 +0300 |
|---|---|---|
| committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2022-05-30 12:18:23 +0300 |
| commit | 4b83e5996056b4a6260d57b7a19180aa4f220c2c (patch) | |
| tree | 0f8932b84622d277b9e67c00334c1f7277ff8348 | |
| parent | a6c5964bb455f77a0c79898f0b99c4c7df3aee3a (diff) | |
catfile: Fix dirtiness check when current object has been fully read
We always return the currently pending object's dirty state in case the
request queue has an object set. In case the object has been fully read
without yet having been closed though this won't account for the queue's
outstanding requests. So ultimately, the queue may now says its clean
even though it still got requests pending.
Fix this issue by not returning early in case there is an object. This
causes us to correctly discard any such processes instead of returning
them to the cache with still-pending requests. This is an issue we have
indeed been observing in RPCs which have limits, like ListLFSPointers.
Amend one of our tests to enable cache reuse of catfile processes, which
does surface the issue previous to this fix.
Changelog: fixed
| -rw-r--r-- | internal/git/catfile/request_queue.go | 4 | ||||
| -rw-r--r-- | internal/gitaly/service/blob/lfs_pointers_test.go | 6 |
2 files changed, 8 insertions, 2 deletions
diff --git a/internal/git/catfile/request_queue.go b/internal/git/catfile/request_queue.go index 62c6c868a..d41ed38dc 100644 --- a/internal/git/catfile/request_queue.go +++ b/internal/git/catfile/request_queue.go @@ -56,8 +56,8 @@ func (q *requestQueue) isDirty() bool { // We must check for the current object first: we cannot queue another object due to the // object lock, but we may queue another request while checking for dirtiness. - if q.currentObject != nil { - return q.currentObject.isDirty() + if q.currentObject != nil && q.currentObject.isDirty() { + return true } if atomic.LoadInt64(&q.outstandingRequests) != 0 { diff --git a/internal/gitaly/service/blob/lfs_pointers_test.go b/internal/gitaly/service/blob/lfs_pointers_test.go index 9e8558209..8bb76da32 100644 --- a/internal/gitaly/service/blob/lfs_pointers_test.go +++ b/internal/gitaly/service/blob/lfs_pointers_test.go @@ -11,12 +11,14 @@ import ( "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v15/internal/git" + "gitlab.com/gitlab-org/gitaly/v15/internal/git/catfile" "gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest" "gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v15/internal/helper/text" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb" "google.golang.org/grpc/codes" + "google.golang.org/grpc/metadata" "google.golang.org/grpc/status" ) @@ -66,6 +68,10 @@ func TestListLFSPointers(t *testing.T) { ctx := testhelper.Context(t) _, repo, _, client := setup(ctx, t) + ctx = testhelper.MergeOutgoingMetadata(ctx, + metadata.Pairs(catfile.SessionIDField, "1"), + ) + for _, tc := range []struct { desc string revs []string |