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
=== modified file 'src/esi/VarState.cc'
--- src/esi/VarState.cc 2012-11-19 11:29:31 +0000
+++ src/esi/VarState.cc 2013-03-21 13:53:47 +0000
@@ -857,35 +857,50 @@
delete currentFunction;
}
-/* XXX FIXME: this should be comma delimited, no? */
void
-ESIVarState::buildVary (HttpReply *rep)
+ESIVarState::buildVary(HttpReply *rep)
{
+ /* TODO: use Key header modifiers.
+ * which will mean the eval() function above need to record what
+ * was scanned for in each of these replies.
+ * For now we generate it taking advantage of the modifiers being
+ * optional such that we can use identical Vary and Key headers.
+ */
char tempstr[1024];
tempstr[0]='\0';
if (flags.language)
- strcat (tempstr, "Accept-Language ");
+ strcat(tempstr, ",Accept-Language");
if (flags.cookie)
- strcat (tempstr, "Cookie ");
+ strcat(tempstr, ",Cookie");
if (flags.host)
- strcat (tempstr, "Host ");
+ strcat(tempstr, ",Host");
if (flags.referer)
- strcat (tempstr, "Referer ");
+ strcat(tempstr, ",Referer");
if (flags.useragent)
- strcat (tempstr, "User-Agent ");
+ strcat(tempstr, ",User-Agent");
if (!tempstr[0])
return;
- String strVary (rep->header.getList (HDR_VARY));
-
- if (!strVary.size() || strVary[0] != '*') {
- rep->header.putStr (HDR_VARY, tempstr);
+ const String strVary(rep->header.getList(HDR_VARY));
+
+ // Vary:* overrides everything including Key:
+ if (strVary.size()>0 && strVary[0] == '*')
+ return;
+
+ const String strKey(rep->header.getList(HDR_KEY));
+ if (strKey.size() == 0 && strVary.size() > 0) {
+ // protect the pre-existing Vary details by upgrading into Key values
+ // unless there is already a Key: supplied.
+ rep->header.putStr(HDR_KEY, strVary.termedBuf());
}
+
+ // append the above tempstr list to both Vary: and Key:
+ rep->header.putStr(HDR_VARY, &tempstr[1]);
+ rep->header.putStr(HDR_KEY, &tempstr[1]);
}
-
