Wicket
  1. Wicket
  2. WICKET-1748

304 Last Modified responses should include an Expires header

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.3.4, 1.4-M3
    • Fix Version/s: 1.3.5, 1.4-RC1
    • Component/s: wicket
    • Labels:
      None

      Description

      I've been experimenting with Wicket and Apache's mod_cache to speed up resource serving performance and I've come across a communications problem between the two. When mod_cache is required to verify the status of a cached resource by a client that unconditionally GETs with a max-age of 0, mod_cache conditionally requests the resource from Wicket with an If-Modified-Since header. Assuming the resource has not been modified, WicketFilter responds as it always does with a 304 Not Modified and nothing else. mod_cache doesn't process the request and just forwards it on to the unprepared client, because mod_cache is following RFC 2616/13.9 that says not to handle query string URL responses unless they have an Expires header. This is the bug:
      https://issues.apache.org/bugzilla/show_bug.cgi?id=45341

      I suspect the behavior exhibited by mod_cache in this scenario is unintended, but the Apache guy is saying that a 304 response must contain all headers relevant for caching. Whether or not that is required by the standard, it is certainly better that the 304 response contain a new Expires header. Consider: a resource in a cache (let's say the browser cache) has passed its expiration date. The browser, then, posts an If-Modified-Since and Wicket responds, simply, "no". This does not update the expiration date for the client, and never will. This resource will always be "expired" and need to be revalidated with every request, instead of having the default shelf-life of 1-hour. Wicket should ideally be resetting the expiration date by setting an Expires header with a 304 the same as it does a 200. This would avoid the mod_cache bug/feature, and improve caching precision generally.

      1. last_mod.patch
        0.6 kB
        Nathan Hamblen

        Activity

        Hide
        Timo Rantalaiho added a comment -

        Fixing both in trunk and in 1.3.x

        Show
        Timo Rantalaiho added a comment - Fixing both in trunk and in 1.3.x
        Hide
        Timo Rantalaiho added a comment -

        Thanks Nathan, this sounds like a good band-aid until resource caching is cleaned up more, and the issue sounds familiar to me too.

        Show
        Timo Rantalaiho added a comment - Thanks Nathan, this sounds like a good band-aid until resource caching is cleaned up more, and the issue sounds familiar to me too.
        Hide
        Nathan Hamblen added a comment -

        I've patched this in a pretty naive way, just to confirm that setting an Expires when sending 304 fixes the problem with mod_cache. It does. Ideally, though, WicketFilter would use the value from WebResource.getCacheDuration(). I didn't attempt that because it would require wider changes in WicketFilter and because I've seen comments indicating that caching is to be moved outside of Resource anyway. So, I don't know. For me just setting the default duration of 1-hour to 304 responses fixes the bug (whosever bug it is) and won't cause any problems I can think of. (The resource has to have been set cacheable to get to that point anyway.)

        Show
        Nathan Hamblen added a comment - I've patched this in a pretty naive way, just to confirm that setting an Expires when sending 304 fixes the problem with mod_cache. It does. Ideally, though, WicketFilter would use the value from WebResource.getCacheDuration(). I didn't attempt that because it would require wider changes in WicketFilter and because I've seen comments indicating that caching is to be moved outside of Resource anyway. So, I don't know. For me just setting the default duration of 1-hour to 304 responses fixes the bug (whosever bug it is) and won't cause any problems I can think of. (The resource has to have been set cacheable to get to that point anyway.)

          People

          • Assignee:
            Timo Rantalaiho
            Reporter:
            Nathan Hamblen
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development