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

Avatar caching doesn't work on Firefox #16964

Closed
6 tasks
seadra opened this issue Sep 5, 2021 · 17 comments
Closed
6 tasks

Avatar caching doesn't work on Firefox #16964

seadra opened this issue Sep 5, 2021 · 17 comments

Comments

@seadra
Copy link

seadra commented Sep 5, 2021

  • Gitea version (or commit ref): 1.15.2
  • Git version: 2.20.1
  • Operating system: Debian
  • Database (use [x]):
    • PostgreSQL
    • MySQL
    • MSSQL
    • SQLite
  • Can you reproduce the bug at https://try.gitea.io:
    • Yes
    • No
  • Log gist:

Description

Avatar images aren't cached with Firefox, a new copy is always fetched on load.

On Chromium, it shows that avatar images are cached (in memory).

Screenshots

On Firefox, after a refresh:

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Sep 6, 2021

What you saw in Firefox network log:

  1. Firefox tried to get an avatar from try.gitea.io
  2. try.gitea.io made a response 302 to redirect to gravatar.com (a common avatar service), because these avatars are from gravatar.
  3. Firefox made a request to gravatar.com with local cache information
  4. gravatar.com made a response 304 (Not Modified) to tell Firefox use local cache.

It seems there is no problem. If you believe you found a bug, just provide the expected behavior you thought it should be.

@lunny
Copy link
Member

lunny commented Sep 6, 2021

Have you opened the option of firefox to disable all cache?

@silverwind
Copy link
Member

silverwind commented Sep 6, 2021

The 302 from gitea is uncachable, we might be able to make it cachable by using 308 or 301, just need to ensure that browsers will eventually invalidate that cache.

image

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Sep 6, 2021

307 is very similar to 302 (except how to handle GET/POST methods), they all have cache control: https://stackoverflow.com/questions/12212839/how-long-is-a-302-redirect-saved-in-browser : It shouldn't be cached at all unless there's also a Cache-Control or Expires header returned by the web server. According to RFC 2616, section 10.3.3 302 Found.

And I do not think 301/308 is a correct way: https://stackoverflow.com/questions/9130422/how-long-do-browsers-cache-http-301s . The redirection may change: https://github.com/go-gitea/gitea/blob/main/routers/web/user/avatar.go#L76

From the screenshots that the author provided, I do not think there is a serious problem.

@silverwind
Copy link
Member

silverwind commented Sep 6, 2021

I meant 308, basically any "permanent" redirect should be cachable. Edited my comment above.

I think browser should honor caching headers fine on such redirects, so they should eventually invalidate.

The browsers still honor the Cache-Control and Expires headers like with any other response, if they are specified.

@wxiaoguang
Copy link
Contributor

What's the difference between 301/308 and 302/307 if they both have cache control?

@silverwind
Copy link
Member

There's probably no difference, browsers should cache all status codes as long as cache-control headers are in place. We do not set any so the current 302 with no cache-control headers is uncachable. It might have been if it were a 301/308. I think we just need to set cache headers on redirect responses like we already do on self-hosted avatars.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Sep 6, 2021

From the above information and my test (on try.gitea.io and my private server), I do not think this is a gitea problem.

Without the cache control header in 302 response, the response is still very quick. It can not cause the problem "20 seconds lag" described by the deleted comment. Even if we add the cache control, this author's problem can't be resolved.

And the author just deleted the comment containing screenshots of his private server, you can refer that in email inbox (maybe some wrong configuration on the private server?)

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Sep 6, 2021

There's probably no difference, browsers should cache all status codes as long as cache-control headers are in place. We do not set any so the current 302 with no cache-control headers is uncachable. It might have been if it were a 301/308. I think we just need to set cache headers on redirect responses like we already do on self-hosted avatars.

ps: if we add a cache control header in avatar's 302 response to reduce browser requests and reduce gitea server/database load, it would be helpful in other ways. LOL.

@silverwind
Copy link
Member

silverwind commented Sep 6, 2021

Yes, I think it's good practice to always set cache-control. We can use time-based cache for avatar requests, e.g.

func HandleTimeCache(req *http.Request, w http.ResponseWriter, fi os.FileInfo) (handled bool) {

silverwind added a commit to silverwind/gitea that referenced this issue Sep 6, 2021
This does seem to do the trick to make the Avatar redirects cachable
in Chrome.

In Firefox, it does not seem to work, thought and I found no way to
suppress the requests to the original URLs, I even tried setting an
Etag to no avail.

Related discussion in go-gitea#16964.
@silverwind
Copy link
Member

silverwind commented Sep 6, 2021

Caching of Avatar requests certainly works in Firefox. What does not work is the caching of the redirection response in case the Avatar is hosted externally. I did add cache headers to those redirects in #16973 but Firefox just seems to ignore them. I think this is the best we can do, unless someone has another idea.

6543 pushed a commit that referenced this issue Sep 6, 2021
* Add Cache-Control to avatar redirects

This does seem to do the trick to make the Avatar redirects cachable
in Chrome.

In Firefox, it does not seem to work, thought and I found no way to
suppress the requests to the original URLs, I even tried setting an
Etag to no avail.

Related discussion in #16964.

Co-authored-by: zeripath <art27@cantab.net>
@6543
Copy link
Member

6543 commented Sep 6, 2021

#16973 got merged 🚀

@silverwind
Copy link
Member

Can't figure out why Firefox would not cache those redirects, maybe it has something to do with their privacy protection, I'm not sure. Couldn't find anything on Bugzilla either.

It might be possible to directly render gravatar links without going through a redirect, but I think that method may not be possible for other federated avatar sources.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Sep 7, 2021

If the cache of avatars responded by Gitea doesn't work (described in the original issue by author, on the private server), maybe it is caused by RUN_MODE != 'prod'

if !setting.IsProd() {

	if !setting.IsProd() {
		return "no-store"
	}

@silverwind
Copy link
Member

No, I am running RUN_MODE = prod in my development setup to to specifically allow cache testing.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Sep 7, 2021

(I mean the original problem described in this issue by the author. And the fix and the original problem seems off-topic now .... )

@silverwind
Copy link
Member

Yeah I think we can close this. OP has deleted any helpful information and is probably not going to respond any more.

@go-gitea go-gitea locked and limited conversation to collaborators Oct 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants