Merge branch 'perf/lazy-open-serve' into 'master'revert-perf-lazy-open-master

perf: open files lazily when serving content

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

6 files changed, 153 insertions, 14 deletions

diff --git a/internal/serving/disk/lazy.go b/internal/serving/disk/lazy.go
new file mode 100644
index 00000000..9b58457c
--- /dev/null
+++ b/internal/serving/disk/lazy.go
@@ -0,0 +1,80 @@
+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 f250a0d6..f804c92c 100644
--- a/internal/serving/disk/reader.go
+++ b/internal/serving/disk/reader.go
@@ -199,14 +199,6 @@ 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)
@@ -232,12 +224,20 @@ func (reader *Reader) serveFile(ctx context.Context, w http.ResponseWriter, r *h
 
 	reader.fileSizeMetric.WithLabelValues(reader.vfs.Name()).Observe(float64(fi.Size()))
 
-	// Support vfs.SeekableFile if available (uncompressed files)
-	if rs, ok := file.(vfs.SeekableFile); ok {
-		http.ServeContent(w, r, origPath, fi.ModTime(), rs)
-	} else {
+	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(), file)
+		vfsServing.ServeCompressedFile(w, r, fi.ModTime(), lf)
+	} else {
+		http.ServeContent(w, r, origPath, fi.ModTime(), lf)
 	}
 
 	return true
diff --git a/internal/serving/disk/zip/serving_test.go b/internal/serving/disk/zip/serving_test.go
index c3b18f2b..88db51aa 100644
--- a/internal/serving/disk/zip/serving_test.go
+++ b/internal/serving/disk/zip/serving_test.go
@@ -11,11 +11,13 @@ 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) {
@@ -33,25 +35,32 @@ func TestZip_ServeFileHTTP(t *testing.T) {
 		path           string
 		expectedStatus int
 		expectedBody   string
-		extraHeaders   http.Header
+		// 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
 	}{
 		"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,
@@ -66,6 +75,7 @@ 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)},
 			},
@@ -82,6 +92,7 @@ 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)},
@@ -91,6 +102,7 @@ func TestZip_ServeFileHTTP(t *testing.T) {
 			vfsPath:        fileURL,
 			path:           "/",
 			expectedStatus: http.StatusOK,
+			expectedOpen:   true,
 			expectedBody:   "zip.gitlab.io/project/index.html\n",
 		},
 		"accessing without /": {
@@ -133,6 +145,7 @@ 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")},
@@ -142,6 +155,7 @@ 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))},
@@ -189,6 +203,8 @@ func TestZip_ServeFileHTTP(t *testing.T) {
 				r.Header = test.extraHeaders
 			}
 
+			zipRequestsCountBefore := zipCacheRequests()
+
 			handler := serving.Handler{
 				Writer:  w,
 				Request: r,
@@ -223,10 +239,25 @@ 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 9cd67a9f..d15fa4e1 100644
--- a/internal/vfs/local/root.go
+++ b/internal/vfs/local/root.go
@@ -105,3 +105,7 @@ 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 9c7528b3..1a2e9ccd 100644
--- a/internal/vfs/root.go
+++ b/internal/vfs/root.go
@@ -16,6 +16,7 @@ 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 {
@@ -68,3 +69,7 @@ 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 2b04310d..23ad7b2e 100644
--- a/internal/vfs/zip/archive.go
+++ b/internal/vfs/zip/archive.go
@@ -239,6 +239,25 @@ 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)