You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by mi...@apache.org on 2010/10/13 00:54:06 UTC
svn commit: r1021946 - /httpd/httpd/trunk/modules/cache/mod_cache.c
Author: minfrin
Date: Tue Oct 12 22:54:06 2010
New Revision: 1021946
URL: http://svn.apache.org/viewvc?rev=1021946&view=rev
Log:
mod_cache: Support stale-on-error behaviour for the mod_proxy case in
addition to the existing case where the error was generated by ourselves.
Modified:
httpd/httpd/trunk/modules/cache/mod_cache.c
Modified: httpd/httpd/trunk/modules/cache/mod_cache.c
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;
+
+ /* morph the current save filter into the out filter, and serve from
+ * cache.
+ */
+ cache->handle = cache->stale_handle;
+ if (r->main) {
+ f->frec = cache_out_subreq_filter_handle;
+ }
+ else {
+ f->frec = cache_out_filter_handle;
+ }
+
+ r->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");
+ }
+
+ cache_run_cache_status(cache->handle, r, r->headers_out, AP_CACHE_HIT,
+ apr_psprintf(r->pool,
+ "cache hit: %d status; stale content returned",
+ r->status));
+
+ /* give someone else the chance to cache the file */
+ cache_remove_lock(conf, cache, f->r, NULL);
+
+ /* pass brigade to our morphed out filter */
+ return ap_pass_brigade(f, in);
+ }
+ }
+
/* read expiry date; if a bad date, then leave it so the client can
* read it
*/
@@ -1534,6 +1589,9 @@ static void cache_insert_error_filter(re
* 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 the error was generated by our server via
+ * ap_die().
*/
apr_pool_userdata_get(&dummy, CACHE_CTX_KEY, r->pool);
if (dummy) {
Re: svn commit: r1021946 - /httpd/httpd/trunk/modules/cache/mod_cache.c
Posted by Graham Leggett <mi...@sharp.fm>.
On 18 Oct 2010, at 4:41 PM, Plüm, Rüdiger, VF-Group wrote:
> 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);
The above still needs to be there, as we use other information from
the request (like max-age) in the freshness check below.
ap_cache_control() has a check to ensure that it only parses the
header once, so calling it a second time isn't a problem.
> 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);
> }
In theory, the above is caught by ap_cache_check_allowed(), so doesn't
do anything, on condition we always call ap_cache_check_allowed()
before running cache_check_freshness().
> Yeah, the patch below makes the behaviour consistent and makes sense.
Will commit it.
I've found some more stuff that can be optimised in
cache_check_freshness(), will look at this in more detail tonight.
Regards,
Graham
--
RE: svn commit: r1021946 - /httpd/httpd/trunk/modules/cache/mod_cache.c
Posted by "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>.
> -----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
Re: svn commit: r1021946 - /httpd/httpd/trunk/modules/cache/mod_cache.c
Posted by Graham Leggett <mi...@sharp.fm>.
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
--
Re: svn commit: r1021946 - /httpd/httpd/trunk/modules/cache/mod_cache.c
Posted by Ruediger Pluem <rp...@apache.org>.
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
Re: svn commit: r1021946 - /httpd/httpd/trunk/modules/cache/mod_cache.c
Posted by Graham Leggett <mi...@sharp.fm>.
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).
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.
>> 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.
Remember that the save_filter isn't triggered at all in the ap_die()
case, so doing this inside save_filter isn't enough.
Regards,
Graham
--
Re: svn commit: r1021946 - /httpd/httpd/trunk/modules/cache/mod_cache.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 10/16/2010 01:01 PM, Graham Leggett wrote:
> On 16 Oct 2010, at 7:22 AM, Ruediger Pluem wrote:
>
>>> It's because if we don't do this, the very first user who sends
>>> "Cache-Control: no-cache" will cause the entry to become invalidated,
>>
>> Hm. Invalidated? We might have no stale entry, so what I am asking
>> about is the
>> case of a fresh 5xx response with no previous stale entry. Why do we
>> remove
>> cache->remove_url_filter in this case?
>
> The url_filter's job is to invalidate and physically remove the existing
> entry. We add the url_filter very early on, and then when we're certain
> we have no need to remove the existing entry, we remove the filter.
>
> The cache->stale_handle value doesn't tell us whether a stale entry is
> present or not. Instead, it tells us that when we tried to open the
> existing entry, and we looked at the headers in the entry, and the
> headers provided by the client, we eventually concluded that in this
> specific case, we should consider the content stale. If the client said
> "Cache-Control: no-cache", we wouldn't have even tried to open the
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);
}
> 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)?
Regards
Rüdiger
Re: svn commit: r1021946 - /httpd/httpd/trunk/modules/cache/mod_cache.c
Posted by Graham Leggett <mi...@sharp.fm>.
On 16 Oct 2010, at 7:22 AM, Ruediger Pluem wrote:
>> It's because if we don't do this, the very first user who sends
>> "Cache-Control: no-cache" will cause the entry to become invalidated,
>
> Hm. Invalidated? We might have no stale entry, so what I am asking
> about is the
> case of a fresh 5xx response with no previous stale entry. Why do we
> remove
> cache->remove_url_filter in this case?
The url_filter's job is to invalidate and physically remove the
existing entry. We add the url_filter very early on, and then when
we're certain we have no need to remove the existing entry, we remove
the filter.
The cache->stale_handle value doesn't tell us whether a stale entry is
present or not. Instead, it tells us that when we tried to open the
existing entry, and we looked at the headers in the entry, and the
headers provided by the client, we eventually concluded that in this
specific case, we should consider the content stale. If the client
said "Cache-Control: no-cache", we wouldn't have even tried to open
the existing file at all, and cache->stale_handle would always be NULL.
Regards,
Graham
--
Re: svn commit: r1021946 - /httpd/httpd/trunk/modules/cache/mod_cache.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 10/15/2010 06:55 PM, Graham Leggett wrote:
> On 14 Oct 2010, at 8:58 PM, Ruediger Pluem wrote:
>
>>> + /* 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);
>>
>> Why doing ap_remove_output_filter that early?
>> In the error filter case (when the error origins from ap_die()) this
>> is done withing
>> the "if (cache->stale_handle && ..." block.
>> Is there any reason why we should not remove a cached entity when we
>> have no
>> stale_handle and the backend created an error code as response code?
>
> It's because if we don't do this, the very first user who sends
> "Cache-Control: no-cache" will cause the entry to become invalidated,
Hm. Invalidated? We might have no stale entry, so what I am asking about is the
case of a fresh 5xx response with no previous stale entry. Why do we remove
cache->remove_url_filter in this case?
> and we go from having a stale entry, to having no entry at all, and
> everyone gets a 5xx from that point on.
>
> This isn't to say that we ignore the no-cache, the user that requested
> no-cache will get no-cache behaviour and the 5xx response, but this
> no-cache doesn't blow away the stale data for other people.
>
> If we want to add an additional directive to offer control over this
> it's fine, but I'm hoping that this is reasonable behaviour given that
> the admin has asked for stale-on-error behaviour for 5xx responses.
Ok, but why doing it differently in the ap_die case?
Regards
Rüdiger
Re: svn commit: r1021946 - /httpd/httpd/trunk/modules/cache/mod_cache.c
Posted by Graham Leggett <mi...@sharp.fm>.
On 14 Oct 2010, at 8:58 PM, Ruediger Pluem wrote:
>> + /* 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);
>
> Why doing ap_remove_output_filter that early?
> In the error filter case (when the error origins from ap_die()) this
> is done withing
> the "if (cache->stale_handle && ..." block.
> Is there any reason why we should not remove a cached entity when we
> have no
> stale_handle and the backend created an error code as response code?
It's because if we don't do this, the very first user who sends "Cache-
Control: no-cache" will cause the entry to become invalidated, and we
go from having a stale entry, to having no entry at all, and everyone
gets a 5xx from that point on.
This isn't to say that we ignore the no-cache, the user that requested
no-cache will get no-cache behaviour and the 5xx response, but this no-
cache doesn't blow away the stale data for other people.
If we want to add an additional directive to offer control over this
it's fine, but I'm hoping that this is reasonable behaviour given that
the admin has asked for stale-on-error behaviour for 5xx responses.
Regards,
Graham
--
Re: svn commit: r1021946 - /httpd/httpd/trunk/modules/cache/mod_cache.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 10/13/2010 12:54 AM, minfrin@apache.org wrote:
> Author: minfrin
> Date: Tue Oct 12 22:54:06 2010
> New Revision: 1021946
>
> URL: http://svn.apache.org/viewvc?rev=1021946&view=rev
> Log:
> mod_cache: Support stale-on-error behaviour for the mod_proxy case in
> addition to the existing case where the error was generated by ourselves.
>
> Modified:
> httpd/httpd/trunk/modules/cache/mod_cache.c
>
> Modified: httpd/httpd/trunk/modules/cache/mod_cache.c
> 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);
Why doing ap_remove_output_filter that early?
In the error filter case (when the error origins from ap_die()) this is done withing
the "if (cache->stale_handle && ..." block.
Is there any reason why we should not remove a cached entity when we have no
stale_handle and the backend created an error code as response code?
Regards
Rüdiger