httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
Subject Re: svn commit: r1021946 - /httpd/httpd/trunk/modules/cache/mod_cache.c
Date Mon, 18 Oct 2010 06:36:08 GMT


On 10/17/2010 05:57 PM, Graham Leggett wrote:
> On 17 Oct 2010, at 2:29 PM, Ruediger Pluem wrote:
> 
>> Are you sure? If the client said Cache-Control: no-cache, we IMHO treat
>> the cached entry as stale and revalidate:
>>
>> From cache_util.c:
>>
>>    /* 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");
>>
>>
>>    if (ap_cache_liststr(NULL, pragma, "no-cache", NULL)
>>        || ap_cache_liststr(NULL, cc_req, "no-cache", NULL)) {
>>
>>        if (!conf->ignorecachecontrol) {
>>            /* Treat as stale, causing revalidation */
>>            return 0;
>>        }
>>
>>        ap_log_error(APLOG_MARK, APLOG_INFO, 0, r->server,
>>                     "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);
>>    }
> 
> 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:

            /* Is our cached response fresh enough? */
            fresh = cache_check_freshness(h, cache, r);
            if (!fresh) {
                const char *etag, *lastmod;

                /* Cache-Control: only-if-cached and revalidation required, try
                 * the next provider
                 */
                if (cache->control_in.only_if_cached) {
                    /* try again with next cache type */
                    list = list->next;
                    continue;
                }

                /* set aside the stale entry for accessing later */
                cache->stale_headers = apr_table_copy(r->pool,
                        r->headers_in);
                cache->stale_handle = h;

                ap_log_rerror(APLOG_MARK, APLOG_DEBUG, APR_SUCCESS, r,
                        "Cached response for %s isn't fresh.  Adding/replacing "
                        "conditional request headers.", r->uri);

                /* We can only revalidate with our own conditionals: remove the
                 * conditions from the original request.
                 */
                apr_table_unset(r->headers_in, "If-Match");
                apr_table_unset(r->headers_in, "If-Modified-Since");
                apr_table_unset(r->headers_in, "If-None-Match");
                apr_table_unset(r->headers_in, "If-Range");
                apr_table_unset(r->headers_in, "If-Unmodified-Since");

                etag = apr_table_get(h->resp_hdrs, "ETag");
                lastmod = apr_table_get(h->resp_hdrs, "Last-Modified");

> 
> What we do differently, is that instead of invalidating the existing
> content with our 5xx response to the no-cache request, we leave it alone
> completely, so that the next request, where the client hasn't asked for
> no-cache, can be served the stale content, instead of an error because
> the stale content had been blown away by the previous no-cache request.

I understand this purpose and I do not question it. It makes a lot of sense.
> 
>>> existing file at all, and cache->stale_handle would always be NULL.
>>
>> Even then remains the question: Why doing it differently in both
>> case (ap_die, mod_proxy)?
> 
> 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:

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.

Regards

RĂ¼diger

Mime
View raw message