On 15/04/2015 9:49 a.m., Sriram Devadas wrote:
> Squid version 3.5.3.
> When the http response received by Squid contains a no-cache="set-cookie", the response is not cached. cache.log has the line:
> 2015/04/14 18:24:38.027 kid1| http.cc(359) cacheableReply: NO because server reply Cache-Control:no-cache has parameters
>
> The relevant source code is in http.cc:
> if (rep->cache_control && rep->cache_control->hasNoCache() && rep->cache_control->noCache().size() > 0) {
> /* TODO: we are allowed to cache when no-cache= has parameters.
> * Provided we strip away any of the listed headers unless they are revalidated
> * successfully (ie, must revalidate AND these headers are prohibited on stale replies).
> * That is a bit tricky for squid right now so we avoid caching entirely.
> */
> debugs(22, 3, HERE << "NO because server reply Cache-Control:no-cache has parameters");
> return 0;
> }
>
> I have read the TODO comment but do not know if this applies to the set-cookie parameter too.
> Is it okay to add a condition here not to return if the cache-control header contains only no-cache="set-cookie"?
I dont think so. At least by itself that is not enough.
The existing code is there to enact the generic fail-safe action
permitted of the HTTP specification. To treat any unsupported no-cache
condition from the server as a no-store.
That is currently being done because there has been zero testing of the
code paths your propose change would enable. Please feel free to go
ahead and do that testing, patches that improve the caching are very
welcome.
But be aware there are very likely also some missing code elsewhere that
causes broken behaviour. This change requires that the header stripping
or updating on replies to clients operates correctly in the presence of
all possibly 20x and 30x response codes. More on that below...
> In client_side_reply.cc the no-cache="set-cookie" seems to be handled correctly, the set-cookie header is removed in the response, which I am guessing removes this header correctly in the response:
> if (is_hit)
> hdr->delById(HDR_SET_COOKIE);
>
Well no, this would likely (untested) cause a significant amount of
traffic to have wrongly missing Set-Cookie. Today that traffic is a
MISS, but MiSS guarantees correct headers.
The (is_hit) condition is wrongly positioned in the code sequence (far
too late currently). Its also not handling any other headers that could
be listed by no-cache="...".
There are two options that would do it right, for all headers as easily
as for Set-Cookie alone:
1) going through the no-cache list and stripping away all those headers
from the cached copy before it got stored (and in collapsed-forwarding
code paths before object sharing).
or,
2) The same removal of headers in the code where object is loaded from
cache (and in collapsed-forwarding code paths before object sharing).
The header needs to be erased from the cached copy that could be sent to
clients, AND all same-named headers the origin server provides on its
revalidation 30x response needs to be added in their place later in
those 30x handling code paths.
Amos
_______________________________________________
squid-users mailing list
squid-users@xxxxxxxxxxxxxxxxxxxxx
http://lists.squid-cache.org/listinfo/squid-users
