Re: [PATCH] Key header support in ESI responses and old ESI Vary bug, fixed.

On 03/21/2013 08:20 AM, Amos Jeffries wrote:
> On 21/03/2013 6:12 p.m., Alex Rousskov wrote:
>> On 03/20/2013 09:34 PM, Amos Jeffries wrote:
>>
>>> The attached patch adds the Key: header on Squid generated ESI responses
>>> so that any receiving clients which support the new header are able to
>>> use it for handling our pages.
>> I do not understand how a receiving client can actually use the Key
>> header if it is the same as the Vary header, which will be the common
>> case after this patch is applied. Can you clarify why we want to send
>> both Vary and Key header fields with identical values?
>
> See the other post about why we need to send them both.
> In this particular case though there is no sub-variation on Key yet. So
> until then its presence is entirely optional. I'm adding it here for the
> sake of making Squid emit Key on all outputs rather than any actual need
> for it. If you want to veto the Key on these outputs for now I'm fine
> with that - however I would like the Vary fixing to happen in either way.

I do not feel strongly enough about ESI to veto anything small that does
not affect the rest of Squid. I just wanted to understand why you want
to add a header that does not change the meaning of the message. (We
will argue whether your interpretation of the draft is correct on
another thread). If you think this Key code is useful for any reason,
and nobody objects, you should add it.

>> It is also strange that we do not want to _add_ Vary values to existing
>> ones. Is this some kind of ESI-specific logic? I would think that, in
>> general, if we know that the responds depends on Cookie, we should add
>> Vary: Cookie regardless of whether there was another Vary header there
>> already. Same for Key, I guess.
>
> HttpHeader::putStr() appends an header to existing values. So yes the
> function was *adding* the default list to any existing ones.

My bad: I got the if-statement conditions for Vary wrong. The above
problem did apply to Key though: the Key if-statement prevented adding
new Key values to the old ones. The new patch does not have that flaw.

Thank you,

Alex.
Received on Thu Mar 21 2013 - 16:25:27 MDT