diff options
author | Vladimir Shushlin <vshushlin@gitlab.com> | 2020-10-13 18:26:49 +0300 |
---|---|---|
committer | Vladimir Shushlin <vshushlin@gitlab.com> | 2020-10-13 18:26:49 +0300 |
commit | 6cba2f2875856a7b4e2d008ca348d7b91098ed35 (patch) | |
tree | 1efb9972ba956a1d732060cb50d5ead1db83e8ab | |
parent | dc721e6866e0efe639279956ac3c26be37933175 (diff) | |
parent | b281e2fb9e153448688a247f91b9bb245130a843 (diff) |
Merge branch 'ensure-that-archives-are-evicted' into 'master'
Ensure that archives are evicted
See merge request gitlab-org/gitlab-pages!371
-rw-r--r-- | internal/vfs/zip/vfs.go | 37 | ||||
-rw-r--r-- | internal/vfs/zip/vfs_test.go | 33 |
2 files changed, 63 insertions, 7 deletions
diff --git a/internal/vfs/zip/vfs.go b/internal/vfs/zip/vfs.go index d6436010..78a77e1c 100644 --- a/internal/vfs/zip/vfs.go +++ b/internal/vfs/zip/vfs.go @@ -4,6 +4,7 @@ import ( "context" "errors" "net/url" + "sync" "time" "github.com/patrickmn/go-cache" @@ -35,7 +36,8 @@ var ( // zipVFS is a simple cached implementation of the vfs.VFS interface type zipVFS struct { - cache *cache.Cache + cache *cache.Cache + cacheLock sync.Mutex dataOffsetCache *lruCache readlinkCache *lruCache @@ -88,10 +90,17 @@ func (fs *zipVFS) Name() string { return "zip" } -// findOrOpenArchive if found in fs.cache refresh if needed and return it. -// otherwise open the archive and try to save it, if saving fails it's because -// the archive has already been cached (e.g. by another concurrent request) -func (fs *zipVFS) findOrOpenArchive(ctx context.Context, path string) (*zipArchive, error) { +// findOrCreateArchive if found in fs.cache refresh if needed and return it. +// otherwise creates the archive entry in a cache and try to save it, +// if saving fails it's because the archive has already been cached +// (e.g. by another concurrent request) +func (fs *zipVFS) findOrCreateArchive(ctx context.Context, path string) (*zipArchive, error) { + // This needs to happen in lock to ensure that + // concurrent access will not remove it + // it is needed due to the bug https://github.com/patrickmn/go-cache/issues/48 + fs.cacheLock.Lock() + defer fs.cacheLock.Unlock() + archive, expiry, found := fs.cache.GetWithExpiration(path) if found { metrics.ZipCacheRequests.WithLabelValues("archive", "hit").Inc() @@ -104,18 +113,32 @@ func (fs *zipVFS) findOrOpenArchive(ctx context.Context, path string) (*zipArchi } else { archive = newArchive(fs, path, DefaultOpenTimeout) + // We call delete to ensure that expired item + // is properly evicted as there's a bug in a cache library: + // https://github.com/patrickmn/go-cache/issues/48 + fs.cache.Delete(path) + // if adding the archive to the cache fails it means it's already been added before // this is done to find concurrent additions. if fs.cache.Add(path, archive, cache.DefaultExpiration) != nil { return nil, errAlreadyCached } + metrics.ZipCacheRequests.WithLabelValues("archive", "miss").Inc() metrics.ZipCachedEntries.WithLabelValues("archive").Inc() } - zipArchive := archive.(*zipArchive) + return archive.(*zipArchive), nil +} + +// findOrOpenArchive gets archive from cache and tries to open it +func (fs *zipVFS) findOrOpenArchive(ctx context.Context, path string) (*zipArchive, error) { + zipArchive, err := fs.findOrCreateArchive(ctx, path) + if err != nil { + return nil, err + } - err := zipArchive.openArchive(ctx) + err = zipArchive.openArchive(ctx) if err != nil { return nil, err } diff --git a/internal/vfs/zip/vfs_test.go b/internal/vfs/zip/vfs_test.go index 434fc84c..a795b214 100644 --- a/internal/vfs/zip/vfs_test.go +++ b/internal/vfs/zip/vfs_test.go @@ -6,7 +6,10 @@ import ( "testing" "time" + "github.com/prometheus/client_golang/prometheus/testutil" "github.com/stretchr/testify/require" + + "gitlab.com/gitlab-org/gitlab-pages/metrics" ) func TestVFSRoot(t *testing.T) { @@ -95,3 +98,33 @@ func TestVFSFindOrOpenArchiveConcurrentAccess(t *testing.T) { return err == errAlreadyCached }, time.Second, time.Nanosecond) } + +func TestVFSFindOrCreateArchiveCacheEvict(t *testing.T) { + testServerURL, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public.zip", nil) + defer cleanup() + + path := testServerURL + "/public.zip" + + vfs := New().(*zipVFS) + + archivesMetric := metrics.ZipCachedEntries.WithLabelValues("archive") + archivesCount := testutil.ToFloat64(archivesMetric) + + // create a new archive and increase counters + archive, err := vfs.findOrOpenArchive(context.Background(), path) + require.NoError(t, err) + require.NotNil(t, archive) + + // inject into cache to be "expired" + // (we could as well wait `defaultCacheExpirationInterval`) + vfs.cache.Set(path, archive, time.Nanosecond) + + // a new object is created + archive2, err := vfs.findOrOpenArchive(context.Background(), path) + require.NoError(t, err) + require.NotNil(t, archive2) + require.NotEqual(t, archive, archive2, "a different archive is returned") + + archivesCountEnd := testutil.ToFloat64(archivesMetric) + require.Equal(t, float64(1), archivesCountEnd-archivesCount, "all expired archives are evicted") +} |