httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Plüm, Rüdiger, VF-Group" <ruediger.pl...@vodafone.com>
Subject RE: svn commit: r1021946 - /httpd/httpd/trunk/modules/cache/mod_cache.c
Date Mon, 18 Oct 2010 14:41:31 GMT
 

> -----Original Message-----
> From: Graham Leggett  
> Sent: Montag, 18. Oktober 2010 16:11
> To: dev@httpd.apache.org
> Subject: Re: svn commit: r1021946 - 
> /httpd/httpd/trunk/modules/cache/mod_cache.c
> 
> On 18 Oct 2010, at 8:36 AM, Ruediger Pluem wrote:
> 
> >> If the client specified no-cache, the cache steps down 
> completely,  
> >> and
> >> the client is guaranteed fresh content from the source 
> server (as per
> >> the RFC). The cache will only revalidate if you say max-age=X (or  
> >> other
> >> valid caching tokens).
> >
> > I cannot follow that. The above code means that  
> > cache_check_freshness returns
> > 0 and thus we replace the client supplied conditionals with our own:
> 
> mod_cache used to do this, but not any more as it's an RFC 
> violation.  
> Now, if the client supplies no-cache, this is caught by a function  
> called ap_cache_check_allowed(), which if not allowed, causes no  
> attempt to be made to open or touch an existing cached file. Look  
> further upwards in cache_select(), you'll see the call to  
> ap_cache_check_allowed():

Ok, I missed that. Thanks for the pointer.
But shouldn't we remove the following code from cache_check_freshness then:

    /* This value comes from the client's initial request. */
    cc_req = apr_table_get(r->headers_in, "Cache-Control");
    pragma = apr_table_get(r->headers_in, "Pragma");

    ap_cache_control(r, &cache->control_in, cc_req, pragma, r->headers_in);

    if (cache->control_in.no_cache) {

        if (!conf->ignorecachecontrol) {
            /* Treat as stale, causing revalidation */
            return 0;
        }

        ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
                "Incoming request is asking for a uncached version of "
                "%s, but we have been configured to ignore it and "
                "serve a cached response anyway",
                r->unparsed_uri);
    }



> 
> So if I'm understanding you correctly, the issue is with the 
> different  
> handling of the check for "must-revalidate"?

Yes.

> 
> Hmmm.
> 
> If "must-revalidate" is present in the original cached response, in  
> both cases, we don't serve the stale-cached-content, which is 
> correct  
> according to the definition of must-revalidate.
> 
> This makes the invalidation of the stale response academic from the  
> client's point of view, as this stale response, which contains the  
> "must-revalidate", will never be served to the client while 
> the error  
> persists.
> 
> This is however inconsistent as you've pointed out, and needs to be  
> fixed.
> 
>  From the server's point of view, invalidating the cached 
> entry means  
> that when the server comes back, it will need to serve a 200 
> response  
> from scratch, and if our server is returning 5xx errors this is less  
> than ideal. I think we should remove the remove_url filter in both  
> cases, so that we're consistent, like below.
> 
> Does this make sense?

Yeah, the patch below makes the behaviour consistent and makes sense.
Thanks for your patience and for explaining.

> 
> Index: modules/cache/mod_cache.c
> ===================================================================
> --- modules/cache/mod_cache.c	(revision 1023462)
> +++ modules/cache/mod_cache.c	(working copy)
> @@ -1595,6 +1595,8 @@
>       if (dummy) {
>           cache_request_rec *cache = (cache_request_rec *) dummy;
> 
> +        ap_remove_output_filter(cache->remove_url_filter);
> +
>           if (cache->stale_handle && cache->save_filter
>                   && !cache->stale_handle->cache_obj- 
>  >info.control.must_revalidate
>                   && !cache->stale_handle->cache_obj- 
>  >info.control.proxy_revalidate) {
> @@ -1627,8 +1629,6 @@
>                           "110 Response is stale");
>               }
> 
> -            ap_remove_output_filter(cache->remove_url_filter);
> -
>               cache_run_cache_status(
>                       cache->handle,
>                       r,
> 

Regards

Rüdiger

Mime
View raw message