On 13.11.2012 13:14, Alex Rousskov wrote:
On 11/11/2012 07:39 PM, Amos Jeffries wrote:
Are there any objections to committing this patch?
http://bugs.squid-cache.org/attachment.cgi?id=2570&action=diff
Yes, there are.
AFAICT, the patch does two things:
1) Patched code assumes that a _valid_ Date header is always present
when HttpReply::hdrCacheInit() is called (but does not verify that
assumption with an assert or some such).
Good point. Validating existing Date header was not a goal of this but
should have been. Goal was to fix the RFC requirement that no response
be emitted without Date, ensuring that the one added was at least
correct.
2) Patched code adds a Date header to _some_ HTTP responses if a Date
header (valid or _not_) is not already present. AFAICT, this is done
after HttpReply::hdrCacheInit() has been called.
Patched code does not add Date to all HTTP responses, and when it
adds
it, it does so too late.
The changes result, among other things, in
HttpReply::hdrExpirationTime() setting incorrect cached expires time
in
some cases (using negative date value in calculations).
Inserting Date: header on replies where it is missing. Using Age and
Last-Modified to infer dates older than current timestamp for
conservative expiry behaviour. Doing so before caching (AFAICT) to
improve HIT ratio.
Perhaps I do not understand the true intention of this patch, but if
the
goal was to set HttpReply::expires value (calculated by
hdrExpirationTime), then HttpReply::hdrExpirationTime() should have
been
modified accordingly.
I have not reviewed the inference logic itself.
Thank you.
Amos
