You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by je...@apache.org on 2004/09/22 00:56:23 UTC
cvs commit: httpd-2.0/modules/experimental cache_storage.c cache_util.c mod_cache.c mod_cache.h mod_disk_cache.c mod_mem_cache.c
jerenkrantz 2004/09/21 15:56:23
Modified: . CHANGES
modules/experimental cache_storage.c cache_util.c
mod_cache.c mod_cache.h mod_disk_cache.c
mod_mem_cache.c
Log:
Fix Expires (freshness) handling in mod_cache.
Previously, if the cached copy was stale, the response would go into an
indeterminate state. Therefore, the freshness check must be done before we
'accept' the response and, if it fails (i.e. stale), we can't allow any side
effects.
This caused a number of changes to how mod_disk_cache reads its headers as
ap_scan_script_header_err() purposely has side-effects and that's
unacceptable. So, factor out only what we need.
Also, remove the broken conditional filter code as you can't reliably alter the
filter list once the response is started. (Regardless, cache_select_url()
has the freshness checks now.)
Assist to Sascha Schumann for reporting mod_cache was busted.
Revision Changes Path
1.1596 +5 -0 httpd-2.0/CHANGES
Index: CHANGES
===================================================================
RCS file: /home/cvs/httpd-2.0/CHANGES,v
retrieving revision 1.1595
retrieving revision 1.1596
diff -u -u -r1.1595 -r1.1596
--- CHANGES 21 Sep 2004 13:23:47 -0000 1.1595
+++ CHANGES 21 Sep 2004 22:56:22 -0000 1.1596
@@ -2,6 +2,11 @@
[Remove entries to the current 2.0 section below, when backported]
+ *) Fix Expires handling in mod_cache. [Justin Erenkrantz]
+
+ *) Alter mod_expires to run at a different filter priority to allow
+ proper Expires storage by mod_cache. [Justin Erenkrantz]
+
*) Fix the global mutex crash when the global mutex is never allocated due
to disabled/empty caches. [Jess Holle <jessh ptc.com>]
1.37 +82 -19 httpd-2.0/modules/experimental/cache_storage.c
Index: cache_storage.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/experimental/cache_storage.c,v
retrieving revision 1.36
retrieving revision 1.37
diff -u -u -r1.36 -r1.37
--- cache_storage.c 5 Aug 2004 19:12:34 -0000 1.36
+++ cache_storage.c 21 Sep 2004 22:56:23 -0000 1.37
@@ -98,6 +98,57 @@
return DECLINED;
}
+static int set_cookie_doo_doo(void *v, const char *key, const char *val)
+{
+ apr_table_addn(v, key, val);
+ return 1;
+}
+
+static void accept_headers(cache_handle_t *h, request_rec *r)
+{
+ apr_table_t *cookie_table;
+ const char *v;
+
+ v = apr_table_get(h->resp_hdrs, "Content-Type");
+ if (v) {
+ ap_set_content_type(r, v);
+ apr_table_unset(h->resp_hdrs, "Content-Type");
+ }
+
+ /* If the cache gave us a Last-Modified header, we can't just
+ * pass it on blindly because of restrictions on future values.
+ */
+ v = apr_table_get(h->resp_hdrs, "Last-Modified");
+ if (v) {
+ ap_update_mtime(r, apr_date_parse_http(v));
+ ap_set_last_modified(r);
+ apr_table_unset(h->resp_hdrs, "Last-Modified");
+ }
+
+ /* The HTTP specification says that it is legal to merge duplicate
+ * headers into one. Some browsers that support Cookies don't like
+ * merged headers and prefer that each Set-Cookie header is sent
+ * separately. Lets humour those browsers by not merging.
+ * Oh what a pain it is.
+ */
+ cookie_table = apr_table_make(r->pool, 2);
+ apr_table_do(set_cookie_doo_doo, cookie_table, r->err_headers_out,
+ "Set-Cookie", NULL);
+ apr_table_do(set_cookie_doo_doo, cookie_table, h->resp_hdrs,
+ "Set-Cookie", NULL);
+ apr_table_unset(r->err_headers_out, "Set-Cookie");
+ apr_table_unset(h->resp_hdrs, "Set-Cookie");
+
+ apr_table_overlap(r->headers_out, h->resp_hdrs,
+ APR_OVERLAP_TABLES_SET);
+ apr_table_overlap(r->err_headers_out, h->resp_err_hdrs,
+ APR_OVERLAP_TABLES_SET);
+ if (!apr_is_empty_table(cookie_table)) {
+ r->err_headers_out = apr_table_overlay(r->pool, r->err_headers_out,
+ cookie_table);
+ }
+}
+
/*
* select a specific URL entity in the cache
*
@@ -118,12 +169,12 @@
cache_request_rec *cache = (cache_request_rec *)
ap_get_module_config(r->request_config, &cache_module);
- rv = cache_generate_key(r,r->pool,&key);
+ rv = cache_generate_key(r, r->pool, &key);
if (rv != APR_SUCCESS) {
return rv;
}
/* go through the cache types till we get a match */
- h = cache->handle = apr_palloc(r->pool, sizeof(cache_handle_t));
+ h = apr_palloc(r->pool, sizeof(cache_handle_t));
list = cache->providers;
@@ -132,32 +183,33 @@
case OK: {
char *vary = NULL;
const char *varyhdr = NULL;
+ int fresh;
+
if (list->provider->recall_headers(h, r) != APR_SUCCESS) {
/* TODO: Handle this error */
return DECLINED;
}
- r->filename = apr_pstrdup(r->pool, h->cache_obj->info.filename);
-
/*
* Check Content-Negotiation - Vary
*
- * At this point we need to make sure that the object we found in the cache
- * is the same object that would be delivered to the client, when the
- * effects of content negotiation are taken into effect.
- *
+ * At this point we need to make sure that the object we found in
+ * the cache is the same object that would be delivered to the
+ * client, when the effects of content negotiation are taken into
+ * effect.
+ *
* In plain english, we want to make sure that a language-negotiated
* document in one language is not given to a client asking for a
* language negotiated document in a different language by mistake.
- *
+ *
* This code makes the assumption that the storage manager will
* cache the req_hdrs if the response contains a Vary
* header.
- *
+ *
* RFC2616 13.6 and 14.44 describe the Vary mechanism.
*/
- if ((varyhdr = apr_table_get(r->err_headers_out, "Vary")) == NULL) {
- varyhdr = apr_table_get(r->headers_out, "Vary");
+ if ((varyhdr = apr_table_get(h->resp_err_hdrs, "Vary")) == NULL) {
+ varyhdr = apr_table_get(h->resp_hdrs, "Vary");
}
vary = apr_pstrdup(r->pool, varyhdr);
while (vary && *vary) {
@@ -186,16 +238,28 @@
}
else {
/* headers do not match, so Vary failed */
- ap_log_error(APLOG_MARK, APLOG_INFO, APR_SUCCESS, r->server,
- "cache_select_url(): Vary header mismatch - Cached document cannot be used. \n");
- apr_table_clear(r->headers_out);
- r->status_line = NULL;
- cache->handle = NULL;
+ ap_log_error(APLOG_MARK, APLOG_DEBUG, APR_SUCCESS,
+ r->server,
+ "cache_select_url(): Vary header mismatch.");
return DECLINED;
}
}
+
+ /* Is our cached response fresh enough? */
+ fresh = ap_cache_check_freshness(h, r);
+ if (!fresh) {
+ return DECLINED;
+ }
+
+ /* Okay, this response looks okay. Merge in our stuff and go. */
+ apr_table_setn(r->headers_out, "Content-Type",
+ ap_make_content_type(r, h->content_type));
+ 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;
}
case DECLINED: {
@@ -205,12 +269,10 @@
}
default: {
/* oo-er! an error */
- cache->handle = NULL;
return rv;
}
}
}
- cache->handle = NULL;
return DECLINED;
}
@@ -224,3 +286,4 @@
}
return APR_SUCCESS;
}
+
1.34 +16 -16 httpd-2.0/modules/experimental/cache_util.c
Index: cache_util.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/experimental/cache_util.c,v
retrieving revision 1.33
retrieving revision 1.34
diff -u -u -r1.33 -r1.34
--- cache_util.c 5 Aug 2004 19:12:34 -0000 1.33
+++ cache_util.c 21 Sep 2004 22:56:23 -0000 1.34
@@ -118,7 +118,7 @@
return apr_time_sec(current_age);
}
-CACHE_DECLARE(int) ap_cache_check_freshness(cache_request_rec *cache,
+CACHE_DECLARE(int) ap_cache_check_freshness(cache_handle_t *h,
request_rec *r)
{
apr_int64_t age, maxage_req, maxage_cresp, maxage, smaxage, maxstale;
@@ -129,7 +129,7 @@
const char *expstr = NULL;
char *val;
apr_time_t age_c = 0;
- cache_info *info = &(cache->handle->cache_obj->info);
+ cache_info *info = &(h->cache_obj->info);
/*
* We now want to check if our cached data is still fresh. This depends
@@ -163,20 +163,20 @@
* entity, and it's value is in the past, it has expired.
*
*/
- cc_cresp = apr_table_get(r->headers_out, "Cache-Control");
- cc_ceresp = apr_table_get(r->err_headers_out, "Cache-Control");
- cc_req = apr_table_get(r->headers_in, "Cache-Control");
-
- if ((agestr = apr_table_get(r->headers_out, "Age"))) {
+ cc_cresp = apr_table_get(h->resp_hdrs, "Cache-Control");
+ cc_ceresp = apr_table_get(h->resp_err_hdrs, "Cache-Control");
+ cc_req = apr_table_get(h->req_hdrs, "Cache-Control");
+
+ if ((agestr = apr_table_get(h->resp_hdrs, "Age"))) {
age_c = apr_atoi64(agestr);
}
- else if ((agestr = apr_table_get(r->err_headers_out, "Age"))) {
+ else if ((agestr = apr_table_get(h->resp_err_hdrs, "Age"))) {
age_c = apr_atoi64(agestr);
age_in_errhdr = 1;
}
- if (!(expstr = apr_table_get(r->err_headers_out, "Expires"))) {
- expstr = apr_table_get(r->headers_out, "Expires");
+ if (!(expstr = apr_table_get(h->resp_err_hdrs, "Expires"))) {
+ expstr = apr_table_get(h->resp_hdrs, "Expires");
}
/* calculate age of object */
@@ -267,23 +267,23 @@
const char *warn_head;
apr_table_t *head_ptr;
- warn_head = apr_table_get(r->headers_out, "Warning");
+ warn_head = apr_table_get(h->resp_hdrs, "Warning");
if (warn_head != NULL) {
- head_ptr = r->headers_out;
+ head_ptr = h->resp_hdrs;
}
else {
- warn_head = apr_table_get(r->err_headers_out, "Warning");
- head_ptr = r->err_headers_out;
+ warn_head = apr_table_get(h->resp_err_hdrs, "Warning");
+ head_ptr = h->resp_err_hdrs;
}
/* it's fresh darlings... */
/* set age header on response */
if (age_in_errhdr) {
- apr_table_set(r->err_headers_out, "Age",
+ apr_table_set(h->resp_err_hdrs, "Age",
apr_psprintf(r->pool, "%lu", (unsigned long)age));
}
else {
- apr_table_set(r->headers_out, "Age",
+ apr_table_set(h->resp_hdrs, "Age",
apr_psprintf(r->pool, "%lu", (unsigned long)age));
}
1.90 +3 -78 httpd-2.0/modules/experimental/mod_cache.c
Index: mod_cache.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/experimental/mod_cache.c,v
retrieving revision 1.89
retrieving revision 1.90
diff -u -u -r1.89 -r1.90
--- mod_cache.c 17 Aug 2004 16:34:51 -0000 1.89
+++ mod_cache.c 21 Sep 2004 22:56:23 -0000 1.90
@@ -28,7 +28,6 @@
*/
static ap_filter_rec_t *cache_save_filter_handle;
static ap_filter_rec_t *cache_out_filter_handle;
-static ap_filter_rec_t *cache_conditional_filter_handle;
/*
* CACHE handler
@@ -131,14 +130,10 @@
* If no existing cache file (DECLINED)
* add cache_save filter
* If cached file (OK)
- * If fresh cache file
- * clear filter stack
- * add cache_out filter
- * return OK
- * If stale cache file
- * add cache_conditional filter (which updates cache)
+ * clear filter stack
+ * add cache_out filter
+ * return OK
*/
-
rv = cache_select_url(r, url);
if (rv != OK) {
if (rv == DECLINED) {
@@ -158,38 +153,6 @@
}
/* We have located a suitable cache file now. */
-
- /* RFC2616 13.2 - Check cache object expiration */
- cache->fresh = ap_cache_check_freshness(cache, r);
-
- /* What we have in our cache isn't fresh. */
- if (!cache->fresh) {
- /* If our stale cached response was conditional... */
- if (!lookup && ap_cache_request_is_conditional(r)) {
- info = &(cache->handle->cache_obj->info);
-
- /* fudge response into a conditional */
- if (info && info->etag) {
- /* if we have a cached etag */
- apr_table_set(r->headers_in, "If-None-Match", info->etag);
- }
- else if (info && info->lastmods) {
- /* if we have a cached IMS */
- apr_table_set(r->headers_in, "If-Modified-Since",
- info->lastmods);
- }
- }
-
- /* Add cache_conditional_filter to see if we can salvage
- * later.
- */
- ap_add_output_filter_handle(cache_conditional_filter_handle,
- NULL, r, r->connection);
- return DECLINED;
- }
-
- /* fresh data available */
-
info = &(cache->handle->cache_obj->info);
if (info && info->lastmod) {
@@ -269,39 +232,6 @@
/*
- * CACHE_CONDITIONAL filter
- * ------------------------
- *
- * Decide whether or not cached content should be delivered
- * based on our fudged conditional request.
- * If response HTTP_NOT_MODIFIED
- * replace ourselves with cache_out filter
- * Otherwise
- * replace ourselves with cache_save filter
- */
-
-static int cache_conditional_filter(ap_filter_t *f, apr_bucket_brigade *in)
-{
- ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, f->r->server,
- "cache: running CACHE_CONDITIONAL filter");
-
- if (f->r->status == HTTP_NOT_MODIFIED) {
- /* replace ourselves with CACHE_OUT filter */
- ap_add_output_filter_handle(cache_out_filter_handle, NULL,
- f->r, f->r->connection);
- }
- else {
- /* replace ourselves with CACHE_SAVE filter */
- ap_add_output_filter_handle(cache_save_filter_handle, NULL,
- f->r, f->r->connection);
- }
- ap_remove_output_filter(f);
-
- return ap_pass_brigade(f->next, in);
-}
-
-
-/*
* CACHE_SAVE filter
* ---------------
*
@@ -973,11 +903,6 @@
cache_out_filter,
NULL,
AP_FTYPE_CONTENT_SET-1);
- cache_conditional_filter_handle =
- ap_register_output_filter("CACHE_CONDITIONAL",
- cache_conditional_filter,
- NULL,
- AP_FTYPE_CONTENT_SET);
ap_hook_post_config(cache_post_config, NULL, NULL, APR_HOOK_REALLY_FIRST);
}
1.50 +7 -3 httpd-2.0/modules/experimental/mod_cache.h
Index: mod_cache.h
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/experimental/mod_cache.h,v
retrieving revision 1.49
retrieving revision 1.50
diff -u -u -r1.49 -r1.50
--- mod_cache.h 15 Sep 2004 15:09:09 -0000 1.49
+++ mod_cache.h 21 Sep 2004 22:56:23 -0000 1.50
@@ -203,7 +203,11 @@
struct cache_handle {
cache_object_t *cache_obj;
- apr_table_t *req_hdrs; /* These are the original request headers */
+ apr_table_t *req_hdrs; /* cached request headers */
+ apr_table_t *resp_hdrs; /* cached response headers */
+ apr_table_t *resp_err_hdrs; /* cached response err headers */
+ const char *content_type; /* cached content type */
+ int status; /* cached status */
};
/* per request cache information */
@@ -229,11 +233,11 @@
/**
* Check the freshness of the cache object per RFC2616 section 13.2 (Expiration Model)
- * @param cache cache_request_rec
+ * @param h cache_handle_t
* @param r request_rec
* @return 0 ==> cache object is stale, 1 ==> cache object is fresh
*/
-CACHE_DECLARE(int) ap_cache_check_freshness(cache_request_rec *cache, request_rec *r);
+CACHE_DECLARE(int) ap_cache_check_freshness(cache_handle_t *h, request_rec *r);
CACHE_DECLARE(apr_time_t) ap_cache_hex2usec(const char *x);
CACHE_DECLARE(void) ap_cache_usec2hex(apr_time_t j, char *y);
CACHE_DECLARE(char *) generate_name(apr_pool_t *p, int dirlevels,
1.61 +90 -23 httpd-2.0/modules/experimental/mod_disk_cache.c
Index: mod_disk_cache.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/experimental/mod_disk_cache.c,v
retrieving revision 1.60
retrieving revision 1.61
diff -u -u -r1.60 -r1.61
--- mod_disk_cache.c 17 Aug 2004 16:34:51 -0000 1.60
+++ mod_disk_cache.c 21 Sep 2004 22:56:23 -0000 1.61
@@ -415,6 +415,88 @@
return OK;
}
+static apr_status_t read_table(cache_handle_t *handle, request_rec *r,
+ apr_table_t *table, apr_file_t *file)
+{
+ char w[MAX_STRING_LEN];
+ char *l;
+ int p;
+ apr_status_t rv;
+
+ while (1) {
+
+ /* ### What about APR_EOF? */
+ rv = apr_file_gets(w, MAX_STRING_LEN - 1, file);
+ if (rv != APR_SUCCESS) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
+ "Premature end of cache headers.");
+ return rv;
+ }
+
+ /* Delete terminal (CR?)LF */
+
+ p = strlen(w);
+ /* Indeed, the host's '\n':
+ '\012' for UNIX; '\015' for MacOS; '\025' for OS/390
+ -- whatever the script generates.
+ */
+ if (p > 0 && w[p - 1] == '\n') {
+ if (p > 1 && w[p - 2] == CR) {
+ w[p - 2] = '\0';
+ }
+ else {
+ w[p - 1] = '\0';
+ }
+ }
+
+ /* If we've finished reading the headers, break out of the loop. */
+ if (w[0] == '\0') {
+ break;
+ }
+
+#if APR_CHARSET_EBCDIC
+ /* Chances are that we received an ASCII header text instead of
+ * the expected EBCDIC header lines. Try to auto-detect:
+ */
+ if (!(l = strchr(w, ':'))) {
+ int maybeASCII = 0, maybeEBCDIC = 0;
+ unsigned char *cp, native;
+ apr_size_t inbytes_left, outbytes_left;
+
+ for (cp = w; *cp != '\0'; ++cp) {
+ native = apr_xlate_conv_byte(ap_hdrs_from_ascii, *cp);
+ if (apr_isprint(*cp) && !apr_isprint(native))
+ ++maybeEBCDIC;
+ if (!apr_isprint(*cp) && apr_isprint(native))
+ ++maybeASCII;
+ }
+ if (maybeASCII > maybeEBCDIC) {
+ ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
+ "CGI Interface Error: Script headers apparently ASCII: (CGI = %s)",
+ r->filename);
+ inbytes_left = outbytes_left = cp - w;
+ apr_xlate_conv_buffer(ap_hdrs_from_ascii,
+ w, &inbytes_left, w, &outbytes_left);
+ }
+ }
+#endif /*APR_CHARSET_EBCDIC*/
+
+ /* if we see a bogus header don't ignore it. Shout and scream */
+ if (!(l = strchr(w, ':'))) {
+ return APR_EGENERAL;
+ }
+
+ *l++ = '\0';
+ while (*l && apr_isspace(*l)) {
+ ++l;
+ }
+
+ apr_table_add(table, w, l);
+ }
+
+ return APR_SUCCESS;
+}
+
/*
* Reads headers from a buffer and returns an array of headers.
* Returns NULL on file error
@@ -433,33 +515,18 @@
return APR_NOTFOUND;
}
- if(!r->headers_out) {
- r->headers_out = apr_table_make(r->pool, 20);
- }
-
- /*
- * Call routine to read the header lines/status line
- */
- r->status = dobj->disk_info.status;
- ap_scan_script_header_err(r, dobj->hfd, NULL);
-
- apr_table_setn(r->headers_out, "Content-Type",
- ap_make_content_type(r, r->content_type));
-
h->req_hdrs = apr_table_make(r->pool, 20);
+ h->resp_hdrs = apr_table_make(r->pool, 20);
+ h->resp_err_hdrs = apr_table_make(r->pool, 20);
- /*
- * Call routine to read the header lines/status line
- *
- * Note that ap_scan_script_header_err sets to r->err_headers_out,
- * so we must set the real one aside.
- */
- tmp = r->err_headers_out;
- r->err_headers_out = h->req_hdrs;
- ap_scan_script_header_err(r, dobj->hfd, NULL);
- r->err_headers_out = tmp;
+ /* Call routine to read the header lines/status line */
+ read_table(h, r, h->resp_hdrs, dobj->hfd);
+ read_table(h, r, h->req_hdrs, dobj->hfd);
apr_file_close(dobj->hfd);
+
+ h->status = dobj->disk_info.status;
+ h->content_type = apr_table_get(h->resp_hdrs, "Content-Type");
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
"disk_cache: Recalled headers for URL %s", dobj->name);
1.117 +6 -5 httpd-2.0/modules/experimental/mod_mem_cache.c
Index: mod_mem_cache.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/experimental/mod_mem_cache.c,v
retrieving revision 1.116
retrieving revision 1.117
diff -u -u -r1.116 -r1.117
--- mod_mem_cache.c 17 Sep 2004 14:58:05 -0000 1.116
+++ mod_mem_cache.c 21 Sep 2004 22:56:23 -0000 1.117
@@ -667,8 +667,9 @@
mem_cache_object_t *mobj = (mem_cache_object_t*) h->cache_obj->vobj;
h->req_hdrs = apr_table_make(r->pool, mobj->num_req_hdrs);
- r->headers_out = apr_table_make(r->pool, mobj->num_header_out);
- r->err_headers_out = apr_table_make(r->pool, mobj->num_err_header_out);
+ h->resp_hdrs = apr_table_make(r->pool, mobj->num_header_out);
+ h->resp_err_hdrs = apr_table_make(r->pool, mobj->num_err_header_out);
+ /* ### FIXME: These two items should not be saved. */
r->subprocess_env = apr_table_make(r->pool, mobj->num_subprocess_env);
r->notes = apr_table_make(r->pool, mobj->num_notes);
@@ -677,10 +678,10 @@
h->req_hdrs);
rc = unserialize_table( mobj->header_out,
mobj->num_header_out,
- r->headers_out);
+ h->resp_hdrs);
rc = unserialize_table( mobj->err_header_out,
mobj->num_err_header_out,
- r->err_headers_out);
+ h->resp_err_hdrs);
rc = unserialize_table( mobj->subprocess_env,
mobj->num_subprocess_env,
r->subprocess_env);
@@ -691,7 +692,7 @@
/* Content-Type: header may not be set if content is local since
* CACHE_IN runs before header filters....
*/
- ap_set_content_type(r, apr_pstrdup(r->pool, h->cache_obj->info.content_type));
+ h->content_type = h->cache_obj->info.content_type;
return rc;
}
Re: cvs commit: httpd-2.0/modules/experimental cache_storage.c cache_util.c
mod_cache.c mod_cache.h mod_disk_cache.c mod_mem_cache.c
Posted by Bill Stoddard <bi...@wstoddard.com>.
ap_get_module_config(r->request_config,
>> &cache_module);
>> - rv = cache_generate_key(r,r->pool,&key);
>> + rv = cache_generate_key(r, r->pool, &key);
>> if (rv != APR_SUCCESS) {
>> return rv;
>> }
>> /* go through the cache types till we get a match */
>> - h = cache->handle = apr_palloc(r->pool, sizeof(cache_handle_t));
>> + h = apr_palloc(r->pool, sizeof(cache_handle_t));
>>
>
>
> This little gem causes a regression. Because cache->handle is left NULL,
> we never cleanup stale entries in the cache (in cache_save_filter). Once
> CacheMaxExpires hits, the stale entry will never be garbage collected
> from the cache and replaced by a new entry. Two ways to fix this come
> to mind right off hand (neither are optimal i expect):
>
> Patch 1:
> Index: cache_storage.c
> ===================================================================
> RCS file: /home/cvs/httpd-2.0/modules/experimental/cache_storage.c,v
> retrieving revision 1.38
> diff -u -r1.38 cache_storage.c
> --- cache_storage.c 22 Sep 2004 22:25:45 -0000 1.38
> +++ cache_storage.c 23 Sep 2004 18:49:17 -0000
> @@ -248,6 +248,7 @@
> /* Is our cached response fresh enough? */
> fresh = ap_cache_check_freshness(h, r);
> if (!fresh) {
> + cache->provider->remove_entity(h);
> return DECLINED;
> }
>
>
> Patch 2:
> Index: cache_storage.c
> ===================================================================
> RCS file: /home/cvs/httpd-2.0/modules/experimental/cache_storage.c,v
> retrieving revision 1.38
> diff -u -r1.38 cache_storage.c
> --- cache_storage.c 22 Sep 2004 22:25:45 -0000 1.38
> +++ cache_storage.c 23 Sep 2004 18:51:10 -0000
> @@ -248,6 +248,7 @@
> /* Is our cached response fresh enough? */
> fresh = ap_cache_check_freshness(h, r);
> if (!fresh) {
> + cache->provider->remove_entity(h);
> return DECLINED;
> }
>
>
>
> If no one ventures an opinion by this evening, I'll dig into it and come
> up with a solution. No time at the moment.
>
> Bill
>
Last post before evening... This patch solves the regression w/o segfaulting. So the question boils down to
this: Do we want to make stale cache entries available to mod_cache or not? This patch does not, I would
suggest that mod_cache -does- need to have access to stale cache entries because sometimes it may need to
serve up the stale entry. Thoughts?
Index: cache_storage.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/experimental/cache_storage.c,v
retrieving revision 1.38
diff -u -r1.38 cache_storage.c
--- cache_storage.c 22 Sep 2004 22:25:45 -0000 1.38
+++ cache_storage.c 23 Sep 2004 19:06:34 -0000
@@ -248,6 +248,7 @@
/* Is our cached response fresh enough? */
fresh = ap_cache_check_freshness(h, r);
if (!fresh) {
+ list->provider->remove_entity(h);
return DECLINED;
}
Re: cvs commit: httpd-2.0/modules/experimental cache_storage.c cache_util.c
mod_cache.c mod_cache.h mod_disk_cache.c mod_mem_cache.c
Posted by Bill Stoddard <bi...@wstoddard.com>.
Justin Erenkrantz wrote:
> --On Thursday, September 23, 2004 8:17 PM -0400 Bill Stoddard
> <bi...@wstoddard.com> wrote:
>
>> Justin Erenkrantz wrote:
>>
>>>> This little gem causes a regression. Because cache->handle is left
>>>> NULL,
>>>> we never cleanup stale entries in the cache (in cache_save_filter).
>>>> Once
>>>> CacheMaxExpires hits, the stale entry will never be garbage collected
>>>> from the cache and replaced by a new entry. Two ways to fix this come
>>>> to mind right off hand (neither are optimal i expect):
>>>
>>>
>>>
>>> Well, I don't know about mod_mem_cache, but I don't believe this affects
>>> mod_disk_cache. mod_disk_cache overwrites the old entry if it is told
>>> to do so. It's possible that mod_mem_cache should do the same.
>>
>>
>> Will investigate.
>
>
> Upon looking into what mod_mem_cache does, I think your first patch is
> the 'right' way to do this. So, I committed it.
>
> Lemme know how that fares. -- justin
>
Yep, I came to the same conclusion. Looks good. BTW Justin, nice patch (the big one I am about to port to
2.0), cleans up some crufty code paths nicely.
Bill
Re: cvs commit: httpd-2.0/modules/experimental cache_storage.c
cache_util.c mod_cache.c mod_cache.h mod_disk_cache.c mod_mem_cache.c
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On Thursday, September 23, 2004 8:17 PM -0400 Bill Stoddard
<bi...@wstoddard.com> wrote:
> Justin Erenkrantz wrote:
>>> This little gem causes a regression. Because cache->handle is left NULL,
>>> we never cleanup stale entries in the cache (in cache_save_filter). Once
>>> CacheMaxExpires hits, the stale entry will never be garbage collected
>>> from the cache and replaced by a new entry. Two ways to fix this come
>>> to mind right off hand (neither are optimal i expect):
>>
>>
>> Well, I don't know about mod_mem_cache, but I don't believe this affects
>> mod_disk_cache. mod_disk_cache overwrites the old entry if it is told
>> to do so. It's possible that mod_mem_cache should do the same.
>
> Will investigate.
Upon looking into what mod_mem_cache does, I think your first patch is the
'right' way to do this. So, I committed it.
Lemme know how that fares. -- justin
Re: cvs commit: httpd-2.0/modules/experimental cache_storage.c cache_util.c
mod_cache.c mod_cache.h mod_disk_cache.c mod_mem_cache.c
Posted by Bill Stoddard <bi...@wstoddard.com>.
Justin Erenkrantz wrote:
> --On Thursday, September 23, 2004 2:55 PM -0400 Bill Stoddard
> <bi...@wstoddard.com> wrote:
>
>> This little gem causes a regression. Because cache->handle is left NULL,
>> we never cleanup stale entries in the cache (in cache_save_filter). Once
>> CacheMaxExpires hits, the stale entry will never be garbage collected
>> from the cache and replaced by a new entry. Two ways to fix this come to
>> mind right off hand (neither are optimal i expect):
>
>
> Well, I don't know about mod_mem_cache, but I don't believe this affects
> mod_disk_cache. mod_disk_cache overwrites the old entry if it is told
> to do so. It's possible that mod_mem_cache should do the same.
Will investigate.
>
> FWIW, your 2 patches are identical. Not sure if that's what you
> intended or not. ;-) -- justin
>
Ooops, the other patch was to set cache->handle to h.
Bill
Re: cvs commit: httpd-2.0/modules/experimental cache_storage.c
cache_util.c mod_cache.c mod_cache.h mod_disk_cache.c mod_mem_cache.c
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On Thursday, September 23, 2004 2:55 PM -0400 Bill Stoddard
<bi...@wstoddard.com> wrote:
> This little gem causes a regression. Because cache->handle is left NULL,
> we never cleanup stale entries in the cache (in cache_save_filter). Once
> CacheMaxExpires hits, the stale entry will never be garbage collected
> from the cache and replaced by a new entry. Two ways to fix this come to
> mind right off hand (neither are optimal i expect):
Well, I don't know about mod_mem_cache, but I don't believe this affects
mod_disk_cache. mod_disk_cache overwrites the old entry if it is told to
do so. It's possible that mod_mem_cache should do the same.
FWIW, your 2 patches are identical. Not sure if that's what you intended
or not. ;-) -- justin
Re: cvs commit: httpd-2.0/modules/experimental cache_storage.c cache_util.c
mod_cache.c mod_cache.h mod_disk_cache.c mod_mem_cache.c
Posted by Bill Stoddard <bi...@wstoddard.com>.
jerenkrantz@apache.org wrote:
> jerenkrantz 2004/09/21 15:56:23
>
> Modified: . CHANGES
> modules/experimental cache_storage.c cache_util.c
> mod_cache.c mod_cache.h mod_disk_cache.c
> mod_mem_cache.c
> Log:
> Fix Expires (freshness) handling in mod_cache.
>
> Previously, if the cached copy was stale, the response would go into an
> indeterminate state. Therefore, the freshness check must be done before we
> 'accept' the response and, if it fails (i.e. stale), we can't allow any side
> effects.
>
> This caused a number of changes to how mod_disk_cache reads its headers as
> ap_scan_script_header_err() purposely has side-effects and that's
> unacceptable. So, factor out only what we need.
>
> Also, remove the broken conditional filter code as you can't reliably alter the
> filter list once the response is started. (Regardless, cache_select_url()
> has the freshness checks now.)
>
> Assist to Sascha Schumann for reporting mod_cache was busted.
>
> Revision Changes Path
> 1.1596 +5 -0 httpd-2.0/CHANGES
>
> Index: CHANGES
> ===================================================================
> RCS file: /home/cvs/httpd-2.0/CHANGES,v
> retrieving revision 1.1595
> retrieving revision 1.1596
> diff -u -u -r1.1595 -r1.1596
> --- CHANGES 21 Sep 2004 13:23:47 -0000 1.1595
> +++ CHANGES 21 Sep 2004 22:56:22 -0000 1.1596
> @@ -2,6 +2,11 @@
>
> [Remove entries to the current 2.0 section below, when backported]
>
> + *) Fix Expires handling in mod_cache. [Justin Erenkrantz]
> +
> + *) Alter mod_expires to run at a different filter priority to allow
> + proper Expires storage by mod_cache. [Justin Erenkrantz]
> +
> *) Fix the global mutex crash when the global mutex is never allocated due
> to disabled/empty caches. [Jess Holle <jessh ptc.com>]
>
>
>
>
> 1.37 +82 -19 httpd-2.0/modules/experimental/cache_storage.c
>
> Index: cache_storage.c
> ===================================================================
> RCS file: /home/cvs/httpd-2.0/modules/experimental/cache_storage.c,v
> retrieving revision 1.36
> retrieving revision 1.37
> diff -u -u -r1.36 -r1.37
> --- cache_storage.c 5 Aug 2004 19:12:34 -0000 1.36
> +++ cache_storage.c 21 Sep 2004 22:56:23 -0000 1.37
> @@ -98,6 +98,57 @@
> return DECLINED;
> }
>
<snip>
> /*
> * select a specific URL entity in the cache
> *
> @@ -118,12 +169,12 @@
> cache_request_rec *cache = (cache_request_rec *)
> ap_get_module_config(r->request_config, &cache_module);
>
> - rv = cache_generate_key(r,r->pool,&key);
> + rv = cache_generate_key(r, r->pool, &key);
> if (rv != APR_SUCCESS) {
> return rv;
> }
> /* go through the cache types till we get a match */
> - h = cache->handle = apr_palloc(r->pool, sizeof(cache_handle_t));
> + h = apr_palloc(r->pool, sizeof(cache_handle_t));
>
This little gem causes a regression. Because cache->handle is left NULL, we never cleanup stale entries in the
cache (in cache_save_filter). Once CacheMaxExpires hits, the stale entry will never be garbage collected from
the cache and replaced by a new entry. Two ways to fix this come to mind right off hand (neither are optimal
i expect):
Patch 1:
Index: cache_storage.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/experimental/cache_storage.c,v
retrieving revision 1.38
diff -u -r1.38 cache_storage.c
--- cache_storage.c 22 Sep 2004 22:25:45 -0000 1.38
+++ cache_storage.c 23 Sep 2004 18:49:17 -0000
@@ -248,6 +248,7 @@
/* Is our cached response fresh enough? */
fresh = ap_cache_check_freshness(h, r);
if (!fresh) {
+ cache->provider->remove_entity(h);
return DECLINED;
}
Patch 2:
Index: cache_storage.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/experimental/cache_storage.c,v
retrieving revision 1.38
diff -u -r1.38 cache_storage.c
--- cache_storage.c 22 Sep 2004 22:25:45 -0000 1.38
+++ cache_storage.c 23 Sep 2004 18:51:10 -0000
@@ -248,6 +248,7 @@
/* Is our cached response fresh enough? */
fresh = ap_cache_check_freshness(h, r);
if (!fresh) {
+ cache->provider->remove_entity(h);
return DECLINED;
}
If no one ventures an opinion by this evening, I'll dig into it and come up with a solution. No time at the
moment.
Bill