Apply suggestions from review

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)
 }