[prev in list] [next in list] [prev in thread] [next in thread]
List: squid-dev
Subject: Re: X-Vary-Options patch
From: Adrian Chadd <adrian () creative ! net ! au>
Date: 2008-02-26 4:30:01
Message-ID: 20080226043001.GA15578 () skywalker ! creative ! net ! au
[Download RAW message or body]
G'day,
I'm happy to commit this to Squid-2.HEAD as-is. Can you throw it in
a Bugzilla report and spit me the number?
Thanks,
Adrian
On Fri, Feb 08, 2008, Tim Starling wrote:
> There are two major sources of suboptimal hit rate on Wikipedia which
> relate to the Vary header:
>
> * In Accept-Encoding, we only care whether "gzip" is present or not, but
> IE and Firefox use different whitespace conventions and so each get
> separate entries in the cache
> * We only care whether the user is logged in or not. Other cookies, such
> as pure-JavaScript cookies used by client-side code to store
> preferences, unnecessarily degrade our hit rate.
>
> There have been other patches related to this problem, but as far as I'm
> aware, they're all special-case, site-specific hacks. My patch adds an
> X-Vary-Options response header (hereafter XVO), and thus gives the
> origin server fine control over cache variance. In the patch, the XVO
> header overrides the Vary header, so the Vary header can still be sent
> as usual for compatibility with caches that don't support this feature.
>
> The format of the XVO header is inspired by the format of the Accept
> header. As in Vary, XVO is separated by commas into parts which relate
> to different request headers. Then those parts are further separated by
> semicolons. The first semicolon-separated part is the request header
> name, and subsequent parts give name/value pairs separated by equals
> signs, defining options relating to the variance of that header.
>
> Two option names are currently defined:
>
> list-contains: splits the request header into comma-separated parts
> and varies depending on whether the resulting list contains the option value
> string-contains: performs a simple search of the request header and
> varies depending on whether it matches.
>
> Multiple such options per header are allowed.
>
> So for example:
>
> X-Vary-Options: Cookie; string-contains=UserID;
> string-contains=_session, Accept-Encoding; list-contains=gzip
>
> This would vary the cache on three tests:
> * whether the Cookie header contains the string "UserID"
> * whether the Cookie header contains the string "_session"
> * whether the Accept-Encoding header, interpreted as a comma-separated
> list, contains the item "gzip"
>
> The patch refactors all references to the Vary and X-Accelerator-Vary
> headers into the functions httpHeaderHasVary() and httpHeaderGetVary()
> in HttpHeader.c. It then adds X-Vary-Options to these functions,
> interpreting it as a string rather than a list to avoid inappropriate
> splitting on whitespace. It puts the resulting combined header into
> X-Vary-Options instead of Vary in the base vary marker object, again to
> avoid inappropriate list-style interpretation. httpMakeVaryMark() then
> interprets this combined header in the way described above.
>
> The added features of the patch are conditional, and are enabled by the
> configure option --enable-vary-options. Autoconf and automake will need
> to be run after applying this patch.
>
> The interpretation of some non-standards-compliant Vary headers (those
> containing semicolons) is changed slightly by this patch regardless of
> --enable-vary-options.
>
> The patch is attached and also available at:
> http://noc.wikimedia.org/~tstarling/patches/vary_options_upstream.patch
>
> For your review and consideration.
>
> -- Tim Starling
> diff -Xdiffx -ru squid-2.6.18.orig/configure.in squid-2.6.18/configure.in
> --- squid-2.6.18.orig/configure.in 2008-01-10 23:34:23.000000000 +1100
> +++ squid-2.6.18/configure.in 2008-02-07 19:43:23.000000000 +1100
> @@ -1507,6 +1507,16 @@
> fi
> ])
>
> +dnl Enable vary options
> +AC_ARG_ENABLE(vary_options,
> +[ --enable-vary-options
> + Enable support for the X-Vary-Options header.],
> +[ if test "$enableval" = "yes" ; then
> + echo "Enabling support for vary options"
> + AC_DEFINE(VARY_OPTIONS, 1, [Enable support for the X-Vary-Options header])
> + fi
> +])
> +
> AC_ARG_ENABLE(follow-x-forwarded-for,
> [ --enable-follow-x-forwarded-for
> Enable support for following the X-Forwarded-For
> diff -Xdiffx -ru squid-2.6.18.orig/src/client_side.c squid-2.6.18/src/client_side.c
> --- squid-2.6.18.orig/src/client_side.c 2008-02-07 19:28:38.000000000 +1100
> +++ squid-2.6.18/src/client_side.c 2008-02-08 14:39:38.000000000 +1100
> @@ -735,10 +735,7 @@
> request_t *request = http->request;
> const char *etag = httpHeaderGetStr(&mem->reply->header, HDR_ETAG);
> const char *vary = request->vary_headers;
> - int has_vary = httpHeaderHas(&entry->mem_obj->reply->header, HDR_VARY);
> -#if X_ACCELERATOR_VARY
> - has_vary |= httpHeaderHas(&entry->mem_obj->reply->header, \
> HDR_X_ACCELERATOR_VARY);
> -#endif
> + int has_vary = httpHeaderHasVary(&entry->mem_obj->reply->header);
> if (has_vary)
> vary = httpMakeVaryMark(request, mem->reply);
>
> @@ -4948,10 +4945,7 @@
> varyEvaluateMatch(StoreEntry * entry, request_t * request)
> {
> const char *vary = request->vary_headers;
> - int has_vary = httpHeaderHas(&entry->mem_obj->reply->header, HDR_VARY);
> -#if X_ACCELERATOR_VARY
> - has_vary |= httpHeaderHas(&entry->mem_obj->reply->header, \
> HDR_X_ACCELERATOR_VARY);
> -#endif
> + int has_vary = httpHeaderHasVary(&entry->mem_obj->reply->header);
> if (!has_vary || !entry->mem_obj->vary_headers) {
> if (vary) {
> /* Oops... something odd is going on here.. */
> diff -Xdiffx -ru squid-2.6.18.orig/src/enums.h squid-2.6.18/src/enums.h
> --- squid-2.6.18.orig/src/enums.h 2008-02-07 19:28:38.000000000 +1100
> +++ squid-2.6.18/src/enums.h 2008-02-07 21:35:18.000000000 +1100
> @@ -256,6 +256,9 @@
> #if X_ACCELERATOR_VARY
> HDR_X_ACCELERATOR_VARY,
> #endif
> +#if VARY_OPTIONS
> + HDR_X_VARY_OPTIONS,
> +#endif
> HDR_X_ERROR_URL, /* errormap, requested URL */
> HDR_X_ERROR_STATUS, /* errormap, received HTTP status line */
> HDR_FRONT_END_HTTPS,
> diff -Xdiffx -ru squid-2.6.18.orig/src/http.c squid-2.6.18/src/http.c
> --- squid-2.6.18.orig/src/http.c 2008-02-07 19:28:38.000000000 +1100
> +++ squid-2.6.18/src/http.c 2008-02-08 14:48:44.000000000 +1100
> @@ -353,20 +353,29 @@
> String vstr = StringNull;
>
> stringClean(&vstr);
> - hdr = httpHeaderGetList(&reply->header, HDR_VARY);
> - if (strBuf(hdr))
> - strListAdd(&vary, strBuf(hdr), ',');
> - stringClean(&hdr);
> -#if X_ACCELERATOR_VARY
> - hdr = httpHeaderGetList(&reply->header, HDR_X_ACCELERATOR_VARY);
> - if (strBuf(hdr))
> - strListAdd(&vary, strBuf(hdr), ',');
> - stringClean(&hdr);
> -#endif
> + vary = httpHeaderGetVary(&reply->header);
> + debug(11,3) ("httpMakeVaryMark: Vary: %s\n", strBuf(vary));
> +
> while (strListGetItem(&vary, ',', &item, &ilen, &pos)) {
> - char *name = xmalloc(ilen + 1);
> - xstrncpy(name, item, ilen + 1);
> - Tolower(name);
> + const char *sc_item, *sc_pos = NULL;
> + int sc_ilen;
> + String str_item;
> + char *name = NULL;
> + String value_spec = StringNull;
> + int need_value = 1;
> +
> + stringLimitInit(&str_item, item, ilen);
> +
> + /* Get the header name */
> + if (strListGetItem(&str_item, ';', &sc_item, &sc_ilen, &sc_pos)) {
> + name = xmalloc(sc_ilen + 1);
> + xstrncpy(name, sc_item, sc_ilen + 1);
> + Tolower(name);
> + } else {
> + name = xmalloc(1);
> + *name = '\0';
> + }
> +
> if (strcmp(name, "accept-encoding") == 0) {
> aclCheck_t checklist;
> memset(&checklist, 0, sizeof(checklist));
> @@ -381,22 +390,76 @@
> if (strcmp(name, "*") == 0) {
> /* Can not handle "Vary: *" efficiently, bail out making the response not \
> cached */ safe_free(name);
> + stringClean(&str_item);
> stringClean(&vary);
> stringClean(&vstr);
> break;
> }
> - strListAdd(&vstr, name, ',');
> +
> + /* Fetch the header string */
> hdr = httpHeaderGetByName(&request->header, name);
> - safe_free(name);
> - value = strBuf(hdr);
> - if (value) {
> - value = rfc1738_escape_part(value);
> - stringAppend(&vstr, "=\"", 2);
> - stringAppend(&vstr, value, strlen(value));
> - stringAppend(&vstr, "\"", 1);
> +
> + /* Process the semicolon-separated options */
> +#ifdef VARY_OPTIONS
> + while (strListGetItem(&str_item, ';', &sc_item, &sc_ilen, &sc_pos)) {
> + char *opt_name = xmalloc(sc_ilen + 1);
> + char *opt_value;
> + char *eqpos;
> + xstrncpy(opt_name, sc_item, sc_ilen + 1);
> + eqpos = strchr(opt_name, '=');
> + if (!eqpos) {
> + opt_value = NULL;
> + } else {
> + *eqpos = '\0';
> + opt_value = eqpos + 1;
> + }
> + Tolower(opt_name);
> +
> + if (strcmp(opt_name, "list-contains") == 0 && opt_value) {
> + if (strBuf(hdr) && strListIsMember(&hdr, opt_value, ',')) {
> + opt_value = rfc1738_escape_part(opt_value);
> + strListAdd(&value_spec, "list-contains[\"", ';');
> + stringAppend(&value_spec, opt_value, strlen(opt_value));
> + stringAppend(&value_spec, "\"]", 2);
> + }
> + need_value = 0;
> + } else if (strcmp(opt_name, "string-contains") == 0 && opt_value) {
> + if (strBuf(hdr) && strIsSubstr(&hdr, opt_value)) {
> + opt_value = rfc1738_escape_part(opt_value);
> + strListAdd(&value_spec, "string-contains[\"", ';');
> + stringAppend(&value_spec, opt_value, strlen(opt_value));
> + stringAppend(&value_spec, "\"]", 2);
> + }
> + need_value = 0;
> + } else {
> + debug(11,3) ("httpMakeVaryMark: unrecognised vary option: %s\n", opt_name);
> + }
> + safe_free(opt_name);
> }
> +#endif
> +
> + if (need_value) {
> + value = strBuf(hdr);
> + if (value) {
> + value = rfc1738_escape_part(value);
> + strListAdd(&value_spec, "\"", ';');
> + stringAppend(&value_spec, value, strlen(value));
> + stringAppend(&value_spec, "\"", 1);
> + }
> + }
> +
> + strListAdd(&vstr, name, ',');
> + stringAppend(&vstr, "=", 1);
> + if (strBuf(value_spec)) {
> + stringAppend(&vstr, strBuf(value_spec), strLen(value_spec));
> + }
> +
> stringClean(&hdr);
> + stringClean(&value_spec);
> + stringClean(&str_item);
> + safe_free(name);
> }
> +
> safe_free(request->vary_hdr);
> safe_free(request->vary_headers);
> if (strBuf(vary) && strBuf(vstr)) {
> @@ -514,11 +577,7 @@
> /* non-chunked. Handle as one single big chunk (-1 if terminated by EOF) */
> httpState->chunk_size = httpReplyBodySize(httpState->orig_request->method, reply);
> }
> - if (httpHeaderHas(&reply->header, HDR_VARY)
> -#if X_ACCELERATOR_VARY
> - || httpHeaderHas(&reply->header, HDR_X_ACCELERATOR_VARY)
> -#endif
> - ) {
> + if (httpHeaderHasVary(&reply->header)) {
> const char *vary = NULL;
> if (Config.onoff.cache_vary)
> vary = httpMakeVaryMark(httpState->orig_request, reply);
> diff -Xdiffx -ru squid-2.6.18.orig/src/HttpHeader.c squid-2.6.18/src/HttpHeader.c
> --- squid-2.6.18.orig/src/HttpHeader.c 2007-12-21 20:56:53.000000000 +1100
> +++ squid-2.6.18/src/HttpHeader.c 2008-02-08 14:49:24.000000000 +1100
> @@ -133,6 +133,9 @@
> #if X_ACCELERATOR_VARY
> {"X-Accelerator-Vary", HDR_X_ACCELERATOR_VARY, ftStr},
> #endif
> +#if VARY_OPTIONS
> + {"X-Vary-Options", HDR_X_VARY_OPTIONS, ftStr},
> +#endif
> {"X-Error-URL", HDR_X_ERROR_URL, ftStr},
> {"X-Error-Status", HDR_X_ERROR_STATUS, ftInt},
> {"Front-End-Https", HDR_FRONT_END_HTTPS, ftStr},
> @@ -210,6 +213,9 @@
> #if X_ACCELERATOR_VARY
> HDR_X_ACCELERATOR_VARY,
> #endif
> +#if VARY_OPTIONS
> + HDR_X_VARY_OPTIONS,
> +#endif
> HDR_X_SQUID_ERROR
> };
>
> @@ -1185,6 +1191,54 @@
> return tot;
> }
>
> +/* Get the combined Vary headers as a String
> + * Returns StringNull if there are no vary headers
> + */
> +String httpHeaderGetVary(const HttpHeader * hdr)
> +{
> + String hdrString = StringNull;
> +#if VARY_OPTIONS
> + HttpHeaderEntry *e;
> + if ((e = httpHeaderFindEntry(hdr, HDR_X_VARY_OPTIONS))) {
> + stringInit(&hdrString, strBuf(e->value));
> + return hdrString;
> + }
> +#endif
> +
> + hdrString = httpHeaderGetList(hdr, HDR_VARY);
> +#if X_ACCELERATOR_VARY
> + {
> + String xavString = StringNull;
> + xavString = httpHeaderGetList(hdr, HDR_X_ACCELERATOR_VARY);
> + if (strBuf(xavString))
> + strListAdd(&hdrString, strBuf(xavString), ',');
> + stringClean(&xavString);
> + }
> +#endif
> + return hdrString;
> +}
> +
> +/*
> + * Returns TRUE if at least one of the vary headers are present
> + */
> +int httpHeaderHasVary(const HttpHeader * hdr)
> +{
> +#if VARY_OPTIONS
> + if (httpHeaderHas(hdr, HDR_X_VARY_OPTIONS)) {
> + return TRUE;
> + }
> +#endif
> +#if X_ACCELERATOR_VARY
> + if (httpHeaderHas(hdr, HDR_X_ACCELERATOR_VARY)) {
> + return TRUE;
> + }
> +#endif
> + if (httpHeaderHas(hdr, HDR_VARY)) {
> + return TRUE;
> + }
> + return FALSE;
> +}
> +
> /*
> * HttpHeaderEntry
> */
> @@ -1438,3 +1492,5 @@
> assert(id >= 0 && id < HDR_ENUM_END);
> return strBuf(Headers[id].name);
> }
> +
> +
> diff -Xdiffx -ru squid-2.6.18.orig/src/HttpReply.c squid-2.6.18/src/HttpReply.c
> --- squid-2.6.18.orig/src/HttpReply.c 2006-06-11 10:28:19.000000000 +1000
> +++ squid-2.6.18/src/HttpReply.c 2008-02-08 14:42:04.000000000 +1100
> @@ -325,8 +325,7 @@
> return squid_curtime;
> }
> }
> - if (Config.onoff.vary_ignore_expire &&
> - httpHeaderHas(&rep->header, HDR_VARY)) {
> + if (Config.onoff.vary_ignore_expire && httpHeaderHasVary(&rep->header)) {
> const time_t d = httpHeaderGetTime(&rep->header, HDR_DATE);
> const time_t e = httpHeaderGetTime(&rep->header, HDR_EXPIRES);
> if (d == e)
> diff -Xdiffx -ru squid-2.6.18.orig/src/protos.h squid-2.6.18/src/protos.h
> --- squid-2.6.18.orig/src/protos.h 2008-02-07 19:28:38.000000000 +1100
> +++ squid-2.6.18/src/protos.h 2008-02-08 14:46:21.000000000 +1100
> @@ -444,6 +444,8 @@
> extern squid_off_t httpHeaderGetSize(const HttpHeader * hdr, http_hdr_type id);
> extern time_t httpHeaderGetTime(const HttpHeader * hdr, http_hdr_type id);
> extern TimeOrTag httpHeaderGetTimeOrTag(const HttpHeader * hdr, http_hdr_type id);
> +extern String httpHeaderGetVary(const HttpHeader * hdr);
> +extern int httpHeaderHasVary(const HttpHeader * hdr);
> extern HttpHdrCc *httpHeaderGetCc(const HttpHeader * hdr);
> extern HttpHdrRange *httpHeaderGetRange(const HttpHeader * hdr);
> extern HttpHdrContRange *httpHeaderGetContRange(const HttpHeader * hdr);
> diff -Xdiffx -ru squid-2.6.18.orig/src/store.c squid-2.6.18/src/store.c
> --- squid-2.6.18.orig/src/store.c 2008-02-07 19:28:38.000000000 +1100
> +++ squid-2.6.18/src/store.c 2008-02-08 14:55:06.000000000 +1100
> @@ -721,7 +721,12 @@
> state->e = storeCreateEntry(url, log_url, flags, method);
> httpBuildVersion(&version, 1, 0);
> httpReplySetHeaders(state->e->mem_obj->reply, version, HTTP_OK, "Internal marker \
> object", "x-squid-internal/vary", -1, -1, squid_curtime + 100000); +#if \
> VARY_OPTIONS + /* Can't put a string into a list header */
> + httpHeaderPutStr(&state->e->mem_obj->reply->header, HDR_X_VARY_OPTIONS, vary);
> +#else
> httpHeaderPutStr(&state->e->mem_obj->reply->header, HDR_VARY, vary);
> +#endif
> storeSetPublicKey(state->e);
> if (!state->oe) {
> /* New entry, create new unique ID */
> @@ -1039,20 +1044,8 @@
> }
> newkey = storeKeyPublicByRequest(mem->request);
> if (mem->vary_headers && !EBIT_TEST(e->flags, KEY_EARLY_PUBLIC)) {
> - String vary = StringNull;
> vary_id_t vary_id;
> - String varyhdr;
> - varyhdr = httpHeaderGetList(&mem->reply->header, HDR_VARY);
> - if (strBuf(varyhdr))
> - strListAdd(&vary, strBuf(varyhdr), ',');
> - stringClean(&varyhdr);
> -#if X_ACCELERATOR_VARY
> - /* This needs to match the order in http.c:httpMakeVaryMark */
> - varyhdr = httpHeaderGetList(&mem->reply->header, HDR_X_ACCELERATOR_VARY);
> - if (strBuf(varyhdr))
> - strListAdd(&vary, strBuf(varyhdr), ',');
> - stringClean(&varyhdr);
> -#endif
> + String vary = httpHeaderGetVary(&mem->reply->header);
> /* Create or update the vary object */
> vary_id = storeAddVary(mem->url, mem->log_url, mem->method, newkey, \
> httpHeaderGetStr(&mem->reply->header, HDR_ETAG), strBuf(vary), mem->vary_headers, \
> mem->vary_encoding); if (vary_id.create_time) {
>
--
- Xenion - http://www.xenion.com.au/ - VPS Hosting - Commercial Squid Support -
- $25/pm entry-level VPSes w/ capped bandwidth charges available in WA -
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic