httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Graham Leggett <minf...@sharp.fm>
Subject Re: svn commit: r1021946 - /httpd/httpd/trunk/modules/cache/mod_cache.c
Date Mon, 18 Oct 2010 14:11:17 GMT
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():

int cache_select(cache_request_rec *cache, request_rec *r)
{
     cache_provider_list *list;
     apr_status_t rv;
     cache_handle_t *h;

     if (!cache) {
         /* This should never happen */
         ap_log_rerror(APLOG_MARK, APLOG_ERR, APR_EGENERAL, r,
                 "cache: No cache request information available for key"
                 " generation");
         return DECLINED;
     }

     if (!cache->key) {
         rv = cache_generate_key(r, r->pool, &cache->key);
         if (rv != APR_SUCCESS) {
             return DECLINED;
         }
     }

     if (!ap_cache_check_allowed(cache, r)) {
         return DECLINED;
     }

     /* go through the cache types till we get a match */
     h = apr_palloc(r->pool, sizeof(cache_handle_t));

     list = cache->providers;

     while (list) {
         switch ((rv = list->provider->open_entity(h, r, cache->key))) {

>> We do it the same way in both cases, the remove_url filter is  
>> removed in
>> the ap_die() case on line  1630, and in the mod_proxy case inside
>> save_filter on line 791.
>
> Sorry for being a PITA, but IMHO we do not:

If a bug exists, we must find it :)

> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cache/mod_cache.c?rev=1021946&r1=1021945&r2=1021946&view=diff
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- httpd/httpd/trunk/modules/cache/mod_cache.c (original)
> +++ httpd/httpd/trunk/modules/cache/mod_cache.c Tue Oct 12 22:54:06  
> 2010
> @@ -779,6 +779,61 @@ static int cache_save_filter(ap_filter_t
>
>     dconf = ap_get_module_config(r->per_dir_config, &cache_module);
>
> +    /* RFC2616 13.8 Errors or Incomplete Response Cache Behavior:
> +     * If a cache receives a 5xx response while attempting to  
> revalidate an
> +     * entry, it MAY either forward this response to the requesting  
> client,
> +     * or act as if the server failed to respond. In the latter  
> case, it MAY
> +     * return a previously received response unless the cached entry
> +     * includes the "must-revalidate" cache-control directive (see  
> section
> +     * 14.9).
> +     *
> +     * This covers the case where an error was generated behind us,  
> for example
> +     * by a backend server via mod_proxy.
> +     */
> +    if (dconf->stale_on_error && r->status >=  
> HTTP_INTERNAL_SERVER_ERROR) {
> +
> +        ap_remove_output_filter(cache->remove_url_filter);
> +
> +        if (cache->stale_handle && !ap_cache_liststr(
> +                NULL,
> +                apr_table_get(cache->stale_handle->resp_hdrs,  
> "Cache-Control"),
> +                "must-revalidate", NULL)) {
> +            const char *warn_head;
>
>
> Modified: httpd/httpd/trunk/modules/cache/mod_cache.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cache/mod_cache.c?rev=1021546&r1=1021545&r2=1021546&view=diff
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- httpd/httpd/trunk/modules/cache/mod_cache.c (original)
> +++ httpd/httpd/trunk/modules/cache/mod_cache.c Mon Oct 11 23:32:56  
> 2010
>
> +    /* RFC2616 13.8 Errors or Incomplete Response Cache Behavior:
> +     * If a cache receives a 5xx response while attempting to  
> revalidate an
> +     * entry, it MAY either forward this response to the requesting  
> client,
> +     * or act as if the server failed to respond. In the latter  
> case, it MAY
> +     * return a previously received response unless the cached entry
> +     * includes the "must-revalidate" cache-control directive (see  
> section
> +     * 14.9).
> +     */
> +    apr_pool_userdata_get(&dummy, CACHE_CTX_KEY, r->pool);
> +    if (dummy) {
> +        cache_request_rec *cache = (cache_request_rec *) dummy;
> +
> +        if (cache->stale_handle && cache->save_filter && ! 
> ap_cache_liststr(
> +                NULL, apr_table_get(cache->stale_handle->resp_hdrs,
> +                        "Cache-Control"), "must-revalidate", NULL)) {
> +            const char *warn_head;
> +
> +            /* morph the current save filter into the out filter,  
> and serve from
> +             * cache.
> +             */
> +            cache->handle = cache->stale_handle;
> +            if (r->main) {
> +                cache->save_filter->frec =  
> cache_out_subreq_filter_handle;
> +            }
> +            else {
> +                cache->save_filter->frec = cache_out_filter_handle;
> +            }
> +
> +            r->output_filters = cache->save_filter;
> +
> +            r->err_headers_out = cache->stale_handle->resp_hdrs;
> +
> +            /* add a stale warning */
> +            warn_head = apr_table_get(r->err_headers_out, "Warning");
> +            if ((warn_head == NULL) || ((warn_head != NULL)
> +                    && (ap_strstr_c(warn_head, "110") == NULL))) {
> +                apr_table_mergen(r->err_headers_out, "Warning",
> +                        "110 Response is stale");
> +            }
> +
> +            ap_remove_output_filter(cache->remove_url_filter);
> +
> +
>
> Please compare the code. In the mod_proxy case we call
>
> ap_remove_output_filter(cache->remove_url_filter);
>
> if (dconf->stale_on_error && r->status >= HTTP_INTERNAL_SERVER_ERROR)
>
> In the ap_die case we require
>
> +        if (cache->stale_handle && cache->save_filter && ! 
> ap_cache_liststr(
> +                NULL, apr_table_get(cache->stale_handle->resp_hdrs,
> +                        "Cache-Control"), "must-revalidate", NULL)) {
> +
>
> These additional conditions are required in the second if in the  
> mod_proxy case.

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

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?

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,
Graham
--


Mime
View raw message