mod_cache: If a 304 response indicates an entity not currently cached, then

the cache MUST disregard the response and repeat the request without the
conditional.

trunk patch: http://svn.apache.org/r1481197
2.4.x patch: http://people.apache.org/~minfrin/httpd-mod_cache-uncacheable304-2.4.patch

Submitted by: minfrin
Reviewed by: jim, wrowe

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1487131 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/CHANGES b/CHANGES
index ca2b33a8629f3d4039864182bbfbcf435a372529..3b6e851714efb0a7b67d1f39e0daf7cc9d5822f3 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -2,6 +2,10 @@
 
 Changes with Apache 2.4.5
 
+  *) mod_cache: If a 304 response indicates an entity not currently cached, then
+     the cache MUST disregard the response and repeat the request without the
+     conditional. [Graham Leggett, Co-Advisor <coad measurement-factory.com>]
+
   *) mod_cache: Ensure that we don't attempt to replace a cached response
      with an older response as per RFC2616 13.12. [Graham Leggett, Co-Advisor
      <coad measurement-factory.com>]

diff --git a/STATUS b/STATUS
index 9eba1de70b9fa43fb08922e67c2dcbe7a3cd9b28..54ebb9d3018dc93e12a5c743ec7c8daccbaeb8e8 100644 (file)
--- a/STATUS
+++ b/STATUS
@@ -90,13 +90,6 @@ RELEASE SHOWSTOPPERS:
 PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
   [ start all new proposals below, under PATCHES PROPOSED. ]
  
-    * mod_cache: If a 304 response indicates an entity not currently cached, then
-      the cache MUST disregard the response and repeat the request without the 
-      conditional.
-      trunk patch: http://svn.apache.org/r1481197
-      2.4.x patch: http://people.apache.org/~minfrin/httpd-mod_cache-uncacheable304-2.4.patch
-      +1: minfrin, jim, wrowe
-
 
 PATCHES PROPOSED TO BACKPORT FROM TRUNK:
   [ New proposals should be added at the end of the list ]

diff --git a/modules/cache/mod_cache.c b/modules/cache/mod_cache.c
index 15fc7e86e01d9530d1fda814e250cb4c248c8de0..513448a5837f7fe751cf0f6cb840333b1791a647 100644 (file)
--- a/modules/cache/mod_cache.c
+++ b/modules/cache/mod_cache.c
@@ -1160,69 +1160,46 @@ static apr_status_t cache_save_filter(ap_filter_t *f, apr_bucket_brigade *in)
     }
 
     /* Hold the phone. Some servers might allow us to cache a 2xx, but
-     * then make their 304 responses non cacheable. This leaves us in a
-     * sticky position. If the 304 is in answer to our own conditional
-     * request, we cannot send this 304 back to the client because the
-     * client isn't expecting it. Instead, our only option is to respect
-     * the answer to the question we asked (has it changed, answer was
-     * no) and return the cached item to the client, and then respect
-     * the uncacheable nature of this 304 by allowing the remove_url
-     * filter to kick in and remove the cached entity.
+     * then make their 304 responses non cacheable. RFC2616 says this:
+     *
+     * If a 304 response indicates an entity not currently cached, then
+     * the cache MUST disregard the response and repeat the request
+     * without the conditional.
+     *
+     * A 304 response with contradictory headers is technically a
+     * different entity, to be safe, we remove the entity from the cache.
      */
-    if (reason && r->status == HTTP_NOT_MODIFIED &&
-             cache->stale_handle) {
-        apr_bucket_brigade *bb;
-        apr_bucket *bkt;
-        int status;
-
-        cache->handle = cache->stale_handle;
-        info = &cache->handle->cache_obj->info;
-
-        /* Load in the saved status and clear the status line. */
-        r->status = info->status;
-        r->status_line = NULL;
-
-        bb = apr_brigade_create(r->pool, r->connection->bucket_alloc);
-
-        r->headers_in = cache->stale_headers;
-        r->headers_out = ap_cache_cacheable_headers_out(r);
+    if (reason && r->status == HTTP_NOT_MODIFIED && cache->stale_handle) {
 
-        /* Merge in our cached headers.  However, keep any updated values. */
-        /* take output, overlay on top of cached */
-        cache_accept_headers(cache->handle, r, r->headers_out,
-                cache->handle->resp_hdrs, 1);
+        ap_log_rerror(
+                APLOG_MARK, APLOG_INFO, 0, r, APLOGNO() "cache: %s responded with an uncacheable 304, retrying the request. Reason: %s", r->unparsed_uri, reason);
 
-        status = ap_meets_conditions(r);
-        if (status != OK) {
-            r->status = status;
+        /* we've got a cache conditional miss! tell anyone who cares */
+        cache_run_cache_status(cache->handle, r, r->headers_out, AP_CACHE_MISS,
+                apr_psprintf(r->pool,
+                        "conditional cache miss: 304 was uncacheable, entity removed: %s",
+                        reason));
 
-            bkt = apr_bucket_flush_create(bb->bucket_alloc);
-            APR_BRIGADE_INSERT_TAIL(bb, bkt);
-        }
-        else {
-            cache->provider->recall_body(cache->handle, r->pool, bb);
+        /* remove the cached entity immediately, we might cache it again */
+        ap_remove_output_filter(cache->remove_url_filter);
+        cache_remove_url(cache, r);
 
-            bkt = apr_bucket_eos_create(bb->bucket_alloc);
-            APR_BRIGADE_INSERT_TAIL(bb, bkt);
-        }
+        /* let someone else attempt to cache */
+        cache_remove_lock(conf, cache, r, NULL);
 
-        cache->block_response = 1;
+        /* remove this filter from the chain */
+        ap_remove_output_filter(f);
 
-        /* we've got a cache conditional hit! tell anyone who cares */
-        cache_run_cache_status(
-                cache->handle,
-                r,
-                r->headers_out,
-                AP_CACHE_REVALIDATE,
-                apr_psprintf(
-                        r->pool,
-                        "conditional cache hit: 304 was uncacheable though (%s); entity removed",
-                        reason));
+        /* retry without the conditionals */
+        apr_table_unset(r->headers_in, "If-Match");
+        apr_table_unset(r->headers_in, "If-Modified-Since");
+        apr_table_unset(r->headers_in, "If-None-Match");
+        apr_table_unset(r->headers_in, "If-Range");
+        apr_table_unset(r->headers_in, "If-Unmodified-Since");
 
-        /* let someone else attempt to cache */
-        cache_remove_lock(conf, cache, r, NULL);
+        ap_internal_redirect(r->uri, r);
 
-        return ap_pass_brigade(f->next, bb);
+        return APR_SUCCESS;
     }
 
     if (reason) {