httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sander Striker <stri...@apache.org>
Subject Re: mod_cache
Date Sun, 06 Mar 2005 12:54:19 GMT
Justin Erenkrantz wrote:
> --On Friday, March 4, 2005 11:55 PM +0100 Sander Striker 

[...]
>> What happens if the 'Cache-Control: no-store' header came in with a
>> 304 Not Modified and the original request wasn't conditional?
>> If I read the spec correctly a 304 can carry a Cache-Control header,
>> if it has a different value since a previous 200 (or 304).
> 
> 
> Hmm.  Good point.  What a goofy corner case though.  I won't lose much 
> sleep if we don't fix this.  Care to add a FIXME?  =)

Sure thing.  Done (r156304).

>> I haven't tracked cache->in_checked fully, so I wonder if it is
>> possible that this is set on a validating request?  That would
>> cause the cache not being updated, which is what I am trying to
>> track down FWIW. [...]

> Yes, it would be set.  in_checked is set after we know that we want to 
> save the response (i.e. all of the cacheability checks passed 
> successfully). However, note that if we are 'blocking' the response 
> (i.e. we revalidated the response succesfully with a 304), we set 
> block_response which is checked right before in_checked.

Ah, the coin is dropping here.  Since it is a filter, cache save can
be called multiple times in succession, with different brigades.  Makes
total sense.
 
[...]
>> AIUI, we can cache "302 Found" (HTTP_MOVED_TEMPORARILY) when it has an
>> Expires or Cache-Control indicating that the request can be cached.
> 
> Fair enough.  Feel free to add it, if you like.

Well, I'm first going to check if we are doing the right thing with
cached 301s (I saw some wonkiness earlier, but need to reverify).  If
that is all good, I'll add 302 support.

[...]
>>         /* Were we initially a conditional request? */
>>         if (ap_cache_request_is_conditional(cache->stale_headers)) {
>>             /* FIXME: We must ensure that the request meets conditions. */
>>
>>             /* Set the status to be a 304. */
>>             r->status = HTTP_NOT_MODIFIED;
>>
>> Is this as simple as clearing r->headers_in, overwriting with
>> cache->stale_headers,
>> and the calling ap_meets_conditions()?
> 
> Yes, I *believe* so.  But, we should double-check that 
> ap_meets_condition would do the 'right' thing.  I'm not 100% sure here.

I'm fairly sure it would.  Considering we have the final response headers,
and the original request headers, this is just like a normal request.
So ap_meets_condition should do the trick just fine when it comes to
figuring out what to send back to the client.  I'll test, and if it works,
I'll commit it.
 
> Okay, back to your real bug:

[...]

> Please correct me if I'm wrong.  My understanding is that you have an 
> expired cache entry which needs to be revalidated and the updated 
> headers aren't updating properly.

Correct.
 
> Now that I read the code I'm betting you are hitting that else block 
> with 'XXX log message' in mod_disk_cache's store_headers.

What is that doing there, second guessing its caller (mod_cache)?!

> The sequence that I'm envisioning is:
> 
> - We have a stale cached entity/handle.
> - We send an If-Modified-Since/etc request
> - We get the 304 Not modified on revalidation with an updated Expires 
> header.
> - At mod_cache.c:533, we restore the cached handle.
> - Around mod_cache.c:658, we merge in the updated response headers
> - Then at mod_cache.c:683, we call the provider to store the headers.
> - Then, we hit store_headers in mod_disk_cache.
> - Because mod_disk_cache *had* a stale response, it would have loaded 
> the stale headers into its handle.
> - Hence, the check at mod_disk_cache.c:532 will fail because hfd is 
> 'valid.'
> - We never update the cached headers with the new Expires header
> 
> Can you confirm this sequence?

Yes, I can.  Sheesh.

>  I'm thinking we shouldn't even check for 
> the !hfd case anyway.  That check has probably been there forever and 
> was likely to be completely bogus from the start.  What do you think?  

I completely agree.  So much even that I just committed it (r156306).
Why are we storing the header fd in the disk object anyways?  I haven't
gone through mod_disk_cache.c yet, but at least for store_headers() it
doesn't seem to make any sense.

When it comes to store_headers(), shouldn't that be done using a temp
file and moving it into place atomically just like store_body?  Are
we relying on OS buffering and header size being small enough to pull
that off?  Or am I just being paranoid? :)

Sander

Mime
View raw message