Try to correctly follow RFC 2616 13.3 on validating stale cache

responses by teaching mod_cache's cache_select_url and
cache_save_filter how to deal with this corner case.
PR:
Obtained from:
Submitted by:	jerenkrantz
Reviewed by:	stoddard, jerenkrantz, minfrin, jim


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

8 files changed, 96 insertions, 52 deletions

diff --git a/CHANGES b/CHANGES
index 8413256f02..feaa29c3f5 100644
--- a/CHANGES
+++ b/CHANGES
@@ -1,5 +1,8 @@
 Changes with Apache 2.0.53
 
+  *) mod_cache: Try to correctly follow RFC 2616 13.3 on validating stale
+     cache responses.  [Justin Erenkrantz]
+
   *) mod_rewrite: Handle per-location rules when r->filename is unset.
      Previously this would segfault or simply not match as expected,
      depending on the platform.  [Jeff Trawick]
diff --git a/STATUS b/STATUS
index 835c5ff0b5..42843dc4c6 100644
--- a/STATUS
+++ b/STATUS
@@ -1,5 +1,5 @@
 APACHE 2.0 STATUS:                                              -*-text-*-
-Last modified at [$Date: 2004/10/13 17:54:43 $]
+Last modified at [$Date: 2004/10/13 18:12:20 $]
 
 Release:
 
@@ -115,17 +115,6 @@ PATCHES TO BACKPORT FROM 2.1
        jorton replies: it does indeed, hang on...
        +1: jorton
 
-    *) Try to correctly follow RFC 2616 13.3 on validating stale cache
-       responses by teaching mod_cache's cache_select_url and
-       cache_save_filter how to deal with this corner case.
-        modules/experimental/cache_storage.c?r1=1.39&r2=1.40
-        modules/experimental/cache_util.c?r1=1.34&r2=1.35
-        modules/experimental/mod_cache.c?r1=1.91&r2=1.92
-        modules/experimental/mod_cache.h?r1=1.50&r2=1.51
-        modules/experimental/mod_disk_cache.c?r1=1.64&r2=1.65
-        modules/experimental/mod_mem_cache.c?r1=1.117&r2=1.118
-       +1: stoddard, jerenkrantz, minfrin, jim
-
     *) mod_rewrite:Fix query string handling for proxied URLs. PR 14518.
        (2.0 + 1.3)
          modules/mappers/mod_rewrite.c: r1.259
diff --git a/modules/experimental/cache_storage.c b/modules/experimental/cache_storage.c
index b37608e8ca..a0356c8d95 100644
--- a/modules/experimental/cache_storage.c
+++ b/modules/experimental/cache_storage.c
@@ -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;
         }
diff --git a/modules/experimental/cache_util.c b/modules/experimental/cache_util.c
index bfb0c740db..9ee9f84c9a 100644
--- a/modules/experimental/cache_util.c
+++ b/modules/experimental/cache_util.c
@@ -22,12 +22,12 @@
 /* -------------------------------------------------------------- */
 
 /* 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;
diff --git a/modules/experimental/mod_cache.c b/modules/experimental/mod_cache.c
index a47424857e..7c8f34d373 100644
--- a/modules/experimental/mod_cache.c
+++ b/modules/experimental/mod_cache.c
@@ -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.
          */
@@ -510,35 +516,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);
@@ -647,6 +654,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);
     }
diff --git a/modules/experimental/mod_cache.h b/modules/experimental/mod_cache.h
index 7e964af5c1..8836c0e65c 100644
--- a/modules/experimental/mod_cache.h
+++ b/modules/experimental/mod_cache.h
@@ -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 */
@@ -218,7 +218,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 */
@@ -244,7 +247,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);
diff --git a/modules/experimental/mod_disk_cache.c b/modules/experimental/mod_disk_cache.c
index 1a8e3b7d83..6ec6fd9e04 100644
--- a/modules/experimental/mod_disk_cache.c
+++ b/modules/experimental/mod_disk_cache.c
@@ -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);
diff --git a/modules/experimental/mod_mem_cache.c b/modules/experimental/mod_mem_cache.c
index 4803f05ba2..5cb1bb7419 100644
--- a/modules/experimental/mod_mem_cache.c
+++ b/modules/experimental/mod_mem_cache.c
@@ -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;
     }