Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTTP cache rework and enable caching for storage assets #13569

Merged
merged 1 commit into from
Nov 17, 2020

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Nov 14, 2020

This enabled HTTP time-based cache for storage assets, primarily avatars. I have not observed If-Modified-Since from browsers during tests but I guess it's good to support regardless.

It introduces a new generic httpcache module that can handle both time-based and etag-based caching.

Additionally, manifest.json and robots.txt are now also cachable.

@lunny lunny added type/enhancement An improvement of existing functionality performance/speed performance issues with slow downs labels Nov 15, 2020
@silverwind
Copy link
Member Author

silverwind commented Nov 15, 2020

Working on generalizing http cache handling a bit in a separate module.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 15, 2020
@silverwind silverwind changed the title Enable HTTP cache for storage assets HTTP cache rework and enable caching for storage assets Nov 15, 2020
@silverwind
Copy link
Member Author

silverwind commented Nov 15, 2020

httpcache module added. manifest.json and robots.txt are now also cachable. The caching is now also disabled when RUN_MODE is dev to ease development.

The only things that remain uncachable are the avatar redirects, we should get rid of them later.

@lafriks lafriks added this to the 1.14.0 milestone Nov 15, 2020
@lunny
Copy link
Member

lunny commented Nov 15, 2020

httpcache module added. manifest.json and robots.txt are now also cacheable. The caching is now also disabled when RUN_MODE is dev to ease development.

The only things that remain uncacheable are the avatar redirects, we should get rid of them later.

Why do you think we should get rid of server redirect? If the assets are stored on a s3 compatible service, it's useful to reduce the bandwidth of the server.

@silverwind
Copy link
Member Author

silverwind commented Nov 16, 2020

Why do you think we should get rid of server redirect?

It seems like a useless roundtrip that hurts performance. I don't see why it couldn't just directly output the correct URLs, especially for local storage.

302/307 redirects are by definition uncachable and must be attempted every page load. We can't use cachable permanent 301/308 redirects either because that would make avatars essentially immutable. I think it'd be best drop it and emit direct links.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Nov 17, 2020
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Nov 17, 2020
This enabled HTTP time-based cache for storage assets, primarily
avatars. I have not observed If-Modified-Since from browsers during
tests but I guess it's good to support regardless.

It introduces a new generic httpcache module that can handle both
time-based and etag-based caching.

Additionally, manifest.json and robots.txt are now also cachable.
@techknowlogick
Copy link
Member

🚀

@techknowlogick techknowlogick merged commit 0615b66 into go-gitea:master Nov 17, 2020
@silverwind silverwind deleted the avas branch November 17, 2020 23:13
@go-gitea go-gitea locked and limited conversation to collaborators Jan 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. performance/speed performance issues with slow downs type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants