From a6e9510c078758794da5f6a1983b595bfc415de9 Mon Sep 17 00:00:00 2001 From: Hoernschen Date: Mon, 26 Feb 2024 22:21:42 +0000 Subject: [PATCH] FIX blank internal pages (#164) (#292) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hello 👋 since it affected my deployment of the pages server I started to look into the problem of the blank pages and think I found a solution for it: 1. There is no check if the file response is empty, neither in cache retrieval nor in writing of a cache. Also the provided method for checking for empty responses had a bug. 2. I identified the redirect response to be the issue here. There is a cache write with the full cache key (e. g. rawContent/user/repo|branch|route/index.html) happening in the handling of the redirect response. But the written body here is empty. In the triggered request from the redirect response the server then finds a cache item to the key and serves the empty body. A quick fix is the check for empty file responses mentioned in 1. 3. The decision to redirect the user comes quite far down in the upstream function. Before that happens a lot of stuff that may not be important since after the redirect response comes a new request anyway. Also, I suspect that this causes the caching problem because there is a request to the forge server and its error handling with some recursions happening before. I propose to move two of the redirects before "Preparing" 4. The recursion in the upstream function makes it difficult to understand what is actually happening. I added some more logging to have an easier time with that. 5. I changed the default behaviour to append a trailing slash to the path to true. In my tested scenarios it happened anyway. This way there is no recursion happening before the redirect. I am not developing in go frequently and rarely contribute to open source -> so feedback of all kind is appreciated closes #164 Reviewed-on: https://codeberg.org/Codeberg/pages-server/pulls/292 Reviewed-by: 6543 <6543@obermui.de> Reviewed-by: crapStone Co-authored-by: Hoernschen Co-committed-by: Hoernschen --- server/gitea/cache.go | 25 +++++++++++++++++-------- server/gitea/client.go | 19 +++++++++++-------- server/handler/handler.go | 1 + server/upstream/upstream.go | 12 ++++++++++++ 4 files changed, 41 insertions(+), 16 deletions(-) diff --git a/server/gitea/cache.go b/server/gitea/cache.go index 267c3d8..4ecc5e0 100644 --- a/server/gitea/cache.go +++ b/server/gitea/cache.go @@ -39,7 +39,7 @@ type FileResponse struct { } func (f FileResponse) IsEmpty() bool { - return len(f.Body) != 0 + return len(f.Body) == 0 } func (f FileResponse) createHttpResponse(cacheKey string) (header http.Header, statusCode int) { @@ -72,13 +72,14 @@ type BranchTimestamp struct { type writeCacheReader struct { originalReader io.ReadCloser buffer *bytes.Buffer - rileResponse *FileResponse + fileResponse *FileResponse cacheKey string cache cache.ICache hasError bool } func (t *writeCacheReader) Read(p []byte) (n int, err error) { + log.Trace().Msgf("[cache] read %q", t.cacheKey) n, err = t.originalReader.Read(p) if err != nil && err != io.EOF { log.Trace().Err(err).Msgf("[cache] original reader for %q has returned an error", t.cacheKey) @@ -90,12 +91,20 @@ func (t *writeCacheReader) Read(p []byte) (n int, err error) { } func (t *writeCacheReader) Close() error { - if !t.hasError { - fc := *t.rileResponse - fc.Body = t.buffer.Bytes() - _ = t.cache.Set(t.cacheKey, fc, fileCacheTimeout) + doWrite := !t.hasError + fc := *t.fileResponse + fc.Body = t.buffer.Bytes() + if fc.IsEmpty() { + log.Trace().Msg("[cache] file response is empty") + doWrite = false } - log.Trace().Msgf("cacheReader for %q saved=%t closed", t.cacheKey, !t.hasError) + if doWrite { + err := t.cache.Set(t.cacheKey, fc, fileCacheTimeout) + if err != nil { + log.Trace().Err(err).Msgf("[cache] writer for %q has returned an error", t.cacheKey) + } + } + log.Trace().Msgf("cacheReader for %q saved=%t closed", t.cacheKey, doWrite) return t.originalReader.Close() } @@ -108,7 +117,7 @@ func (f FileResponse) CreateCacheReader(r io.ReadCloser, cache cache.ICache, cac return &writeCacheReader{ originalReader: r, buffer: bytes.NewBuffer(make([]byte, 0)), - rileResponse: &f, + fileResponse: &f, cache: cache, cacheKey: cacheKey, } diff --git a/server/gitea/client.go b/server/gitea/client.go index 42cf065..4f7eaa6 100644 --- a/server/gitea/client.go +++ b/server/gitea/client.go @@ -112,26 +112,27 @@ func (client *Client) GiteaRawContent(targetOwner, targetRepo, ref, resource str func (client *Client) ServeRawContent(targetOwner, targetRepo, ref, resource string) (io.ReadCloser, http.Header, int, error) { cacheKey := fmt.Sprintf("%s/%s/%s|%s|%s", rawContentCacheKeyPrefix, targetOwner, targetRepo, ref, resource) log := log.With().Str("cache_key", cacheKey).Logger() - + log.Trace().Msg("try file in cache") // handle if cache entry exist if cache, ok := client.responseCache.Get(cacheKey); ok { cache := cache.(FileResponse) cachedHeader, cachedStatusCode := cache.createHttpResponse(cacheKey) // TODO: check against some timestamp mismatch?!? if cache.Exists { + log.Debug().Msg("[cache] exists") if cache.IsSymlink { linkDest := string(cache.Body) log.Debug().Msgf("[cache] follow symlink from %q to %q", resource, linkDest) return client.ServeRawContent(targetOwner, targetRepo, ref, linkDest) - } else { - log.Debug().Msg("[cache] return bytes") + } else if !cache.IsEmpty() { + log.Debug().Msgf("[cache] return %d bytes", len(cache.Body)) return io.NopCloser(bytes.NewReader(cache.Body)), cachedHeader, cachedStatusCode, nil + } else if cache.IsEmpty() { + log.Debug().Msg("[cache] is empty") } - } else { - return nil, cachedHeader, cachedStatusCode, ErrorNotFound } } - + log.Trace().Msg("file not in cache") // not in cache, open reader via gitea api reader, resp, err := client.sdkClient.GetFileReader(targetOwner, targetRepo, ref, resource, client.supportLFS) if resp != nil { @@ -155,12 +156,14 @@ func (client *Client) ServeRawContent(targetOwner, targetRepo, ref, resource str linkDest = path.Join(path.Dir(resource), linkDest) // we store symlink not content to reduce duplicates in cache - if err := client.responseCache.Set(cacheKey, FileResponse{ + fileResponse := FileResponse{ Exists: true, IsSymlink: true, Body: []byte(linkDest), ETag: resp.Header.Get(ETagHeader), - }, fileCacheTimeout); err != nil { + } + log.Trace().Msgf("file response has %d bytes", len(fileResponse.Body)) + if err := client.responseCache.Set(cacheKey, fileResponse, fileCacheTimeout); err != nil { log.Error().Err(err).Msg("[cache] error on cache write") } diff --git a/server/handler/handler.go b/server/handler/handler.go index 96788e4..ffc3400 100644 --- a/server/handler/handler.go +++ b/server/handler/handler.go @@ -26,6 +26,7 @@ func Handler( dnsLookupCache, canonicalDomainCache, redirectsCache cache.ICache, ) http.HandlerFunc { return func(w http.ResponseWriter, req *http.Request) { + log.Debug().Msg("\n----------------------------------------------------------") log := log.With().Strs("Handler", []string{req.Host, req.RequestURI}).Logger() ctx := context.New(w, req) diff --git a/server/upstream/upstream.go b/server/upstream/upstream.go index 07b6ad2..d9c131e 100644 --- a/server/upstream/upstream.go +++ b/server/upstream/upstream.go @@ -56,6 +56,8 @@ type Options struct { func (o *Options) Upstream(ctx *context.Context, giteaClient *gitea.Client, redirectsCache cache.ICache) bool { log := log.With().Strs("upstream", []string{o.TargetOwner, o.TargetRepo, o.TargetBranch, o.TargetPath}).Logger() + log.Debug().Msg("Start") + if o.TargetOwner == "" || o.TargetRepo == "" { html.ReturnErrorPage(ctx, "forge client: either repo owner or name info is missing", http.StatusBadRequest) return true @@ -104,13 +106,16 @@ func (o *Options) Upstream(ctx *context.Context, giteaClient *gitea.Client, redi // Handle not found error if err != nil && errors.Is(err, gitea.ErrorNotFound) { + log.Debug().Msg("Handling not found error") // Get and match redirects redirects := o.getRedirects(giteaClient, redirectsCache) if o.matchRedirects(ctx, giteaClient, redirects, redirectsCache) { + log.Trace().Msg("redirect") return true } if o.TryIndexPages { + log.Trace().Msg("try index page") // copy the o struct & try if an index page exists optionsForIndexPages := *o optionsForIndexPages.TryIndexPages = false @@ -121,6 +126,7 @@ func (o *Options) Upstream(ctx *context.Context, giteaClient *gitea.Client, redi return true } } + log.Trace().Msg("try html file with path name") // compatibility fix for GitHub Pages (/example → /example.html) optionsForIndexPages.appendTrailingSlash = false optionsForIndexPages.redirectIfExists = strings.TrimSuffix(ctx.Path(), "/") + ".html" @@ -130,8 +136,11 @@ func (o *Options) Upstream(ctx *context.Context, giteaClient *gitea.Client, redi } } + log.Trace().Msg("not found") + ctx.StatusCode = http.StatusNotFound if o.TryIndexPages { + log.Trace().Msg("try not found page") // copy the o struct & try if a not found page exists optionsForNotFoundPages := *o optionsForNotFoundPages.TryIndexPages = false @@ -142,6 +151,7 @@ func (o *Options) Upstream(ctx *context.Context, giteaClient *gitea.Client, redi return true } } + log.Trace().Msg("not found page missing") } return false @@ -173,10 +183,12 @@ func (o *Options) Upstream(ctx *context.Context, giteaClient *gitea.Client, redi // Append trailing slash if missing (for index files), and redirect to fix filenames in general // o.appendTrailingSlash is only true when looking for index pages if o.appendTrailingSlash && !strings.HasSuffix(ctx.Path(), "/") { + log.Trace().Msg("append trailing slash and redirect") ctx.Redirect(ctx.Path()+"/", http.StatusTemporaryRedirect) return true } if strings.HasSuffix(ctx.Path(), "/index.html") && !o.ServeRaw { + log.Trace().Msg("remove index.html from path and redirect") ctx.Redirect(strings.TrimSuffix(ctx.Path(), "index.html"), http.StatusTemporaryRedirect) return true }