diff options
author | Vladimir Shushlin <vshushlin@gitlab.com> | 2022-07-15 12:39:41 +0300 |
---|---|---|
committer | Vladimir Shushlin <v.shushlin@gmail.com> | 2022-07-15 12:56:33 +0300 |
commit | 805858da777ee070d661be3a93171932bf06d951 (patch) | |
tree | 7553e1e08205d7b7d4eb3288ec459dd89c80d415 | |
parent | 8dd16c9c043d08617a1d01d46880d7506a299ba2 (diff) |
Revert: open files lazily when serving content
This reverts merge request
https://gitlab.com/gitlab-org/gitlab-pages/-/merge_requests/803
because it caused an incident when serving large compressed files
Changelog: fixed
-rw-r--r-- | internal/serving/disk/lazy.go | 80 | ||||
-rw-r--r-- | internal/serving/disk/reader.go | 26 | ||||
-rw-r--r-- | internal/serving/disk/zip/serving_test.go | 33 | ||||
-rw-r--r-- | internal/vfs/local/root.go | 4 | ||||
-rw-r--r-- | internal/vfs/root.go | 5 | ||||
-rw-r--r-- | internal/vfs/zip/archive.go | 19 |
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) |