diff options
author | Jaime Martinez <jmartinez@gitlab.com> | 2021-12-07 04:10:14 +0300 |
---|---|---|
committer | Jaime Martinez <jmartinez@gitlab.com> | 2021-12-08 04:21:29 +0300 |
commit | 34190011b2f774bc9a3613850a7be9d67c8ecd7e (patch) | |
tree | 21fda65154d004844dbd6ed2ba101bdef4b3a216 /internal | |
parent | d9c27aea72fb9ef1dcfe3c8fb4ecface8400e534 (diff) |
refactor: handle error outside Root caller
and pretend to serve content even if the request has been cancelled
to prevent making extra operations if this happens early in the
request chain. For example, if we know the request was cancelled
there is no point in checking the redirects or the custom
404 file handler.
Diffstat (limited to 'internal')
-rw-r--r-- | internal/serving/disk/reader.go | 48 | ||||
-rw-r--r-- | internal/vfs/zip/vfs.go | 18 |
2 files changed, 35 insertions, 31 deletions
diff --git a/internal/serving/disk/reader.go b/internal/serving/disk/reader.go index 258df3bc..2438daf7 100644 --- a/internal/serving/disk/reader.go +++ b/internal/serving/disk/reader.go @@ -16,6 +16,7 @@ import ( "gitlab.com/gitlab-org/labkit/errortracking" "gitlab.com/gitlab-org/gitlab-pages/internal/httperrors" + "gitlab.com/gitlab-org/gitlab-pages/internal/logging" "gitlab.com/gitlab-org/gitlab-pages/internal/redirects" "gitlab.com/gitlab-org/gitlab-pages/internal/serving" "gitlab.com/gitlab-org/gitlab-pages/internal/serving/disk/symlink" @@ -41,11 +42,8 @@ func (reader *Reader) serveRedirectsStatus(h serving.Handler, redirects *redirec func (reader *Reader) tryRedirects(h serving.Handler) bool { ctx := h.Request.Context() root, err := reader.vfs.Root(ctx, h.LookupPath.Path, h.LookupPath.SHA256) - if errors.Is(err, fs.ErrNotExist) { - return false - } else if err != nil { - httperrors.Serve500WithRequest(h.Writer, h.Request, "vfs.Root", err) - return true + if err != nil { + return handleRootError(err, h) } r := redirects.ParseRedirects(ctx, root) @@ -74,12 +72,8 @@ func (reader *Reader) tryFile(h serving.Handler) bool { ctx := h.Request.Context() root, err := reader.vfs.Root(ctx, h.LookupPath.Path, h.LookupPath.SHA256) - if errors.Is(err, fs.ErrNotExist) { - return false - } else if err != nil { - httperrors.Serve500WithRequest(h.Writer, h.Request, - "vfs.Root", err) - return true + if err != nil { + return handleRootError(err, h) } fullPath, err := reader.resolvePath(ctx, root, h.SubPath) @@ -137,11 +131,8 @@ func (reader *Reader) tryNotFound(h serving.Handler) bool { ctx := h.Request.Context() root, err := reader.vfs.Root(ctx, h.LookupPath.Path, h.LookupPath.SHA256) - if errors.Is(err, fs.ErrNotExist) { - return false - } else if err != nil { - httperrors.Serve500WithRequest(h.Writer, h.Request, "vfs.Root", err) - return true + if err != nil { + return handleRootError(err, h) } page404, err := reader.resolvePath(ctx, root, "404.html") @@ -153,6 +144,12 @@ func (reader *Reader) tryNotFound(h serving.Handler) bool { err = reader.serveCustomFile(ctx, h.Writer, h.Request, http.StatusNotFound, root, page404) if err != nil { + // Handle context.Canceled error as not exist https://gitlab.com/gitlab-org/gitlab-pages/-/issues/669 + if errors.Is(err, context.Canceled) { + logging.LogRequest(h.Request).WithError(err).Warn("user cancelled request") + return false + } + httperrors.Serve500WithRequest(h.Writer, h.Request, "serveCustomFile", err) return true } @@ -251,11 +248,6 @@ func (reader *Reader) serveCustomFile(ctx context.Context, w http.ResponseWriter // Open and serve content of file file, err := root.Open(ctx, fullPath) if err != nil { - // Handle context.Canceled error as not exist https://gitlab.com/gitlab-org/gitlab-pages/-/issues/669 - if errors.Is(err, context.Canceled) { - return fs.ErrNotExist - } - return err } defer file.Close() @@ -283,3 +275,17 @@ func (reader *Reader) serveCustomFile(ctx context.Context, w http.ResponseWriter return nil } + +func handleRootError(err error, h serving.Handler) bool { + switch err { + case fs.ErrNotExist: + return false + case context.Canceled: + // Handle context.Canceled error as not found exist https://gitlab.com/gitlab-org/gitlab-pages/-/issues/669 + //httperrors.Serve404(h.Writer) + return true + default: + httperrors.Serve500WithRequest(h.Writer, h.Request, "vfs.Root", err) + return true + } +} diff --git a/internal/vfs/zip/vfs.go b/internal/vfs/zip/vfs.go index cc0ff290..4ad1075f 100644 --- a/internal/vfs/zip/vfs.go +++ b/internal/vfs/zip/vfs.go @@ -179,16 +179,14 @@ func (zfs *zipVFS) Root(ctx context.Context, path string, cacheKey string) (vfs. // we do it in loop to not use any additional locks for { root, err := zfs.findOrOpenArchive(ctx, cacheKey, path) - switch err { - case context.Canceled: - // treat user-initiated cancellations as not found and - // do not report a 500 https://gitlab.com/gitlab-org/gitlab-pages/-/issues/669 - fallthrough - case httprange.ErrNotFound: - // If archive is not found, return a known `vfs` error - return nil, fs.ErrNotExist - case errAlreadyCached: - continue + if err != nil { + switch err { + case httprange.ErrNotFound: + // If archive is not found, return a known `vfs` error + return nil, fs.ErrNotExist + case errAlreadyCached: + continue + } } return root, err |