Improve code layout

1 files changed, 40 insertions, 36 deletions

diff --git a/internal/vfs/zip/vfs.go b/internal/vfs/zip/vfs.go
index ccce8b3c..dfa2f38e 100644
--- a/internal/vfs/zip/vfs.go
+++ b/internal/vfs/zip/vfs.go
@@ -11,13 +11,14 @@ import (
 )
 
 const (
-	// TODO: should we make zip root cache configurable?
+	// TODO: make these configurable https://gitlab.com/gitlab-org/gitlab-pages/-/issues/464
 	cacheExpirationInterval = time.Minute
 	cacheRefreshInterval    = time.Minute / 2
 )
 
 var (
 	errNotZipArchive = errors.New("cached item is not a zip archive")
+	errAlreadyCached = errors.New("archive already cached")
 )
 
 // zipVFS is a simple cached implementation of the vfs.VFS interface
@@ -26,54 +27,57 @@ type zipVFS struct {
 }
 
 // Root opens an archive given a URL path and returns an instance of zipArchive
-// that implements the vfs.Root.
-// To avoid using locks, the function runs in a for loop until an archive is
-// either found or created and saved. If there's an error while saving the
-// archive to the given path because it already exists, the for loop will
-// continue to try and find the already saved archive again.
+// that implements the vfs.VFS interface.
+// To avoid using locks, the findOrOpenArchive function runs inside of a for
+// loop until an archive is either found or created and saved.
+// If findOrOpenArchive returns errAlreadyCached, the for loop will continue
+// to try and find the cached archive or return if there's an error, for example
+// if the context is canceled.
 func (fs *zipVFS) Root(ctx context.Context, path string) (vfs.Root, error) {
 	// we do it in loop to not use any additional locks
 	for {
-		archive, expiry, found := fs.cache.GetWithExpiration(path)
-		if found {
-			if time.Until(expiry) < cacheRefreshInterval {
-				// refresh item
-				fs.cache.Set(path, archive, cache.DefaultExpiration)
-			}
-		} else {
-			archive = newArchive(path, DefaultOpenTimeout)
-
-			// if it errors, it means that it is already added
-			// retry again to get it
-			// if adding the archive to the cache fails it means it's already been added before, retry getting archive
-			if fs.cache.Add(path, archive, cache.DefaultExpiration) != nil {
-				continue
-			}
-		}
-
-		zipDir, ok := archive.(*zipArchive)
-		if !ok {
-			// fail if the found archive in cache is not a zipArchive
-			return nil, errNotZipArchive
+		root, err := fs.findOrOpenArchive(ctx, path)
+		if err == errAlreadyCached {
+			continue
 		}
 
-		err := zipDir.openArchive(ctx)
-		return zipDir, err
+		return root, err
 	}
 }
 
 // New creates a zipVFS instance that can be used by a serving request
 func New() vfs.VFS {
-	zipVFS := &zipVFS{
+	return &zipVFS{
+		// TODO: add cache operation callbacks https://gitlab.com/gitlab-org/gitlab-pages/-/issues/465
 		cache: cache.New(cacheExpirationInterval, cacheRefreshInterval),
 	}
+}
 
-	zipVFS.cache.OnEvicted(func(path string, object interface{}) {
-		// TODO: Add and update zip metric on eviction https://gitlab.com/gitlab-org/gitlab-pages/-/issues/423
-		if archive, ok := object.(*zipArchive); archive != nil && ok {
-			archive.close()
+// 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)
+func (fs *zipVFS) findOrOpenArchive(ctx context.Context, path string) (*zipArchive, error) {
+	archive, expiry, found := fs.cache.GetWithExpiration(path)
+	if found {
+		if time.Until(expiry) < cacheRefreshInterval {
+			// refresh item
+			fs.cache.Set(path, archive, cache.DefaultExpiration)
 		}
-	})
+	} else {
+		archive = newArchive(path, DefaultOpenTimeout)
+
+		// if adding the archive to the cache fails it means it's already been added before
+		if fs.cache.Add(path, archive, cache.DefaultExpiration) != nil {
+			return nil, errAlreadyCached
+		}
+	}
+
+	zipDir, ok := archive.(*zipArchive)
+	if !ok {
+		// fail if the found archive in cache is not a zipArchive (just for type safety)
+		return nil, errNotZipArchive
+	}
 
-	return zipVFS
+	err := zipDir.openArchive(ctx)
+	return zipDir, err
 }