Re: [patch] refresh bugs

On 2 May 2006, at 12:25, Henrik Nordstrom wrote:

> tis 2006-05-02 klockan 11:08 +1200 skrev Doug Dixon:
>
>> Not sure I follow... unless I'm mistaken, I think this fixes Bug #7
>
> Bug #7 is about updating HTTP headers on a 304. This is mainly
>
> Date
> Expires
> Cache-Control
>
> but there may also be other headers which needs to be refreshed with
> content from the new response.

Still sounds the same as bug 1548. See comment #1
http://www.squid-cache.org/bugs/show_bug.cgi?id=1548#c1

RFC2616 (13.5.3) says that ALL end-to-end headers in the 304 should
override those in the cached entry

In the patch I supplied, the existing entry's headers are always
overridden on receipt of a 304 from the origin server. This is done
with a call to HttpReply::updateOnNotModified() which in turn calls
httpHeaderUpdate() with a Denied304HeadersMask that says which
headers should not be updated.

However..... ulgh... looking at it further, the headers in the
Denied304HeadersMask are completely wrong. Looks like it allows
updating of many hop-by-hop headers (by not denying them), and
prevents updating of many end-to-end headers. In fact, all it does is
PREVENT end-to-end headers being updated that SHOULD be updated!
Behold evil:

static http_hdr_type Denied304HeadersArr[] =
     {
         HDR_ALLOW, HDR_CONTENT_ENCODING, HDR_CONTENT_LANGUAGE,
HDR_CONTENT_LENGTH,
         HDR_CONTENT_LOCATION, HDR_CONTENT_RANGE, HDR_LAST_MODIFIED,
HDR_LINK,
         HDR_OTHER
     };

The only reason my patch appears to work is that Cache-Control,
Expires and Date are mercifully not listed here!

How about renaming it and put all the hop-by-hop headers (as defined
in section 13.5.1 of the spec) in it?
E.g.:

/* If we receive a 304 from the origin during a cache revalidation,
  * then hop-by-hop headers must NOT be updated in the cache entry.
  *
  * End-to-end headers (i.e. all others) must be updated.
  */
static HttpHeaderMask HopByHopHeadersMask;
static http_hdr_type HopByHopHeadersArr[] =
{
     HDR_CONNECTION, HDR_KEEP_ALIVE, HDR_PROXY_AUTHENTICATE,
HDR_PROXY_AUTHORIZATION,
     HDR_TE, HDR_TRAILERS, HDR_TRANSFER_ENCODING, HDR_UPGRADE
};

If this is the correct approach, then the following headers would
need to be defined in http_hdr_type:

HDR_KEEP_ALIVE
HDR_TE
HDR_TRAILERS

Does this sound like the right way to go? Can you see any gotchas?

>
>> I guess the question is whether the call to storeTimestampsSet
>> (old_entry) sets the timestamp correctly or not in this instance, and
>> therefore whether we need to overwrite this immediately afterwards
>> (as happens at present).
>
> Currently the timestamps are never set 100% correct according to
> specs.
> And it's worse on refreshed objects as then only the internal
> timestamp
> is updated, not the HTTP headers..

As I say, the HTTP headers should be updated, not just the store
entry's attributes.

>
> Some of the most apparent bad effects of this can be seen in cache
> hierarchies, where child caches sometimes always consider objects
> stale
> due to old Date & Expires headers. It's also seen by changes in
> Cache-Control never becoming effective at all until a forced reload of
> the whole object.

Regardless of any timestamp bug, this sounds eerily like the bug
currently under discussion (7 aka 1548)
No headers are ever updated on a 304 from the origin - Date & Expires
as well as Cache-Control - so all objects become permanently stale
and only a forced reload will make them fresh again.

In any case, I'm assuming the timestamp issue is not limited to
refreshes, and so should be fixed separately. Is there an open bug
for this btw?

Cheers
Doug
Received on Mon May 01 2006 - 21:52:23 MDT