diff options
author | Jaime Martinez <jmartinez@gitlab.com> | 2020-09-25 05:28:52 +0300 |
---|---|---|
committer | Jaime Martinez <jmartinez@gitlab.com> | 2020-09-25 05:28:52 +0300 |
commit | 003e7145a5bf6faa8991531d77cc4abff4869eda (patch) | |
tree | c437e880bd6f35bba4202ee0b91799de7df1a668 | |
parent | 6230f6525fbe6cb1a75e650185062070621d82a4 (diff) |
Apply suggestions from review
-rw-r--r-- | acceptance_test.go | 3 | ||||
-rw-r--r-- | internal/serving/disk/local/serving.go | 2 | ||||
-rw-r--r-- | internal/serving/disk/symlink/path_test.go | 2 | ||||
-rw-r--r-- | internal/serving/disk/zip/serving.go | 2 | ||||
-rw-r--r-- | internal/testhelpers/tmpdir.go | 2 | ||||
-rw-r--r-- | internal/vfs/local/vfs.go | 13 | ||||
-rw-r--r-- | internal/vfs/local/vfs_test.go | 2 | ||||
-rw-r--r-- | internal/vfs/vfs.go | 13 | ||||
-rw-r--r-- | internal/vfs/zip/vfs.go | 18 | ||||
-rw-r--r-- | internal/vfs/zip/vfs_test.go | 60 |
10 files changed, 55 insertions, 62 deletions
diff --git a/acceptance_test.go b/acceptance_test.go index ff1b22d1..02c71f1d 100644 --- a/acceptance_test.go +++ b/acceptance_test.go @@ -1994,11 +1994,10 @@ func TestZipServing(t *testing.T) { t.Run(name, func(t *testing.T) { response, err := GetPageFromListener(t, httpListener, "zip.gitlab.io", tt.urlSuffix) require.NoError(t, err) + defer response.Body.Close() require.Equal(t, tt.expectedStatusCode, response.StatusCode) if tt.expectedStatusCode == http.StatusOK || tt.expectedStatusCode == http.StatusNotFound { - defer response.Body.Close() - body, err := ioutil.ReadAll(response.Body) require.NoError(t, err) diff --git a/internal/serving/disk/local/serving.go b/internal/serving/disk/local/serving.go index 95aebd10..3b23b8e9 100644 --- a/internal/serving/disk/local/serving.go +++ b/internal/serving/disk/local/serving.go @@ -7,7 +7,7 @@ import ( "gitlab.com/gitlab-org/gitlab-pages/internal/vfs/local" ) -var instance = disk.New(vfs.Instrumented(local.New("local"))) +var instance = disk.New(vfs.Instrumented(&local.VFS{})) // Instance returns a serving instance that is capable of reading files // from the disk diff --git a/internal/serving/disk/symlink/path_test.go b/internal/serving/disk/symlink/path_test.go index 5e7ea16d..ef9726c1 100644 --- a/internal/serving/disk/symlink/path_test.go +++ b/internal/serving/disk/symlink/path_test.go @@ -21,7 +21,7 @@ import ( "gitlab.com/gitlab-org/gitlab-pages/internal/vfs/local" ) -var fs = vfs.Instrumented(local.New("local")) +var fs = vfs.Instrumented(&local.VFS{}) type EvalSymlinksTest struct { // If dest is empty, the path is created; otherwise the dest is symlinked to the path. diff --git a/internal/serving/disk/zip/serving.go b/internal/serving/disk/zip/serving.go index e9a5f1fa..95894fc9 100644 --- a/internal/serving/disk/zip/serving.go +++ b/internal/serving/disk/zip/serving.go @@ -7,7 +7,7 @@ import ( "gitlab.com/gitlab-org/gitlab-pages/internal/vfs/zip" ) -var instance = disk.New(vfs.Instrumented(zip.New("zip"))) +var instance = disk.New(vfs.Instrumented(zip.New())) // Instance returns a serving instance that is capable of reading files // from a zip archives opened from a URL, most likely stored in object storage diff --git a/internal/testhelpers/tmpdir.go b/internal/testhelpers/tmpdir.go index 89223621..81f0781b 100644 --- a/internal/testhelpers/tmpdir.go +++ b/internal/testhelpers/tmpdir.go @@ -13,7 +13,7 @@ import ( "gitlab.com/gitlab-org/gitlab-pages/internal/vfs/local" ) -var fs = vfs.Instrumented(local.New("local")) +var fs = vfs.Instrumented(&local.VFS{}) func TmpDir(t *testing.T, pattern string) (vfs.Root, string, func()) { tmpDir, err := ioutil.TempDir("", pattern) diff --git a/internal/vfs/local/vfs.go b/internal/vfs/local/vfs.go index 0ece253d..ca74dfbe 100644 --- a/internal/vfs/local/vfs.go +++ b/internal/vfs/local/vfs.go @@ -11,16 +11,7 @@ import ( var errNotDirectory = errors.New("path needs to be a directory") -type VFS struct { - name string -} - -// New instance of VFS -func New(name string) *VFS { - return &VFS{ - name: name, - } -} +type VFS struct{} func (fs VFS) Root(ctx context.Context, path string) (vfs.Root, error) { rootPath, err := filepath.Abs(path) @@ -46,5 +37,5 @@ func (fs VFS) Root(ctx context.Context, path string) (vfs.Root, error) { } func (fs *VFS) Name() string { - return fs.name + return "local" } diff --git a/internal/vfs/local/vfs_test.go b/internal/vfs/local/vfs_test.go index 0c56cbbc..ec67d595 100644 --- a/internal/vfs/local/vfs_test.go +++ b/internal/vfs/local/vfs_test.go @@ -11,7 +11,7 @@ import ( "github.com/stretchr/testify/require" ) -var localVFS = New("local_test") +var localVFS = &VFS{} func tmpDir(t *testing.T) (string, func()) { tmpDir, err := ioutil.TempDir("", "vfs") diff --git a/internal/vfs/vfs.go b/internal/vfs/vfs.go index bd87dc7c..7bd51db2 100644 --- a/internal/vfs/vfs.go +++ b/internal/vfs/vfs.go @@ -16,20 +16,19 @@ type VFS interface { } func Instrumented(fs VFS) VFS { - return &instrumentedVFS{fs: fs, name: fs.Name()} + return &instrumentedVFS{fs: fs} } type instrumentedVFS struct { - fs VFS - name string + fs VFS } func (i *instrumentedVFS) increment(operation string, err error) { - metrics.VFSOperations.WithLabelValues(i.name, operation, strconv.FormatBool(err == nil)).Inc() + metrics.VFSOperations.WithLabelValues(i.fs.Name(), operation, strconv.FormatBool(err == nil)).Inc() } func (i *instrumentedVFS) log() *log.Entry { - return log.WithField("vfs", i.name) + return log.WithField("vfs", i.fs.Name()) } func (i *instrumentedVFS) Root(ctx context.Context, path string) (Root, error) { @@ -45,9 +44,9 @@ func (i *instrumentedVFS) Root(ctx context.Context, path string) (Root, error) { return nil, err } - return &instrumentedRoot{root: root, name: i.name, rootPath: path}, nil + return &instrumentedRoot{root: root, name: i.fs.Name(), rootPath: path}, nil } func (i *instrumentedVFS) Name() string { - return i.name + return i.fs.Name() } diff --git a/internal/vfs/zip/vfs.go b/internal/vfs/zip/vfs.go index b5405627..4e3e2416 100644 --- a/internal/vfs/zip/vfs.go +++ b/internal/vfs/zip/vfs.go @@ -24,14 +24,12 @@ var ( // zipVFS is a simple cached implementation of the vfs.VFS interface type zipVFS struct { - name string cache *cache.Cache } // New creates a zipVFS instance that can be used by a serving request -func New(name string) vfs.VFS { +func New() vfs.VFS { return &zipVFS{ - name: name, // TODO: add cache operation callbacks https://gitlab.com/gitlab-org/gitlab-pages/-/issues/465 cache: cache.New(defaultCacheExpirationInterval, defaultCacheRefreshInterval), } @@ -62,34 +60,36 @@ func (fs *zipVFS) Root(ctx context.Context, path string) (vfs.Root, error) { } func (fs *zipVFS) Name() string { - return fs.name + 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 request) +// the archive has already been cached (e.g. by another concurrent request) func (fs *zipVFS) findOrOpenArchive(ctx context.Context, path string) (*zipArchive, error) { archive, expiry, found := fs.cache.GetWithExpiration(path) if found { + // TODO: do not refreshed errored archives https://gitlab.com/gitlab-org/gitlab-pages/-/merge_requests/351 if time.Until(expiry) < defaultCacheRefreshInterval { // refresh item - fs.cache.Set(path, archive, cache.DefaultExpiration) + fs.cache.SetDefault(path, archive) } } else { archive = newArchive(path, DefaultOpenTimeout) // 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 } } - zipDir, ok := archive.(*zipArchive) + zipArchive, ok := archive.(*zipArchive) if !ok { // fail if the found archive in cache is not a zipArchive (just for type safety) return nil, errNotZipArchive } - err := zipDir.openArchive(ctx) - return zipDir, err + err := zipArchive.openArchive(ctx) + return zipArchive, err } diff --git a/internal/vfs/zip/vfs_test.go b/internal/vfs/zip/vfs_test.go index 54b9d9bb..62b5f62c 100644 --- a/internal/vfs/zip/vfs_test.go +++ b/internal/vfs/zip/vfs_test.go @@ -3,14 +3,14 @@ package zip import ( "context" "io/ioutil" - "sync" "testing" + "time" "github.com/stretchr/testify/require" ) func TestVFSRoot(t *testing.T) { - testServerURL, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public.zip") + url, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public.zip") defer cleanup() tests := map[string]struct { @@ -30,11 +30,11 @@ func TestVFSRoot(t *testing.T) { }, } - testZipVFS := New("zip_test") + vfs := New() for name, tt := range tests { t.Run(name, func(t *testing.T) { - rootVFS, err := testZipVFS.Root(context.Background(), testServerURL+tt.path) + root, err := vfs.Root(context.Background(), url+tt.path) if tt.expectedErrMsg != "" { require.Error(t, err) require.Contains(t, err.Error(), tt.expectedErrMsg) @@ -42,52 +42,56 @@ func TestVFSRoot(t *testing.T) { } require.NoError(t, err) - require.IsType(t, &zipArchive{}, rootVFS) + require.IsType(t, &zipArchive{}, root) - f, err := rootVFS.Open(context.Background(), "index.html") + f, err := root.Open(context.Background(), "index.html") require.NoError(t, err) content, err := ioutil.ReadAll(f) require.NoError(t, err) require.Equal(t, "zip.gitlab.io/project/index.html\n", string(content)) - fi, err := rootVFS.Lstat(context.Background(), "index.html") + fi, err := root.Lstat(context.Background(), "index.html") require.NoError(t, err) require.Equal(t, "index.html", fi.Name()) - link, err := rootVFS.Readlink(context.Background(), "symlink.html") + link, err := root.Readlink(context.Background(), "symlink.html") require.NoError(t, err) require.Equal(t, "subdir/linked.html", link) }) } } -func TestVFSRootMultipleRequests(t *testing.T) { +func TestVFSFindOrOpenArchiveConcurrentAccess(t *testing.T) { testServerURL, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public.zip") defer cleanup() - testZipVFS := New("zip_test") - - wg := &sync.WaitGroup{} - wg.Add(5) + path := testServerURL + "/public.zip" - for i := 0; i < 5; i++ { - go func(i int) { - defer wg.Done() + vfs := New().(*zipVFS) + root, err := vfs.Root(context.Background(), path) + require.NoError(t, err) - vfs, err := testZipVFS.Root(context.Background(), testServerURL+"/public.zip") - require.NoError(t, err, i) + done := make(chan struct{}) + defer close(done) - f, err := vfs.Open(context.Background(), "index.html") - require.NoError(t, err, i) - - content, err := ioutil.ReadAll(f) - require.NoError(t, err, i) + // Try to hit a condition between the invocation + // of cache.GetWithExpiration and cache.Add + go func() { + for { + select { + case <-done: + return - require.Equal(t, "zip.gitlab.io/project/index.html\n", string(content), i) - }(i) - } + default: + vfs.cache.Flush() + vfs.cache.SetDefault(path, root) + } + } + }() - wg.Wait() - // TODO: add tests for cache callbacks https://gitlab.com/gitlab-org/gitlab-pages/-/issues/465 + require.Eventually(t, func() bool { + _, err := vfs.findOrOpenArchive(context.Background(), path) + return err == errAlreadyCached + }, time.Second, time.Nanosecond) } |