594962 - Send and handle caching headers for show_bug.cgi

Status: RESOLVED DUPLICATE of bug 156865

(Bugzilla :: Bugzilla-General, enhancement, P4)

Description

[Max Kanat-Alexander]

People often visit the same bug many times when it hasn't been changed.

Although we can't cache security bugs, and we shouldn't cache bugs in requirelogin installations, we can still cache bugs in most other situations.

I believe what we need to do is:

1) Send a Last-Modified header along with show_bug.
2) Understand If-Modified-Since and return a 304 Not Modified as appropriate.
3) Set Cache-Control to public for the appropriate bugs, to make sure that they are cached even on https.

We may instead need to use the ETag header, though, to support multiple Bugzilla users on the same machine using the same browser.

Comment 1

[Max Kanat-Alexander]

Attachment: etag-showbug.diff

Okay, I've tested this locally and my browser properly sends If-None-Match and Bugzilla correctly returns a 304. This should especially help with visiting very large bugs that you've already seen and that haven't changed since last time.

Comment 2

[:glob ✱]

wow, this look very interesting!

with the current code however, it looks like if someone changes their real name (which often happens on bmo to indicate they are unavailable), this won't force a cache miss.

likewise if the template itself changes, this won't result in a different etag.


are these things we should worry about?

Comment 3

[Max Kanat-Alexander]

(In reply to comment #2)
> are these things we should worry about?

  Mmm, probably not. Perhaps we should also hash in BUGZILLA_VERSION though, as a basic stopgap against retaining cached pages against upgrades.

Comment 4

[Max Kanat-Alexander]

Attachment: v2

Comment 5

[Max Kanat-Alexander]

For reference, I don't want to first generate all the output and then digest the output for the etag--generating the show_bug.cgi output actually takes a long time (too long, right now--needs to be fixed) and so not generating it saves us significant time.

Comment 6

[:glob ✱]

the etag needs to change when the current user changes their details, so this depends on bug 600118.


do you think it's worth adding a "configuration last-modified" to the db, which is touched when anything relating to the "config" is changed?  (parameters, default preferences, classifications, products, flags, custom fields, field values, workflow, groups and keywords).  if this field exists, it would catch a lot of edge cases which should trigger an immediate invalidation of the etag.

Comment 7

[Frédéric Buclin]

Comment on attachment 474292 [details] [diff] [review]
v2

Unless I miss something, this patch doesn't take into account admin actions, such as editing flags, or changing mandatory groups (which do not alter bugs.delta_ts, IIRC). Also, if you don't want to cache the page when the bug is restricted to some group, why is it useful to do all this stuff and set ETag?

Comment 8

[Max Kanat-Alexander]

(In reply to comment #7)
> Comment on attachment 474292 [details] [diff] [review]
> v2
> 
> Unless I miss something, this patch doesn't take into account admin actions,
> such as editing flags, or changing mandatory groups (which do not alter
> bugs.delta_ts, IIRC).

  That's true.

> Also, if you don't want to cache the page when the bug is
> restricted to some group, why is it useful to do all this stuff and set ETag?

  Because in most installations, most bugs are not restricted to any group. This increases the speed of using Bugzilla a remarkable amount.

Comment 9

[:glob ✱]

> Unless I miss something, this patch doesn't take into account admin actions,
> such as editing flags, or changing mandatory groups (which do not alter
> bugs.delta_ts, IIRC)

yeah, this is why i suggested adding a "configuration last-modified" in comment 6 -- any admin action would invalidate caches.

> if you don't want to cache the page when the bug is restricted to some group,
> why is it useful to do all this stuff and set ETag?

that raises the question why //shouldn't// an e-tag apply to group restricted bugs?  if the e-tag mechanism is sound, then it shouldn't matter.

Comment 10

[Max Kanat-Alexander]

(In reply to comment #9)
> that raises the question why //shouldn't// an e-tag apply to group restricted
> bugs?  if the e-tag mechanism is sound, then it shouldn't matter.

  Because they shouldn't be cached on disk.

Comment 11

[Max Kanat-Alexander]

I suppose I could try just doing an md5sum of the whole show_bug output before it's sent and see if that still actually results in a performance speedup.

Comment 12

[Max Kanat-Alexander]

Ah, due to issue_hash_token, md5-summing the full output doesn't work either--the etag is always different.

Comment 13

[Max Kanat-Alexander]

Comment on attachment 474292 [details] [diff] [review]
v2

Okay, so as far as I can see, this isn't going to be reliably possible. There are too many possible variables to track. 

In addition:

* The variables that need tracking could be highly different across installations.
* It would be a very easy thing for a customizer or extension author to mess up or forget to account for--and then Bugzilla would display the wrong content.
* It would be too easy for *us* to forget to account for one of the numerous variables in the future and mess this up and not even notice until somebody inexplicably was experiencing bad content and we had to do enormous amounts of debugging to track it down.

Comment 14

[Max Kanat-Alexander]

Maybe at some point in the future this will become easier, so I'm going to leave this bug open.

Comment 15

[Frédéric Buclin]

This is a dupe of bug 156865. Feel free to either copy the dependency chain to the other bug, or mark the other bug as a dupe of this one (though we usually mark the newer bug as a dupe of the older one, unless the newer bug brings more data than the older one).