On 04/16/2007 10:58 PM, Ruediger Pluem wrote:
> Looking at the problem I noticed a related problem already mentioned in a > FIXME comment: > It can happen that the headers of a 304 response from the backend cause the > response to be > uncacheable (e.g. Cache-Control: no-store). > Currently this leads to a wrong response (304) if the original request was > unconditional > (just like in your HEAD case and your HEAD case will also fail here, even > after your patch). The following patch should fix this. Any comments (especially is it RFC compliant to serve the content from the cache in this case as Henrik said in http://mail-archives.apache.org/mod_mbox/httpd-dev/200704.mbox/[EMAIL PROTECTED] ?) Regards Rüdiger Index: modules/cache/mod_cache.c =================================================================== --- modules/cache/mod_cache.c (Revision 539392) +++ modules/cache/mod_cache.c (Arbeitskopie) @@ -318,6 +318,7 @@ static int cache_save_filter(ap_filter_t *f, apr_bucket_brigade *in) { int rv = !OK; + int store_headers = 1; int date_in_errhdr = 0; request_rec *r = f->r; cache_request_rec *cache; @@ -486,11 +487,6 @@ * indicating do not cache, or stop now if you are * trying to cache it. */ - /* FIXME: The Cache-Control: no-store could have come in on a 304, - * FIXME: while the original request wasn't conditional. IOW, we - * FIXME: made the the request conditional earlier to revalidate - * FIXME: our cached response. - */ reason = "Cache-Control: no-store present"; } else if (!conf->store_private && @@ -499,7 +495,6 @@ * this object is marked for this user's eyes only. Behave * as a tunnel. */ - /* FIXME: See above (no-store) */ reason = "Cache-Control: private present"; } else if (apr_table_get(r->headers_in, "Authorization") != NULL @@ -526,15 +521,33 @@ } if (reason) { - ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server, - "cache: %s not cached. Reason: %s", r->unparsed_uri, - reason); + /* + * It may be possible that + * - we have a stale entity in the cache + * - the original request was unconditional + * - the conditions added by the cache caused the origin server to + * respond with a 304 + * - for some reason we are not allowed to cache the response by the + * origin server, e.g. Cache-Control: no-store / + * Cache-Control: private + * In this case we should serve the client from the cache, but should + * NOT update the headers of the cached entity (only sent the updated + * headers to the client). + */ + if ((r->status == HTTP_NOT_MODIFIED) && cache->stale_handle) { + store_headers = 0; + } + else { + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server, + "cache: %s not cached. Reason: %s", r->unparsed_uri, + reason); - /* remove this filter from the chain */ - ap_remove_output_filter(f); + /* remove this filter from the chain */ + ap_remove_output_filter(f); - /* ship the data up the stack */ - return ap_pass_brigade(f->next, in); + /* ship the data up the stack */ + return ap_pass_brigade(f->next, in); + } } /* Make it so that we don't execute this path again. */ @@ -782,14 +795,26 @@ ap_cache_accept_headers(cache->handle, r, 1); } - /* Write away header information to cache. It is possible that we are - * trying to update headers for an entity which has already been cached. - * - * This may fail, due to an unwritable cache area. E.g. filesystem full, - * permissions problems or a read-only (re)mount. This must be handled - * later. + /* + * Check if we are allowed to save / update the headers of the cached + * entity. See comments above why this might not me the case. If we + * are not allowed to save / update the headers of the cached entity we + * regard this operation as successful. */ - rv = cache->provider->store_headers(cache->handle, r, info); + if (store_headers) { + /* + * Write away header information to cache. It is possible that we are + * trying to update headers for an entity which has already been cached. + * + * This may fail, due to an unwritable cache area. E.g. filesystem full, + * permissions problems or a read-only (re)mount. This must be handled + * later. + */ + rv = cache->provider->store_headers(cache->handle, r, info); + } + else { + rv = APR_SUCCESS; + } /* Did we just update the cached headers on a revalidated response? *
