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
Justin Erenkrantz wrote: --On Thursday, September 23, 2004 8:17 PM -0400 Bill Stoddard [EMAIL PROTECTED] 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
[EMAIL PROTECTED] wrote: 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.) How does cache handle responses that are stale? Are these objects revalidated (ie If-None-Match) or are the objects discarded and fetched again from scratch? Regards, Graham --
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
Graham Leggett wrote: [EMAIL PROTECTED] wrote: 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.) How does cache handle responses that are stale? Are these objects revalidated (ie If-None-Match) or are the objects discarded and fetched again from scratch? Stale objects are discarded and fetched from the origin server. 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
Bill Stoddard wrote: Graham Leggett wrote: [EMAIL PROTECTED] wrote: 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.) How does cache handle responses that are stale? Are these objects revalidated (ie If-None-Match) or are the objects discarded and fetched again from scratch? Stale objects are discarded and fetched from the origin server. So we're missing all the code to handle RFC 2616 section 13.3. We're not violating the RFC but our cache is sure not as efficient as possible. 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
Bill Stoddard wrote: Are these objects revalidated (ie If-None-Match) or are the objects discarded and fetched again from scratch? Stale objects are discarded and fetched from the origin server. Hmmm... that's quite a performance hit - suddenly what should be 304's with no bodies are suddenly 200's with bodies coming from the backend (or downstream). What is the issue with the CACHE_CONDITIONAL filter? Regards, Graham -- smime.p7s Description: S/MIME Cryptographic Signature
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
Bill Stoddard wrote: Stale objects are discarded and fetched from the origin server. So we're missing all the code to handle RFC 2616 section 13.3. We're not violating the RFC but our cache is sure not as efficient as possible. What the CACHE_CONDITIONAL filter did was say if the cached response is stale, change this request to a conditional request by adding If-None-Match. If I get a 304 back, then serve my cached copy, it is fresh. If I get a 200 back (or something else), throw away my cached copy and serve the new 200 content instead. If the CACHE_CONDITIONAL filter is the wrong way to go about this, what is the correct way to go about this? So far simplifying the code is nice, but if simplifying means ignore part of the RFC, then we're moving backwards. :( Regards, Graham -- smime.p7s Description: S/MIME Cryptographic Signature
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
--On Friday, September 24, 2004 11:39 AM -0400 Bill Stoddard [EMAIL PROTECTED] wrote: So we're missing all the code to handle RFC 2616 section 13.3. We're not violating the RFC but our cache is sure not as efficient as possible. Put it this way: we were violating the RFC because our Expires handling was completely broken and our handling of RFC 2616 13.3 was also broken too. As I indicated in the commit logs, the problem with cache_conditional_filter is that it's not possible to alter the filter chain like it was doing. The cache_conditional_filter was trying to serve or save the response: but neither was possible at that stage. Therefore, it didn't do anything at all and ended up returning bogus responses to the client. If we want to add that functionality, we need to rethink how to handle caching-proxied-conditional requests. My preliminary thought is: 1) If cache_select_url() determines the cached response is out-of-date, it can then add the 'If-None-Match' header (and any other headers as required) to the request. 2) We'll unconditionally add cache_save_filter because we don't have a usable cached entity. 3) cache_save_filter can then determine if we receive a 304 back by looking at r-status. 4) If we receive a 304, then cache_save_filter can update the cached headers and leave the body alone. [We'd have to add a lookup function at this point so we can identify which provider needs to be updated.] 5) If we did get a 304 *and* the original request was conditional, cache_save_filter can then remove itself (after updating headers) and get out of the way. 6) If we did get a 304 *and* the original request was *not* conditional (somehow we'd have to signal that, not sure how at the moment), then cache_save_filter can serve the cached body by calling recall_body and then eating all saved output. 7) If we didn't get a 304 from the origin server, cache_save_filter continues normally. What do you think? -- 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
Graham Leggett wrote: Bill Stoddard wrote: Stale objects are discarded and fetched from the origin server. So we're missing all the code to handle RFC 2616 section 13.3. We're not violating the RFC but our cache is sure not as efficient as possible. What the CACHE_CONDITIONAL filter did was say if the cached response is stale, change this request to a conditional request by adding If-None-Match. If I get a 304 back, then serve my cached copy, it is fresh. If I get a 200 back (or something else), throw away my cached copy and serve the new 200 content instead. If the CACHE_CONDITIONAL filter is the wrong way to go about this, what is the correct way to go about this? So far simplifying the code is nice, but if simplifying means ignore part of the RFC, then we're moving backwards. :( Regards, Graham -- Yea, I agree we lost some good function in the CACHE_CONDITIONAL filter. Here is Justin's explanation: 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.) Looking at adding the function back in. 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
As I recall the problem was that, at the time the CACHE_CONDITIONAL code actually ran and tried to install the correct filter (in or out), it was unable to alter the filter chain in a way that would allow the filter to run. It was able to successfully add the filter, but it apparently was added before the current one and never got run. It simply silently failed to return the content. There was actually a bug in the code that resulted in the cache_conditional portion of things being avoided that I fixed which then resulted in finding this out. I put a comment in the code and broke it again so it wouldn't try to use the cache_conditional logic. The issue to resolve is how to make the choice, late in the game, of which path to follow and add the right filter *after* the current point in the chain. This may require a single merged filter that has both paths included and uses the cache_conditional logic for the top level code-path choice (thus avoiding the whole filter adding issue). Paul J. Reder Bill Stoddard wrote: Graham Leggett wrote: Bill Stoddard wrote: Stale objects are discarded and fetched from the origin server. So we're missing all the code to handle RFC 2616 section 13.3. We're not violating the RFC but our cache is sure not as efficient as possible. What the CACHE_CONDITIONAL filter did was say if the cached response is stale, change this request to a conditional request by adding If-None-Match. If I get a 304 back, then serve my cached copy, it is fresh. If I get a 200 back (or something else), throw away my cached copy and serve the new 200 content instead. If the CACHE_CONDITIONAL filter is the wrong way to go about this, what is the correct way to go about this? So far simplifying the code is nice, but if simplifying means ignore part of the RFC, then we're moving backwards. :( Regards, Graham -- Yea, I agree we lost some good function in the CACHE_CONDITIONAL filter. Here is Justin's explanation: 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.) Looking at adding the function back in. Bill -- Paul J. Reder --- The strength of the Constitution lies entirely in the determination of each citizen to defend it. Only if every single citizen feels duty bound to do his share in this defense are the constitutional rights secure. -- Albert Einstein
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
[EMAIL PROTECTED] wrote:
jerenkrantz2004/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 ChangesPath
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 - 1.1595
+++ CHANGES 21 Sep 2004 22:56:22 - 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 -19httpd-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 - 1.36
+++ cache_storage.c 21 Sep 2004 22:56:23 - 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 - 1.38
+++ cache_storage.c 23 Sep 2004 18:49:17 -
@@ -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 - 1.38
+++ cache_storage.c 23 Sep 2004 18:51:10 -
@@ -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
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
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.c22 Sep 2004 22:25:45 -1.38
+++ cache_storage.c23 Sep 2004 18:49:17 -
@@ -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.c22 Sep 2004 22:25:45 -1.38
+++ cache_storage.c23 Sep 2004 18:51:10 -
@@ -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 - 1.38
+++ cache_storage.c 23 Sep 2004 19:06:34 -
@@ -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
--On Thursday, September 23, 2004 2:55 PM -0400 Bill Stoddard [EMAIL PROTECTED] 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
Justin Erenkrantz wrote: --On Thursday, September 23, 2004 2:55 PM -0400 Bill Stoddard [EMAIL PROTECTED] 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
--On Thursday, September 23, 2004 8:17 PM -0400 Bill Stoddard [EMAIL PROTECTED] 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
