Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Received-SPF: pass (hermes.apache.org: local policy)
Message-ID: <41531C14.40705@wstoddard.com>
Date: Thu, 23 Sep 2004 14:55:16 -0400
From: Bill Stoddard <bill@wstoddard.com>
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US;
 rv:1.7.3) Gecko/20040910
MIME-Version: 1.0
To: dev@httpd.apache.org
Subject: 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
References: <20040921225623.18345.qmail@minotaur.apache.org>
In-Reply-To: <20040921225623.18345.qmail@minotaur.apache.org>
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit

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