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