Merge branch 'revert-8dd16c9c' into 'master'

Revert "Merge branch 'perf/lazy-open-serve' into 'master'"

See merge request gitlab-org/gitlab-pages!816

6 files changed, 14 insertions, 153 deletions

diff --git a/internal/serving/disk/lazy.go b/internal/serving/disk/lazy.go
deleted file mode 100644
index 9b58457c..00000000
--- a/internal/serving/disk/lazy.go
+++ /dev/null
@@ -1,80 +0,0 @@
-package disk
-
-import (
-	"context"
-	"errors"
-	"fmt"
-	"io"
-
-	"gitlab.com/gitlab-org/gitlab-pages/internal/vfs"
-)
-
-var (
-	// Make sure lazyFile is a vfs.File to support vfs.ServeCompressedFile
-	// if the file is compressed.
-	// This should always be satisfied because root.Open always returns
-	// a vfs.File.
-	_ vfs.File = &lazyFile{}
-
-	// Make sure lazyFile is a ReadSeeker to support http.ServeContent
-	// if the file is not compressed.
-	// Note: lazyFile.Seek only works if the underlying root.Open returns
-	// a vfs.SeekableFile which is the case if the file is not compressed.
-	_ io.ReadSeeker = &lazyFile{}
-
-	// ErrInvalidSeeker is returned if lazyFile.Seek is called and the
-	// underlying file is not seekable
-	ErrInvalidSeeker = errors.New("file is not seekable")
-)
-
-type lazyFile struct {
-	f    vfs.File
-	err  error
-	load func() (vfs.File, error)
-}
-
-func lazyOpen(ctx context.Context, root vfs.Root, fullPath string) lazyFile {
-	lf := lazyFile{
-		load: func() (vfs.File, error) {
-			return root.Open(ctx, fullPath)
-		},
-	}
-
-	return lf
-}
-
-func (lf lazyFile) Read(p []byte) (int, error) {
-	if lf.f == nil && lf.err == nil {
-		lf.f, lf.err = lf.load()
-	}
-
-	if lf.err != nil {
-		return 0, lf.err
-	}
-
-	return lf.f.Read(p)
-}
-
-func (lf lazyFile) Close() error {
-	if lf.f != nil {
-		return lf.f.Close()
-	}
-
-	return nil
-}
-
-func (lf lazyFile) Seek(offset int64, whence int) (int64, error) {
-	if lf.f == nil && lf.err == nil {
-		lf.f, lf.err = lf.load()
-	}
-
-	if lf.err != nil {
-		return 0, lf.err
-	}
-
-	if sf, ok := lf.f.(io.ReadSeeker); ok {
-		return sf.Seek(offset, whence)
-	}
-
-	return 0, fmt.Errorf("unable to seek from %T: %w", lf.f, ErrInvalidSeeker)
-}
diff --git a/internal/serving/disk/reader.go b/internal/serving/disk/reader.go
index f804c92c..f250a0d6 100644
--- a/internal/serving/disk/reader.go
+++ b/internal/serving/disk/reader.go
@@ -199,6 +199,14 @@ func (reader *Reader) resolvePath(ctx context.Context, root vfs.Root, subPath ..
 func (reader *Reader) serveFile(ctx context.Context, w http.ResponseWriter, r *http.Request, root vfs.Root, origPath, sha string, accessControl bool) bool {
 	fullPath := reader.handleContentEncoding(ctx, w, r, root, origPath)
 
+	file, err := root.Open(ctx, fullPath)
+	if err != nil {
+		httperrors.Serve500WithRequest(w, r, "root.Open", err)
+		return true
+	}
+
+	defer file.Close()
+
 	fi, err := root.Lstat(ctx, fullPath)
 	if err != nil {
 		httperrors.Serve500WithRequest(w, r, "root.Lstat", err)
@@ -224,20 +232,12 @@ func (reader *Reader) serveFile(ctx context.Context, w http.ResponseWriter, r *h
 
 	reader.fileSizeMetric.WithLabelValues(reader.vfs.Name()).Observe(float64(fi.Size()))
 
-	compressed, err := root.IsCompressed(fullPath)
-	if err != nil {
-		httperrors.Serve500WithRequest(w, r, "root.IsCompressed", err)
-		return true
-	}
-
-	lf := lazyOpen(ctx, root, fullPath)
-	defer lf.Close()
-
-	if compressed {
-		w.Header().Set("Content-Length", strconv.FormatInt(fi.Size(), 10))
-		vfsServing.ServeCompressedFile(w, r, fi.ModTime(), lf)
+	// Support vfs.SeekableFile if available (uncompressed files)
+	if rs, ok := file.(vfs.SeekableFile); ok {
+		http.ServeContent(w, r, origPath, fi.ModTime(), rs)
 	} else {
-		http.ServeContent(w, r, origPath, fi.ModTime(), lf)
+		w.Header().Set("Content-Length", strconv.FormatInt(fi.Size(), 10))
+		vfsServing.ServeCompressedFile(w, r, fi.ModTime(), file)
 	}
 
 	return true
diff --git a/internal/serving/disk/zip/serving_test.go b/internal/serving/disk/zip/serving_test.go
index 88db51aa..c3b18f2b 100644
--- a/internal/serving/disk/zip/serving_test.go
+++ b/internal/serving/disk/zip/serving_test.go
@@ -11,13 +11,11 @@ import (
 	"testing"
 	"time"
 
-	"github.com/prometheus/client_golang/prometheus/testutil"
 	"github.com/stretchr/testify/require"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/config"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/serving"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers"
-	"gitlab.com/gitlab-org/gitlab-pages/metrics"
 )
 
 func TestZip_ServeFileHTTP(t *testing.T) {
@@ -35,32 +33,25 @@ func TestZip_ServeFileHTTP(t *testing.T) {
 		path           string
 		expectedStatus int
 		expectedBody   string
-		// Used to assert there are no zip requests if we don't
-		// need to open or read from the file, making sure the
-		// lazyFile is working as intended.
-		expectedOpen bool
-		extraHeaders http.Header
+		extraHeaders   http.Header
 	}{
 		"accessing /index.html": {
 			vfsPath:        httpURL,
 			path:           "/index.html",
 			expectedStatus: http.StatusOK,
 			expectedBody:   "zip.gitlab.io/project/index.html\n",
-			expectedOpen:   true,
 		},
 		"accessing /index.html from disk": {
 			vfsPath:        fileURL,
 			path:           "/index.html",
 			expectedStatus: http.StatusOK,
 			expectedBody:   "zip.gitlab.io/project/index.html\n",
-			expectedOpen:   true,
 		},
 		"accessing /": {
 			vfsPath:        httpURL,
 			path:           "/",
 			expectedStatus: http.StatusOK,
 			expectedBody:   "zip.gitlab.io/project/index.html\n",
-			expectedOpen:   true,
 		},
 		"accessing / If-Modified-Since": {
 			vfsPath:        httpURL,
@@ -75,7 +66,6 @@ func TestZip_ServeFileHTTP(t *testing.T) {
 			path:           "/",
 			expectedStatus: http.StatusOK,
 			expectedBody:   "zip.gitlab.io/project/index.html\n",
-			expectedOpen:   true,
 			extraHeaders: http.Header{
 				"If-Modified-Since": {time.Now().AddDate(-10, 0, 0).Format(http.TimeFormat)},
 			},
@@ -92,7 +82,6 @@ func TestZip_ServeFileHTTP(t *testing.T) {
 			vfsPath:        httpURL,
 			path:           "/",
 			expectedStatus: http.StatusOK,
-			expectedOpen:   true,
 			expectedBody:   "zip.gitlab.io/project/index.html\n",
 			extraHeaders: http.Header{
 				"If-Unmodified-Since": {time.Now().Format(http.TimeFormat)},
@@ -102,7 +91,6 @@ func TestZip_ServeFileHTTP(t *testing.T) {
 			vfsPath:        fileURL,
 			path:           "/",
 			expectedStatus: http.StatusOK,
-			expectedOpen:   true,
 			expectedBody:   "zip.gitlab.io/project/index.html\n",
 		},
 		"accessing without /": {
@@ -145,7 +133,6 @@ func TestZip_ServeFileHTTP(t *testing.T) {
 			vfsPath:        httpURL,
 			path:           "/",
 			expectedStatus: http.StatusOK,
-			expectedOpen:   true,
 			expectedBody:   "zip.gitlab.io/project/index.html\n",
 			extraHeaders: http.Header{
 				"If-None-Match": {fmt.Sprintf("%q", "badetag")},
@@ -155,7 +142,6 @@ func TestZip_ServeFileHTTP(t *testing.T) {
 			vfsPath:        httpURL,
 			path:           "/",
 			expectedStatus: http.StatusOK,
-			expectedOpen:   true,
 			expectedBody:   "zip.gitlab.io/project/index.html\n",
 			extraHeaders: http.Header{
 				"If-Match": {fmt.Sprintf("%q", sha(httpURL))},
@@ -203,8 +189,6 @@ func TestZip_ServeFileHTTP(t *testing.T) {
 				r.Header = test.extraHeaders
 			}
 
-			zipRequestsCountBefore := zipCacheRequests()
-
 			handler := serving.Handler{
 				Writer:  w,
 				Request: r,
@@ -239,25 +223,10 @@ func TestZip_ServeFileHTTP(t *testing.T) {
 			if test.expectedStatus != http.StatusInternalServerError {
 				require.Equal(t, test.expectedBody, string(body))
 			}
-
-			zipRequestsCountAfter := zipCacheRequests()
-
-			if !test.expectedOpen {
-				require.Equal(t, zipRequestsCountBefore, zipRequestsCountAfter)
-			} else {
-				require.NotEqual(t, zipRequestsCountBefore, zipRequestsCountAfter)
-			}
 		})
 	}
 }
 
-func zipCacheRequests() float64 {
-	zipMetricHit := metrics.ZipCacheRequests.WithLabelValues("data-offset", "hit")
-	zipMetricMiss := metrics.ZipCacheRequests.WithLabelValues("data-offset", "miss")
-
-	return testutil.ToFloat64(zipMetricHit) + testutil.ToFloat64(zipMetricMiss)
-}
-
 func sha(path string) string {
 	sha := sha256.Sum256([]byte(path))
 	s := hex.EncodeToString(sha[:])
diff --git a/internal/vfs/local/root.go b/internal/vfs/local/root.go
index d15fa4e1..9cd67a9f 100644
--- a/internal/vfs/local/root.go
+++ b/internal/vfs/local/root.go
@@ -105,7 +105,3 @@ func (r *Root) Open(ctx context.Context, name string) (vfs.File, error) {
 
 	return file, nil
 }
-
-func (r *Root) IsCompressed(_ string) (bool, error) {
-	return false, nil
-}
diff --git a/internal/vfs/root.go b/internal/vfs/root.go
index 1a2e9ccd..9c7528b3 100644
--- a/internal/vfs/root.go
+++ b/internal/vfs/root.go
@@ -16,7 +16,6 @@ type Root interface {
 	Lstat(ctx context.Context, name string) (os.FileInfo, error)
 	Readlink(ctx context.Context, name string) (string, error)
 	Open(ctx context.Context, name string) (File, error)
-	IsCompressed(name string) (bool, error)
 }
 
 type instrumentedRoot struct {
@@ -69,7 +68,3 @@ func (i *instrumentedRoot) Open(ctx context.Context, name string) (File, error)
 
 	return f, err
 }
-
-func (i *instrumentedRoot) IsCompressed(name string) (bool, error) {
-	return i.root.IsCompressed(name)
-}
diff --git a/internal/vfs/zip/archive.go b/internal/vfs/zip/archive.go
index 23ad7b2e..2b04310d 100644
--- a/internal/vfs/zip/archive.go
+++ b/internal/vfs/zip/archive.go
@@ -239,25 +239,6 @@ func (a *zipArchive) Open(ctx context.Context, name string) (vfs.File, error) {
 	}
 }
 
-// IsCompressed finds the file by name and checks its compression method. If the method is
-// zip.Store it means it's not compressed.
-// Returns errNotFile for directories or non-regular files.
-func (a *zipArchive) IsCompressed(name string) (bool, error) {
-	file := a.findFile(name)
-	if file == nil {
-		if a.findDirectory(name) != nil {
-			return false, errNotFile
-		}
-		return false, os.ErrNotExist
-	}
-
-	if !file.Mode().IsRegular() {
-		return false, errNotFile
-	}
-
-	return file.Method != zip.Store, nil
-}
-
 // Lstat finds the file by name inside the zipArchive and returns its FileInfo
 func (a *zipArchive) Lstat(ctx context.Context, name string) (os.FileInfo, error) {
 	file := a.findFile(name)