Re: [RFC] bug 1890 fix: upgrading Date header

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
Received on Tue Nov 13 2012 - 01:12:13 MST