mod_cache: 304 on HEAD (bug 41230)

Any progress on 
http://issues.apache.org/bugzilla/show_bug.cgi?id=41230 ??

It seems that stuff like the Ubuntu automatic mirror checker
is 
hitting this, at least the symptoms match perfectly.

A quick perusal of mod_cache.c and cache_storage.c indeed
shows that 
cache_select() tries to revalidate even though it's a head
request, 
and that fits badly with the behaviour in
cache_save_filter().

Would the correct fix be to check for r->header_only in 
cache_select(), or are there even more funky stuff going on?
You 
don't want the cached object to be removed just because you
got a 
HEAD request when it really isn't stale but just in need of

revalidation. Ideally the HEAD request would cause the
object to be 
revalidated if possible, but we can live with head requests
just 
doing fallback without touching the cache.

I can whip up a patch for it, but I suspect that you guys
are more 
clued on the deep magic involved 

In any case, we'd really appreciate a fix, and we'd really
like to see 
it go into httpd 2.2.5 (even though we'll most likely apply
it to our 
own build as soon as the fix is available). It's a rather
obscure bug 
that we assume is the reason for a fair share of "it
didn't work for a 
while"-bugreports when people are running apt-get
update and whatnot.

/Nikke
-- 
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
-=-=-=-=-=-=-=-
  Niklas Edmundsson, Admin  {acc,hpc2n}.umu.se      | 
   nikkeacc.umu.se
------------------------------------------------------------
---------------
  Sam: "Beer, Norm?". Norm: "Have I gotten
that predictable?  Good."
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=-=-=-=-=-=

On Wed, 11 Apr 2007, Niklas Edmundsson wrote:

> Would the correct fix be to check for r->header_only
in cache_select(), or 
> are there even more funky stuff going on? You don't
want the cached object to 
> be removed just because you got a HEAD request when it
really isn't stale but 
> just in need of revalidation. Ideally the HEAD request
would cause the object 
> to be revalidated if possible, but we can live with
head requests just doing 
> fallback without touching the cache.
>
> I can whip up a patch for it, but I suspect that you
guys are more clued on 
> the deep magic involved 

Looking a bit further, I think that something like this
would actually 
be enough:
-----------8<--------------
--- mod_cache.c.orig	2007-04-11 13:29:14.000000000 +0200
+++ mod_cache.c	2007-04-11 14:06:29.000000000 +0200
 -456,7
+456,7  static int cache_save_filter(ap_filter_t
           */
          reason = "No Last-Modified, Etag, or Expires
headers";
      }
-    else if (r->header_only) {
+    else if (r->header_only &&
!cache->stale_handle) {
          /* HEAD requests */
          reason = "HTTP HEAD request";
      }
 -589,11
+589,12  static int cache_save_filter(ap_filter_t
             
cache->provider->remove_entity(cache->stale_handle)
;
              /* Treat the request as if it wasn't
conditional. */
              cache->stale_handle = NULL;
+            rv = !OK;
          }
      }

-    /* no cache handle, create a new entity */
-    if (!cache->handle) {
+    /* no cache handle, create a new entity only for
non-HEAD request */
+    if (!cache->handle && !r->header_only) {
          rv = cache_create_entity(r, size);
          info = apr_pcalloc(r->pool,
sizeof(cache_info));
          /* We only set info->status upon the initial
creation. */
-----------8<--------------

If I have understood things right this would:
- Accept revalidations even though it's a HEAD if the object
wasn't
   stale.
- Bail out if the object is stale and it's a HEAD.

I haven't tried it yet though, I'm just trying to get a
grasp of 
things. I have no clue on whether other things would break
due to the 
fact that it's revalidated based on a HEAD instead of a GET,
for 
example.

/Nikke
-- 
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
-=-=-=-=-=-=-=-
  Niklas Edmundsson, Admin  {acc,hpc2n}.umu.se      | 
   nikkeacc.umu.se
------------------------------------------------------------
---------------
  I am Mr. T of Borg. I pity da fool that resists me.
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=-=-=-=-=-=

On Wed, 11 Apr 2007, Niklas Edmundsson wrote:

> Looking a bit further, I think that something like this
would actually be 
> enough:
<snip, included as an attachment>

I have now tested this patch, and it seems to solve the
problem. This 
is on httpd-2.2.4 + patch for PR41475 + our mod_disk_cache
patches.

Without the patch a HEAD on a cached expired object that
isn't 
modified will unconditionally return 304 and furthermore
cause the 
cached object to be deleted. We believe that this is the
explanation 
to why it has been so hard to track down this bug - it only
bites one 
user and that user usually has no clue on what happens, and
even if we 
try to reproduce it immediately afterwards it won't
trigger.

With the patch stuff works like expected:
- A HEAD on a cached expired object that isn't modified will
update
   the cache header and return the proper return code, it
follows the
   same code path as other requests on expired unmodified
objects.
- A HEAD on a cached expired object that IS modified will
remove the
   object from cache and then decline the opportunity to
cache the
   object.

I request that this is reviewed, commited and proposed for
backport to 
httpd 2.2.5.


/Nikke
-- 
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
-=-=-=-=-=-=-=-
  Niklas Edmundsson, Admin  {acc,hpc2n}.umu.se      | 
   nikkeacc.umu.se
------------------------------------------------------------
---------------
  "The pain is bad enough. Don't go poetic on me."
- Madeline
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=-=-=-=-=-=

On 04/13/2007 05:31 PM, Niklas Edmundsson wrote:
> On Wed, 11 Apr 2007, Niklas Edmundsson wrote:
> 
>> Looking a bit further, I think that something like
this would actually
>> be enough:
> 
> <snip, included as an attachment>
> 
> I have now tested this patch, and it seems to solve the
problem. This is
> on httpd-2.2.4 + patch for PR41475 + our mod_disk_cache
patches.
> 
> Without the patch a HEAD on a cached expired object
that isn't modified
> will unconditionally return 304 and furthermore cause
the cached object
> to be deleted. We believe that this is the explanation
to why it has
> been so hard to track down this bug - it only bites one
user and that
> user usually has no clue on what happens, and even if
we try to
> reproduce it immediately afterwards it won't trigger.
> 
> With the patch stuff works like expected:
> - A HEAD on a cached expired object that isn't modified
will update
>   the cache header and return the proper return code,
it follows the
>   same code path as other requests on expired
unmodified objects.
> - A HEAD on a cached expired object that IS modified
will remove the
>   object from cache and then decline the opportunity to
cache the
>   object.

Are you really sure that it gets deleted?
cache->provider->remove_entity does
not really remove the object from the cache. Only
cache->provider->remove_url
does this.
I consider the CACHE_SAVE filter already as hard to read
(not your fault by any means),
but from my point of view your patch does increase this
(specificly I think about
the rv = !OK line. I know that a similar trick is done some
lines above, but I don't
like that one either).
Looking at the problem I noticed a related problem already
mentioned in a FIXME comment:
It can happen that the headers of a 304 response from the
backend cause the response to be
uncacheable (e.g. Cache-Control: no-store).
Currently this leads to a wrong response (304) if the
original request was unconditional
(just like in your HEAD case and your HEAD case will also
fail here, even after your patch).
My first question in this situation is: What is the correct
thing to do here?
Generate the response from the cache (of course with the
updated headers from the 304
backend response) and delete the cache entry afterwards?



Regards

Rüdiger

MċN 2007-04-16 KLOCKAN 22:58 +0200 SKREV RUEDIGER PLUEM:

> MY FIRST QUESTION IN THIS SITUATION IS: WHAT IS THE
CORRECT THING TO DO HERE?
> GENERATE THE RESPONSE FROM THE CACHE (OF COURSE WITH
THE UPDATED HEADERS FROM THE 304
> BACKEND RESPONSE) AND DELETE THE CACHE ENTRY
AFTERWARDS?

MY UNDERSTANDING (REGARDING NO-STORE AND CACHE UPDATES FROM
304
RESPONSES):

THE RESPONSE YOU SEND IF THE CLIENT REQUEST WAS A
UNCONDITIONAL SHOULD
BE THE MERGED RESPONSE OF THE OLD RESPONSE AND ENTITY
HEADERS FROM THE
304. 

BUT YOU DO NOT NEED TO DELETE THE ALREADY CACHED RESPONSE
WITHOUT
NO-STORE IF YOU DO NOT WANT TO (CHANGE OF CC IS NOT AN
INVALIDATION
CRITERIA), BUT YOU MUST NOT STORE THE UPDATED HEADERS FROM A
NO-STORE
SESSION (NO-STORE ON EITHER RESPONSE OR REQUEST).

REGARDS
HENRIK

On Mon, 16 Apr 2007, Ruediger Pluem wrote:

>> I have now tested this patch, and it seems to solve
the problem. This is
>> on httpd-2.2.4 + patch for PR41475 + our
mod_disk_cache patches.
>>
>> Without the patch a HEAD on a cached expired object
that isn't modified
>> will unconditionally return 304 and furthermore
cause the cached object
>> to be deleted. We believe that this is the
explanation to why it has
>> been so hard to track down this bug - it only bites
one user and that
>> user usually has no clue on what happens, and even
if we try to
>> reproduce it immediately afterwards it won't
trigger.
>>
>> With the patch stuff works like expected:
>> - A HEAD on a cached expired object that isn't
modified will update
>>   the cache header and return the proper return
code, it follows the
>>   same code path as other requests on expired
unmodified objects.
>> - A HEAD on a cached expired object that IS
modified will remove the
>>   object from cache and then decline the
opportunity to cache the
>>   object.
>
> Are you really sure that it gets deleted?
cache->provider->remove_entity does
> not really remove the object from the cache. Only
cache->provider->remove_url
> does this.

Yes, but the CACHE_REMOVE_URL filter will remove it, right?
It removes 
the CACHE_REMOVE_URL filter only after it has decided that
it's 
actually caching the response so it will bite in that case.

> I consider the CACHE_SAVE filter already as hard to
read (not your 
> fault by any means), but from my point of view your
patch does 
> increase this (specificly I think about the rv = !OK
line. I know 
> that a similar trick is done some lines above, but I
don't like that 
> one either).

I also found rv=!OK ugly, but I just followed the
established style to 
create a minimal patch without extra fuzz. Feel free to
clean stuff up 
to improve readability, as long as the bug gets fixed I'm
happy 


/Nikke
-- 
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
-=-=-=-=-=-=-=-
  Niklas Edmundsson, Admin  {acc,hpc2n}.umu.se      | 
   nikkeacc.umu.se
------------------------------------------------------------
---------------
  Push any key. Then push the "any other" key.
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=-=-=-=-=-=

On 04/16/2007 10:58 PM, Ruediger Pluem wrote:

> Looking at the problem I noticed a related problem
already mentioned in a FIXME comment:
> It can happen that the headers of a 304 response from
the backend cause the response to be
> uncacheable (e.g. Cache-Control: no-store).
> Currently this leads to a wrong response (304) if the
original request was unconditional
> (just like in your HEAD case and your HEAD case will
also fail here, even after your patch).

The following patch should fix this. Any comments
(especially is it RFC compliant to serve
the content from the cache in this case as Henrik said in
http://mail-archives.
apache.org/mod_mbox/httpd-dev/200704.mbox/%3c1176759373.2553
7.30.camelhenriknordstrom.net%3e
?)

Regards

Rüdiger

Index: modules/cache/mod_cache.c
============================================================
=======
--- modules/cache/mod_cache.c	(Revision 539392)
+++ modules/cache/mod_cache.c	(Arbeitskopie)
 -318,6
+318,7 
 static int cache_save_filter(ap_filter_t *f,
apr_bucket_brigade *in)
 {
     int rv = !OK;
+    int store_headers = 1;
     int date_in_errhdr = 0;
     request_rec *r = f->r;
     cache_request_rec *cache;
 -486,11
+487,6 
          * indicating do not cache, or stop now if you are
          * trying to cache it.
          */
-        /* FIXME: The Cache-Control: no-store could have
come in on a 304,
-         * FIXME: while the original request wasn't
conditional.  IOW, we
-         * FIXME:  made the the request conditional earlier
to revalidate
-         * FIXME: our cached response.
-         */
         reason = "Cache-Control: no-store
present";
     }
     else if (!conf->store_private &&
 -499,7
+495,6 
          * this object is marked for this user's eyes only.
Behave
          * as a tunnel.
          */
-        /* FIXME: See above (no-store) */
         reason = "Cache-Control: private
present";
     }
     else if (apr_table_get(r->headers_in,
"Authorization") != NULL
 -526,15
+521,33 
     }

     if (reason) {
-        ap_log_error(APLOG_MARK, APLOG_DEBUG, 0,
r->server,
-                     "cache: %s not cached. Reason:
%s", r->unparsed_uri,
-                     reason);
+        /*
+         * It may be possible that
+         * - we have a stale entity in the cache
+         * - the original request was unconditional
+         * - the conditions added by the cache caused the
origin server to
+         *   respond with a 304
+         * - for some reason we are not allowed to cache
the response by the
+         *   origin server, e.g. Cache-Control: no-store /
+         *   Cache-Control: private
+         * In this case we should serve the client from the
cache, but should
+         * NOT update the headers of the cached entity
(only sent the updated
+         * headers to the client).
+         */
+        if ((r->status == HTTP_NOT_MODIFIED) &&
cache->stale_handle) {
+            store_headers = 0;
+        }
+        else {
+            ap_log_error(APLOG_MARK, APLOG_DEBUG, 0,
r->server,
+                         "cache: %s not cached.
Reason: %s", r->unparsed_uri,
+                         reason);

-        /* remove this filter from the chain */
-        ap_remove_output_filter(f);
+            /* remove this filter from the chain */
+            ap_remove_output_filter(f);

-        /* ship the data up the stack */
-        return ap_pass_brigade(f->next, in);
+            /* ship the data up the stack */
+            return ap_pass_brigade(f->next, in);
+        }
     }

     /* Make it so that we don't execute this path again.
*/
 -782,14
+795,26 
         ap_cache_accept_headers(cache->handle, r, 1);
     }

-    /* Write away header information to cache. It is
possible that we are
-     * trying to update headers for an entity which has
already been cached.
-     *
-     * This may fail, due to an unwritable cache area. E.g.
filesystem full,
-     * permissions problems or a read-only (re)mount. This
must be handled
-     * later.
+    /*
+     * Check if we are allowed to save / update the headers
of the cached
+     * entity. See comments above why this might not me the
case. If we
+     * are not allowed to save / update the headers of the
cached entity we
+     * regard this operation as successful.
      */
-    rv =
cache->provider->store_headers(cache->handle, r,
info);
+    if (store_headers) {
+        /*
+         * Write away header information to cache. It is
possible that we are
+         * trying to update headers for an entity which has
already been cached.
+         *
+         * This may fail, due to an unwritable cache area.
E.g. filesystem full,
+         * permissions problems or a read-only (re)mount.
This must be handled
+         * later.
+         */
+        rv =
cache->provider->store_headers(cache->handle, r,
info);
+    }
+    else {
+        rv = APR_SUCCESS;
+    }

     /* Did we just update the cached headers on a
revalidated response?
      *