]> granicus.if.org Git - apache/commitdiff
Try to correctly follow RFC 2616 13.3 on validating stale cache responses by
authorJustin Erenkrantz <jerenkrantz@apache.org>
Tue, 28 Sep 2004 17:37:54 +0000 (17:37 +0000)
committerJustin Erenkrantz <jerenkrantz@apache.org>
Tue, 28 Sep 2004 17:37:54 +0000 (17:37 +0000)
teaching mod_cache's cache_select_url and cache_save_filter how to deal with
this corner case.

* modules/experimental/cache_storage.c
  (cache_select_url): If we have a stale entry, save the handle so that
  cache_save_filter can use it later, and make the request conditional.
* modules/experimental/cache_util.c
  (ap_cache_request_is_conditional): Take in a table rather than request_rec.
* modules/experimental/mod_cache.c
  (cache_out_filter): Fix bogus comment.
  (cache_save_filter): If we have already responded to the client, block all
  data; correctly merge in 'stale' handles that are not really stale; set
  r->status where appropriate; serve cached response if 'fresh'
* modules/experimental/mod_cache.h
  (cache_info): Add a status field.
  (cache_request_rec): Add stale handle field and note we may block responses.
  (ap_cache_request_is_conditional): Update prototype.
* modules/experimental/mod_disk_cache.c
  (store_headers): Use cache_info status instead of r->status.
* modules/experimental/mod_mem_cache.c
  (recall_headers): Properly recall the status field.
  (store_headers): Store the status field via cache_info status.

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@105322 13f79535-47bb-0310-9956-ffa450edef68

CHANGES
modules/experimental/cache_storage.c
modules/experimental/cache_util.c
modules/experimental/mod_cache.c
modules/experimental/mod_cache.h
modules/experimental/mod_disk_cache.c
modules/experimental/mod_mem_cache.c

diff --git a/CHANGES b/CHANGES
index bd4923e479a733e9911351fdd65eac7f5343d7f3..8b55901c85ea3f2aa6ceccceec322c6a295b4860 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -2,6 +2,9 @@ Changes with Apache 2.1.0-dev
 
   [Remove entries to the current 2.0 section below, when backported]
 
+  *) mod_cache: Try to correctly follow RFC 2616 13.3 on validating stale
+     cache responses.  [Justin Erenkrantz]
+
   *) mod_disk_cache: Do not store aborted content.  PR 21492.
      [Rüdiger Plüm <r.pluem t-online.de>]
 
index 329031ec4792356d6dd1f6648f18797804e1743c..a300bcb33429c1b7438f05192e23865c8d6b788b 100644 (file)
@@ -245,10 +245,32 @@ int cache_select_url(request_rec *r, char *url)
                 }
             }
 
+            cache->provider = list->provider;
+            cache->provider_name = list->provider_name;
+
             /* Is our cached response fresh enough? */
             fresh = ap_cache_check_freshness(h, r);
             if (!fresh) {
-                list->provider->remove_entity(h);
+                cache_info *info = &(h->cache_obj->info);
+
+                /* Make response into a conditional */
+                /* FIXME: What if the request is already conditional? */
+                if (info && info->etag) {
+                    /* if we have a cached etag */
+                    cache->stale_headers = apr_table_copy(r->pool,
+                                                          r->headers_in);
+                    apr_table_set(r->headers_in, "If-None-Match", info->etag);
+                    cache->stale_handle = h;
+                }
+                else if (info && info->lastmods) {
+                    /* if we have a cached Last-Modified header */
+                    cache->stale_headers = apr_table_copy(r->pool,
+                                                          r->headers_in);
+                    apr_table_set(r->headers_in, "If-Modified-Since",
+                                  info->lastmods);
+                    cache->stale_handle = h;
+                }
+
                 return DECLINED;
             }
 
@@ -258,8 +280,6 @@ int cache_select_url(request_rec *r, char *url)
             r->filename = apr_pstrdup(r->pool, h->cache_obj->info.filename);
             accept_headers(h, r);
 
-            cache->provider = list->provider;
-            cache->provider_name = list->provider_name;
             cache->handle = h;
             return OK;
         }
index bfb0c740db23ee79f4ac128b11530c61fd5897d7..9ee9f84c9afc5494c539769420771bc019699394 100644 (file)
 /* -------------------------------------------------------------- */
 
 /* return true if the request is conditional */
-CACHE_DECLARE(int) ap_cache_request_is_conditional(request_rec *r)
+CACHE_DECLARE(int) ap_cache_request_is_conditional(apr_table_t *table)
 {
-    if (apr_table_get(r->headers_in, "If-Match") ||
-        apr_table_get(r->headers_in, "If-None-Match") ||
-        apr_table_get(r->headers_in, "If-Modified-Since") ||
-        apr_table_get(r->headers_in, "If-Unmodified-Since")) {
+    if (apr_table_get(table, "If-Match") ||
+        apr_table_get(table, "If-None-Match") ||
+        apr_table_get(table, "If-Modified-Since") ||
+        apr_table_get(table, "If-Unmodified-Since")) {
         return 1;
     }
     return 0;
index 9daff6f6009cd53b933563584d41352358bfba62..45928d68f9a4ee4f664f5a1c96c7c9c6531e1b80 100644 (file)
@@ -219,7 +219,7 @@ static int cache_out_filter(ap_filter_t *f, apr_bucket_brigade *bb)
     ap_log_error(APLOG_MARK, APLOG_DEBUG, APR_SUCCESS, r->server,
                  "cache: running CACHE_OUT filter");
 
-    /* recall_body() was called in cache_select_url() */
+    /* recall_headers() was called in cache_select_url() */
     cache->provider->recall_body(cache->handle, r->pool, bb);
 
     /* This filter is done once it has served up its content */
@@ -290,6 +290,12 @@ static int cache_save_filter(ap_filter_t *f, apr_bucket_brigade *in)
      * This section passes the brigades into the cache modules, but only
      * if the setup section (see below) is complete.
      */
+    if (cache->block_response) {
+        /* We've already sent down the response and EOS.  So, ignore
+         * whatever comes now.
+         */
+        return APR_SUCCESS;
+    }
 
     /* have we already run the cachability check and set up the
      * cached file handle? 
@@ -312,7 +318,6 @@ static int cache_save_filter(ap_filter_t *f, apr_bucket_brigade *in)
      * parameters, and decides whether this URL should be cached at
      * all. This section is* run before the above section.
      */
-    info = apr_pcalloc(r->pool, sizeof(cache_info));
 
     /* read expiry date; if a bad date, then leave it so the client can
      * read it 
@@ -332,7 +337,7 @@ static int cache_save_filter(ap_filter_t *f, apr_bucket_brigade *in)
 
     /* read the last-modified date; if the date is bad, then delete it */
     lastmods = apr_table_get(r->err_headers_out, "Last-Modified");
-    if (lastmods ==NULL) {
+    if (lastmods == NULL) {
         lastmods = apr_table_get(r->headers_out, "Last-Modified");
     }
     if (lastmods != NULL) {
@@ -384,7 +389,8 @@ static int cache_save_filter(ap_filter_t *f, apr_bucket_brigade *in)
          */
         reason = "Query string present but no expires header";
     }
-    else if (r->status == HTTP_NOT_MODIFIED && (NULL == cache->handle)) {
+    else if (r->status == HTTP_NOT_MODIFIED &&
+             !cache->handle && !cache->stale_handle) {
         /* if the server said 304 Not Modified but we have no cache
          * file - pass this untouched to the user agent, it's not for us.
          */
@@ -503,35 +509,36 @@ static int cache_save_filter(ap_filter_t *f, apr_bucket_brigade *in)
      * - cache->handle == NULL. In this case there is no previously
      * cached entity anywhere on the system. We must create a brand
      * new entity and store the response in it.
-     * - cache->handle != NULL. In this case there is a stale
+     * - cache->stale_handle != NULL. In this case there is a stale
      * entity in the system which needs to be replaced by new
      * content (unless the result was 304 Not Modified, which means
      * the cached entity is actually fresh, and we should update
      * the headers).
      */
+
+    /* Did we have a stale cache entry that really is stale? */
+    if (cache->stale_handle) {
+        if (r->status == HTTP_NOT_MODIFIED) {
+            /* Oh, hey.  It isn't that stale!  Yay! */
+            cache->handle = cache->stale_handle;
+            info = &cache->handle->cache_obj->info;
+        }
+        else {
+            /* Oh, well.  Toss it. */
+            cache->provider->remove_entity(cache->stale_handle);
+            /* Treat the request as if it wasn't conditional. */
+            cache->stale_handle = NULL;
+        }
+    }
+
     /* no cache handle, create a new entity */
     if (!cache->handle) {
         rv = cache_create_entity(r, url, size);
+        info = apr_pcalloc(r->pool, sizeof(cache_info));
+        /* We only set info->status upon the initial creation. */
+        info->status = r->status;
     }
-    /* pre-existing cache handle and 304, make entity fresh */
-    else if (r->status == HTTP_NOT_MODIFIED) {
-        /* update headers: TODO */
-
-        /* remove this filter ??? */
 
-        /* XXX is this right?  we must set rv to something other than OK 
-         * in this path
-         */
-        rv = HTTP_NOT_MODIFIED;
-    }
-    /* pre-existing cache handle and new entity, replace entity
-     * with this one
-     */
-    else {
-        cache->provider->remove_entity(cache->handle);
-        rv = cache_create_entity(r, url, size);
-    }
-    
     if (rv != OK) {
         /* Caching layer declined the opportunity to cache the response */
         ap_remove_output_filter(f);
@@ -640,6 +647,31 @@ static int cache_save_filter(ap_filter_t *f, apr_bucket_brigade *in)
      * Write away header information to cache.
      */
     rv = cache->provider->store_headers(cache->handle, r, info);
+
+    /* Did we actually find an entity before, but it wasn't really stale? */
+    if (rv == APR_SUCCESS && cache->stale_handle) {
+        apr_bucket_brigade *bb;
+        apr_bucket *bkt;
+
+        bb = apr_brigade_create(r->pool, r->connection->bucket_alloc);
+
+        /* Were we initially a conditional request? */
+        if (ap_cache_request_is_conditional(cache->stale_headers)) {
+            /* FIXME: Should we now go and make sure it's really not
+             * modified since what the user thought?
+             */
+            bkt = apr_bucket_eos_create(bb->bucket_alloc);
+            APR_BRIGADE_INSERT_TAIL(bb, bkt);
+        }
+        else {
+            r->status = info->status;
+            cache->provider->recall_body(cache->handle, r->pool, bb);
+        }
+
+        cache->block_response = 1;
+        return ap_pass_brigade(f->next, bb);
+    }
+
     if (rv == APR_SUCCESS) {
         rv = cache->provider->store_body(cache->handle, r, in);
     }
index e519940e668d7bd4b074bd296d72b264dcd65bec..1135a4de1be71dade9d66f1fe4f44fc2cdd980b3 100644 (file)
@@ -138,6 +138,7 @@ typedef struct {
 /* cache info information */
 typedef struct cache_info cache_info;
 struct cache_info {
+    int status;
     char *content_type;
     char *etag;
     char *lastmods;         /* last modified of cache entity */
@@ -153,7 +154,6 @@ struct cache_info {
     apr_time_t ius;    /*  If-UnModified_Since header value    */
     const char *im;         /* If-Match header value */
     const char *inm;         /* If-None-Match header value */
-
 };
 
 /* cache handle information */
@@ -217,7 +217,10 @@ typedef struct {
     const char *provider_name;              /* current cache provider name */
     int fresh;                         /* is the entitey fresh? */
     cache_handle_t *handle;            /* current cache handle */
-    int in_checked;                    /* CACHE_IN must cache the entity */
+    cache_handle_t *stale_handle;      /* stale cache handle */
+    apr_table_t *stale_headers;                /* original request headers. */
+    int in_checked;                    /* CACHE_SAVE must cache the entity */
+    int block_response;                        /* CACHE_SAVE must block response. */
     apr_bucket_brigade *saved_brigade;  /* copy of partial response */
     apr_off_t saved_size;               /* length of saved_brigade */
     apr_time_t exp;                     /* expiration */
@@ -243,7 +246,7 @@ CACHE_DECLARE(void) ap_cache_usec2hex(apr_time_t j, char *y);
 CACHE_DECLARE(char *) generate_name(apr_pool_t *p, int dirlevels, 
                                     int dirlength, 
                                     const char *name);
-CACHE_DECLARE(int) ap_cache_request_is_conditional(request_rec *r);
+CACHE_DECLARE(int) ap_cache_request_is_conditional(apr_table_t *table);
 CACHE_DECLARE(cache_provider_list *)ap_cache_get_providers(request_rec *r, cache_server_conf *conf, const char *url);
 CACHE_DECLARE(int) ap_cache_liststr(apr_pool_t *p, const char *list,
                                     const char *key, char **val);
index 125bec9c40688f1469152fd93b059f8a63131ad8..c835f6f193b2fcdb9141f7df485a775433901c8e 100644 (file)
@@ -589,14 +589,9 @@ static apr_status_t store_headers(cache_handle_t *h, request_rec *r, cache_info
         disk_info.entity_version = dobj->disk_info.entity_version++;
         disk_info.request_time = info->request_time;
         disk_info.response_time = info->response_time;
+        disk_info.status = info->status;
 
         disk_info.name_len = strlen(dobj->name);
-        disk_info.status = r->status;
-
-        /* This case only occurs when the content is generated locally */
-        if (!r->status_line) {
-            r->status_line = ap_get_status_line(r->status);
-        }
 
         iov[0].iov_base = (void*)&disk_info;
         iov[0].iov_len = sizeof(disk_cache_info_t);
index 108522f498116f4d0a4ffcaa6db5b6d2d7cd43aa..f34ba809d72cb035c99a1c7bb29cdbc112ef115c 100644 (file)
@@ -693,6 +693,7 @@ static apr_status_t recall_headers(cache_handle_t *h, request_rec *r)
      * CACHE_IN runs before header filters....
      */
     h->content_type = h->cache_obj->info.content_type;
+    h->status = h->cache_obj->info.status;
 
     return rc;
 }
@@ -766,6 +767,7 @@ static apr_status_t store_headers(cache_handle_t *h, request_rec *r, cache_info
     }
  
     /* Init the info struct */
+    obj->info.status = info->status;
     if (info->date) {
         obj->info.date = info->date;
     }