Moodle
  1. Moodle
  2. MDL-28709

Performance: Send cache/contenttype headers with 304 responses

    Details

      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...

        Gliffy Diagrams

          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 Skoda added a comment -

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

            Show
            Petr Skoda 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: