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

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?

> + 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?

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

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.

Both strVary and strVaryKey can be declared const.

Please s/strVaryKey/strKey/ for consistency if possible.

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

Thank you,

Alex.
Received on Thu Mar 21 2013 - 05:13:01 MDT