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