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

New patch attached resolving most of the issues identified below.

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.

>
>> + String strVary(rep->header.getList(HDR_VARY));
>> if (!strVary.size() || strVary[0] != '*') {
>> - rep->header.putStr (HDR_VARY, tempstr);
>> + rep->header.putStr(HDR_VARY, &tempstr[1]));
>> + }
>> +
>> + String strVaryKey(rep->header.getList(HDR_KEY));
>> + if (!strVaryKey.size()) {
>> + rep->header.putStr(HDR_KEY, &tempstr[1]);
>> }
> When strVary[0] is '*', will the above code essentially overwrite a very
> strong
>
> Vary: *
>
> with a much weaker
>
> Key: Host
>
> (for example) because Key takes priority over Vary?

It would in that patch. I double-checked with the authors and Mark
indicted that Vary:* should have preference over both Vary and Key patterns.
So I've since shuffled the Vary:* test up into an earlier:

  if (strVary.size() >0 && strVary[0] == '*') return;

> Similarly, when strVary is not "*" and is not empty, are we not
> overwriting that whole Vary value with a potentially much weaker Key

Good catch. Fixed by 'upgrading' any existing Vary entries to Key
entries in the event that Key is itself not already supplied.

> 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.

> Both strVary and strVaryKey can be declared const.
>
> Please s/strVaryKey/strKey/ for consistency if possible.

Done.

>
>> Due to the complexity of identifying which values are actually
>> considered we cannot at this stage emit any Key flags to narrow down the
>> caching choices. That is marked as a TODO
> In that TODO, please use official terminology and call them "modifiers"
> rather than "flags".

Done.

Amos