Sander Striker wrote:Justin Erenkrantz wrote:Sander Striker wrote:
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.
I'm fairly sure this doesn't work too well yet. See below.
[...]
/* 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.
It works (gave the correct results on cached, cached revalidating, not cached) so I committed it (r156330).
However, I do think you are right that ap_meets_conditions() doesn't do the right thing. But that is in general, not just in this case. It doesn't seem to take responses other than 2xx into account. In those cases it shouldn't return a 304, yet it does. We'll have to visit all the places where ap_meets_conditions() is called to make sure r->status is set, and check r->status in ap_meets_conditions() to fix this.
Luckily for us, there is more work left even in mod_cache. Right now, whenever we hit a Cache-Control: no-cache in the request, the cache declines to handle the request, while it could be handling it (be it with a required validation request to the backend). That would be a lot more efficient. And within bounds of the spec.
Furthermore, Cache-Control: max-age=0 or even max-age=X seems to be completely ignored. A response is given back with an Age header with a larger value then what max-age was set to in the request.
Sander
