Moodle
  1. Moodle
  2. MDL-28709

Performance: Send cache/contenttype headers with 304 responses

    Details

    • Rank:
      18500

      Description

      Apache is sending a max-age header of 1 second for image.php etc requests. This seems to be bug/confusion/bad configuration in apache that causes incorrect application of mod_expires rules before the content type is determined. (It does send the correct content-type to the browser, so there is some race here...)

      For us, this was trivially reproducible by loading any moodle page, then pressing F5 in the browser, which sends if-modified-since requests. The server then responds with a cache-control: max-age=1 header, triggering the browser to revalidate that resource on every future page load for at least the session.

      This can be worked around by sending the content-type and cache headers from moodle for 304 Not Modified responses, which also helps buggy browsers and caches to retain the content for longer since they may forget or drop old headers when a new request for that resource happens.

      The max-age and expires headers are made consistent, since according to the http spec, max-age will always override expires the difference seemed to be giving 10x too long expiry.

      github for 20_STABLE incoming...

        Issue Links

          Activity

          Show
          Tony Levi added a comment - https://github.com/tlevi/moodle/tree/mdl28709_20STABLE
          Hide
          Michael de Raadt added a comment -

          Thanks for suggesting this.

          Show
          Michael de Raadt added a comment - Thanks for suggesting this.
          Hide
          Tony Levi added a comment -

          Also affects 2.2/dev

          Show
          Tony Levi added a comment - Also affects 2.2/dev
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          Today is my last day in the office and I won't be back online for several days as such I'm assigning this to Petr to look at.

          Tony, thanks for the work on this. I've had a quick look over the patch now, there are a couple of minor things that pop out regarding our coding guidelines however all in all the patch definitely looks like a good improvement!

          Petr, I'll leave this to you being the expert here. I haven't delved down into this myself however there certainly seems to be an easy performance win here.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, Today is my last day in the office and I won't be back online for several days as such I'm assigning this to Petr to look at. Tony, thanks for the work on this. I've had a quick look over the patch now, there are a couple of minor things that pop out regarding our coding guidelines however all in all the patch definitely looks like a good improvement! Petr, I'll leave this to you being the expert here. I haven't delved down into this myself however there certainly seems to be an easy performance win here. Cheers Sam
          Hide
          Tony Levi added a comment -

          Hi,

          I've had a squint at this and can't identify the style issues; if anyone can clarify what those might be I'll take it on board for future patches...

          I guess there isn't docblock on the new function, but no others there did already, its not exactly uncommon

          Cheers,
          Tony

          Show
          Tony Levi added a comment - Hi, I've had a squint at this and can't identify the style issues; if anyone can clarify what those might be I'll take it on board for future patches... I guess there isn't docblock on the new function, but no others there did already, its not exactly uncommon Cheers, Tony
          Hide
          Tony Levi added a comment -

          Oh of course, I forgot we also added if-modified-since to our code before this, should have put it with this patch too.

          Nice catch Petr.

          Show
          Tony Levi added a comment - Oh of course, I forgot we also added if-modified-since to our code before this, should have put it with this patch too. Nice catch Petr.
          Hide
          Petr Škoda added a comment -

          thanks a lot for discovering of this issue and the patch!

          Show
          Petr Škoda added a comment - thanks a lot for discovering of this issue and the patch!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
          Hide
          Sam Hemelryk added a comment -

          Passing - thanks guys

          Show
          Sam Hemelryk added a comment - Passing - thanks guys
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many thanks for the hard work, this has been sent upstream and is available in all the git and cvs repositories.

          Show
          Eloy Lafuente (stronk7) added a comment - Many thanks for the hard work, this has been sent upstream and is available in all the git and cvs repositories.

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: