On Wed, Mar 27, 2013 at 5:44 PM, Graham Leggett <[email protected]> wrote:
> Been snowed under and haven't had a chance to look at this in detail, but one
> quick thing - we would definitely want to be able to backport this to v2.4 so
> as to get it into people's hands, and to do that, we cannot change the public
> APIs. We would need to find a way to do this without changing the API.
>
> A further thing is the reparsing on the Cache-Control string, I'd like to see
> if I can find a way to avoid this, but need to dig as how to do that.
In the patch below there is no API change and Cache-Control headers
won't be parsed twice, by updating the h->req/resp_hdrs before calling
provider->store_headers() and let it use h->req/resp_headers instead
of recomputing the whole from r->(err_)headers_out.
Regards,
Yann.
Index: modules/cache/cache_util.c
===================================================================
--- modules/cache/cache_util.c (revision 1461557)
+++ modules/cache/cache_util.c (working copy)
@@ -542,13 +542,10 @@
}
/* These come from the cached entity. */
- if (h->cache_obj->info.control.no_cache
- || h->cache_obj->info.control.no_cache_header
- || h->cache_obj->info.control.private_header) {
+ if (h->cache_obj->info.control.no_cache) {
/*
- * The cached entity contained Cache-Control: no-cache, or a
- * no-cache with a header present, or a private with a header
- * present, so treat as stale causing revalidation.
+ * The cached entity contained Cache-Control: no-cache,
+ * so treat as stale causing revalidation.
*/
return 0;
}
@@ -1069,9 +1066,7 @@
/* ...then try slowest cases */
else if (!strncasecmp(token, "no-cache", 8)) {
if (token[8] == '=') {
- if (apr_table_get(headers, token + 9)) {
- cc->no_cache_header = 1;
- }
+ cc->no_cache_header = 1;
}
else if (!token[8]) {
cc->no_cache = 1;
@@ -1146,9 +1141,7 @@
}
else if (!strncasecmp(token, "private", 7)) {
if (token[7] == '=') {
- if (apr_table_get(headers, token + 8)) {
- cc->private_header = 1;
- }
+ cc->private_header = 1;
}
else if (!token[7]) {
cc->private = 1;
Index: modules/cache/mod_cache.c
===================================================================
--- modules/cache/mod_cache.c (revision 1461557)
+++ modules/cache/mod_cache.c (working copy)
@@ -714,6 +714,65 @@
}
/*
+ * Same as ap_cacheable_headers_out(), but also strips the headers
+ * specified by the Cache-Control private= or no-cache= directives.
+ */
+static int cc_field_doo_doo(void *t, const char *key,
+ const char *val)
+{
+ if (val) {
+ apr_table_addn(t, key, val);
+ return 0;
+ }
+ return 1;
+}
+static apr_table_t *cache_cacheable_headers_cc(request_rec *r,
+ const cache_control_t *cc)
+{
+ apr_table_t *headers_out = ap_cache_cacheable_headers_out(r);
+ if (cc && (cc->no_cache_header || cc->private_header)) {
+ char *token;
+ const char *cc_out = apr_table_get(headers_out, "Cache-Control");
+ while (cc_out && (token = ap_get_list_item(r->pool, &cc_out))) {
+ apr_size_t len = strlen(token);
+
+ /* ap_get_list_item() strips the spurious whitespaces and
+ * lowercases anything (but the quoted-strings) */
+ if (len > 9 && strncmp(token, "no-cache=", 9) == 0) {
+ token += 9;
+ len -= 9;
+ }
+ else if (len > 8 && strncmp(token, "private=", 8) == 0) {
+ token += 8;
+ len -= 8;
+ }
+ else {
+ continue;
+ }
+
+ /* RFC2616 14.9: quoted list of field-names */
+ if (len > 2 && token[0] == '"' && token[--len] == '"') {
+ /* strip the header(s) from the cacheable headers out,
+ * but preserve the ones from the current response by
+ * adding them to the err_headers_out */
+ const char *tok, *header;
+ (++token)[--len] = '\0';
+ tok = token;
+ do {
+ if ((header = ap_cache_tokstr(r->pool, tok, &tok)) &&
+ !apr_table_do(cc_field_doo_doo, r->err_headers_out,
+ headers_out, header, NULL)) {
+ apr_table_unset(r->headers_out, header);
+ apr_table_unset(headers_out, header);
+ }
+ } while (tok);
+ }
+ }
+ }
+ return headers_out;
+}
+
+/*
* CACHE_SAVE filter
* ---------------
*
@@ -1075,7 +1134,7 @@
* err_headers_out and we also need to strip any hop-by-hop
* headers that might have snuck in.
*/
- r->headers_out = ap_cache_cacheable_headers_out(r);
+ r->headers_out = cache_cacheable_headers_cc(r, &control);
/* Merge in our cached headers. However, keep any
updated values. */
cache_accept_headers(cache->handle, r, 1);
@@ -1332,6 +1391,9 @@
}
info->expire = exp;
+ cache->handle->req_hdrs = ap_cache_cacheable_headers_in(r);
+ cache->handle->resp_hdrs = cache_cacheable_headers_cc(r, &control);
+
/* We found a stale entry which wasn't really stale. */
if (cache->stale_handle) {
/* Load in the saved status and clear the status line. */
@@ -1347,7 +1409,7 @@
* err_headers_out and we also need to strip any hop-by-hop
* headers that might have snuck in.
*/
- r->headers_out = ap_cache_cacheable_headers_out(r);
+ r->headers_out = apr_table_copy(r->pool, cache->handle->resp_hdrs);
/* Merge in our cached headers. However, keep any updated values. */
cache_accept_headers(cache->handle, r, 1);
Index: modules/cache/mod_cache_disk.c
===================================================================
--- modules/cache/mod_cache_disk.c (revision 1461557)
+++ modules/cache/mod_cache_disk.c (working copy)
@@ -933,11 +933,11 @@
memcpy(&h->cache_obj->info, info, sizeof(cache_info));
if (r->headers_out) {
- dobj->headers_out = ap_cache_cacheable_headers_out(r);
+ dobj->headers_out = h->resp_hdrs;
}
if (r->headers_in) {
- dobj->headers_in = ap_cache_cacheable_headers_in(r);
+ dobj->headers_in = h->req_hdrs;
}
return APR_SUCCESS;
Index: modules/cache/mod_cache_socache.c
===================================================================
--- modules/cache/mod_cache_socache.c (revision 1461557)
+++ modules/cache/mod_cache_socache.c (working copy)
@@ -791,11 +791,11 @@
memcpy(&h->cache_obj->info, info, sizeof(cache_info));
if (r->headers_out) {
- sobj->headers_out = ap_cache_cacheable_headers_out(r);
+ sobj->headers_out = h->resp_hdrs;
}
if (r->headers_in) {
- sobj->headers_in = ap_cache_cacheable_headers_in(r);
+ sobj->headers_in = h->req_hdrs;
}
sobj->expire
