On Mon, May 5, 2014 at 3:27 PM, Eric Covener <[email protected]> wrote:
> Trying to get my 2.4.x reviews in.
>
> Maybe I'm misunderstanding the change, but wasn't the previous
> behavior more desirable?
>
> If the entry were within its expiry, those same headers wouldn't have
> been sent to the client (well, none but the first who filled in the
> cache). Why should it act differently just because it had to
> revalidated with its own conditional?
The previous behaviour was to forward 304s with only cacheable
headers, stripping the others, including those which are not part of
the entity (meant exclusively to the client).
Is this the desirable behaviour?
Actually the case I encountered was an origin (re)setting a cookie at
(almost) every hit, including (updated) 304s (using Cache-Control:
private="Set-Cookie").
Since the Set-Cookie wasn't forwarded to the client, it resulted in
the next request hitting the origin being rejected because of an
invalid (outdated) cookie.
I don't see why mod_cache would remove those from the stream, RFC 2616
10.3.5 (304 Not Modified), cited in the corresponding code, talks
about entity-headers that MUST NOT/SHOULD NOT be included in 304
responses, but Set-Cookie is not an entity-header.
The point in this code, IMHO, is to merge the stale headers with the
updated ones from the origin's 304, so that store_headers() is using
up to date values. But since store_headers() ignores non-cacheable
headers by itself, we don't need to remove them from r->headers_out
before (losing original headers together).
However, this patch seems incomplete.
By replacing the call to ap_cache_cacheable_headers_out() by the
simple apr_table_overlay(), I missed the r->content_type and
r->content_encoding copies into r->headers_out, which may be needed by
store_headers() and the following code.
Maybe I should add the following patch :
Index: modules/cache/mod_cache.c
===================================================================
--- modules/cache/mod_cache.c (revision 1592855)
+++ modules/cache/mod_cache.c (working copy)
@@ -1453,6 +1453,19 @@ static apr_status_t cache_save_filter(ap_filter_t
r->err_headers_out);
apr_table_clear(r->err_headers_out);
+ if (r->content_type
+ && !apr_table_get(r->headers_out, "Content-Type")) {
+ apr_table_setn(r->headers_out, "Content-Type",
+ ap_make_content_type(r, r->content_type));
+ }
+
+ if (r->content_encoding
+ && !apr_table_get(r->headers_out, "Content-Encoding")) {
+ apr_table_setn(r->headers_out, "Content-Encoding",
+ r->content_encoding);
+ }
+
+
/* Merge in our cached headers. However, keep any updated values. */
/* take output, overlay on top of cached */
cache_accept_headers(cache->handle, r, r->headers_out,
[END]
It seems that a common function should be defined and called both by
ap_cache_cacheable_headers_out() and cache_save_filter().
