You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Eric Covener <co...@gmail.com> on 2014/05/05 15:27:25 UTC
Re: svn commit: r1591328 - in /httpd/httpd/trunk: CHANGES modules/cache/mod_cache.c
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?
On Wed, Apr 30, 2014 at 10:58 AM, <yl...@apache.org> wrote:
> Author: ylavic
> Date: Wed Apr 30 14:58:33 2014
> New Revision: 1591328
>
> URL: http://svn.apache.org/r1591328
> Log:
> mod_cache: Preserve non-cacheable headers forwarded from an origin 304
> response. PR 55547.
>
> When mod_cache asks for a revalidation of a stale entry and the origin responds
> with a 304 (not that stale), the module strips the non-cacheable headers from
> the origin response and merges the stale headers to update the cache.
>
> The problem is that mod_cache won't forward the non-cacheable headers to the
> client, for example if the 304 response contains both Set-Cookie and
> 'Cache-Control: no-cache="Set-Cookie"' headers, or CacheIgnoreHeaders is used.
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/modules/cache/mod_cache.c
>
> Modified: httpd/httpd/trunk/CHANGES
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1591328&r1=1591327&r2=1591328&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
> +++ httpd/httpd/trunk/CHANGES [utf-8] Wed Apr 30 14:58:33 2014
> @@ -1,6 +1,9 @@
> -*- coding: utf-8 -*-
> Changes with Apache 2.5.0
>
> + *) mod_cache: Preserve non-cacheable headers forwarded from an origin 304
> + response. PR 55547. [Yann Ylavic]
> +
> *) mod_cache: Don't add cached/revalidated entity headers to a 304 response.
> PR 55547. [Yann Ylavic]
>
>
> Modified: httpd/httpd/trunk/modules/cache/mod_cache.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cache/mod_cache.c?rev=1591328&r1=1591327&r2=1591328&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/cache/mod_cache.c (original)
> +++ httpd/httpd/trunk/modules/cache/mod_cache.c Wed Apr 30 14:58:33 2014
> @@ -1444,10 +1444,14 @@ static apr_status_t cache_save_filter(ap
> * the cached headers.
> *
> * However, before doing that, we need to first merge in
> - * err_headers_out and we also need to strip any hop-by-hop
> - * headers that might have snuck in.
> + * err_headers_out (note that store_headers() below already selects
> + * the cacheable only headers using ap_cache_cacheable_headers_out(),
> + * here we want to keep the original headers in r->headers_out and
> + * forward all of them to the client, including non-cacheable ones).
> */
> - r->headers_out = ap_cache_cacheable_headers_out(r);
> + r->headers_out = apr_table_overlay(r->pool, r->headers_out,
> + r->err_headers_out);
> + apr_table_clear(r->err_headers_out);
>
> /* Merge in our cached headers. However, keep any updated values. */
> /* take output, overlay on top of cached */
>
>
--
Eric Covener
covener@gmail.com
Re: svn commit: r1591328 - in /httpd/httpd/trunk: CHANGES modules/cache/mod_cache.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, May 5, 2014 at 3:27 PM, Eric Covener <co...@gmail.com> 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().