You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Dirk-Willem van Gulik <di...@webweaving.org> on 2008/02/08 23:23:31 UTC
Patch: Centralize some Cache header handling
On Feb 8, 2008, at 6:26 PM, Dirk-Willem van Gulik wrote:
> Right now we have a nice utility function:
>
> ap_cache_cacheable_hdrs_out()
> Make a copy of the headers, and remove from
> the copy any hop-by-hop headers, as defined in Section
> 13.5.1 of RFC 2616
>
> (in cache_util.c, part of mod_cache.h public headers) which we can
> apply to both the in and outbound headers.
>
> Now as far as I can see -every- caching module will have to take
> care of the error headers and the content -- and filtering both in-
> and-out that way.
>
> So how about changing this to:
>
> - ap_cache_cacheable_hdrs_out(r);
> calls ap_cache_cacheable_hdrs
> AND
> Set Content-Type if not set
> AND
> overlays r->err_headers_out
>
> - ap_cache_cacheable_hdrs_in(r);
> calles ap_cache_cacheable_hdrs
>
> - ap_cache_cacheable_hdrs(pool, headers, server) (private)
> delete/cleanse as per RFC 2616
> and as per CacheIgnoreHeaders
>
> Or is there a fundamental reason as to why some caches will not want
> to do the 'usual'* ?
>
> Thoughts ?
Love to hear caching experts their thoughts -- esp. to hear confirmed
that it is correct that mod_cache.c needs indeed just to touch
headers_out (and not in) at that point and has no need to touch up
content-type.
Thanks,
Dw
Index: mod_cache.h
===================================================================
--- mod_cache.h (revision 618646)
+++ mod_cache.h (working copy)
@@ -289,12 +289,23 @@
CACHE_DECLARE(const char *)ap_cache_tokstr(apr_pool_t *p, const char
*list, const char **str);
/* Create a new table consisting of those elements from a
request_rec's
- * headers_out that are allowed to be stored in a cache
+ * headers that are allowed to be stored in a cache
*/
-CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs_out(apr_pool_t
*pool,
+CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs(apr_pool_t *pool,
apr_table_t
*t,
server_rec
*s);
+/* Create a new table consisting of those elements from an input
+ * headers table that are allowed to be stored in a cache.
+ */
+CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs_in(request_rec *);
+
+/* Create a new table consisting of those elements from an output
+ * headers table that are allowed to be stored in a cache;
+ * ensure there is a content type and capture any errors.
+ */
+CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs_out(request_rec *);
+
/**
* cache_storage.c
*/
Index: cache_util.c
===================================================================
--- cache_util.c (revision 618646)
+++ cache_util.c (working copy)
@@ -571,10 +573,7 @@
return apr_pstrdup(p, hashfile);
}
-/* Create a new table consisting of those elements from an input
- * headers table that are allowed to be stored in a cache.
- */
-CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs_out(apr_pool_t
*pool,
+CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs(apr_pool_t *pool,
apr_table_t
*t,
server_rec *s)
{
@@ -608,3 +607,34 @@
}
return headers_out;
}
+
+/* Create a new table consisting of those elements from an input
+ * headers table that are allowed to be stored in a cache.
+ */
+CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs_in(request_rec * r)
+{
+ return ap_cache_cacheable_hdrs(r->pool, r->headers_in, r->server);
+}
+
+/* Create a new table consisting of those elements from an output
+ * headers table that are allowed to be stored in a cache;
+ * ensure there is a content type and capture any errors.
+ */
+CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs_out(request_rec *
r)
+{
+ apr_table_t *headers_out;
+
+ headers_out = ap_cache_cacheable_hdrs(r->pool, r->headers_out,
+ r->server);
+
+ if (!apr_table_get(headers_out, "Content-Type")
+ && r->content_type) {
+ apr_table_setn(headers_out, "Content-Type",
+ ap_make_content_type(r, r->content_type));
+ }
+
+ headers_out = apr_table_overlay(r->pool, headers_out,
+ r->err_headers_out);
+
+ return headers_out;
+}
Index: mod_cache.c
===================================================================
--- mod_cache.c (revision 618646)
+++ mod_cache.c (working copy)
@@ -752,10 +778,12 @@
* However, before doing that, we need to first merge in
* err_headers_out and we also need to strip any hop-by-hop
* headers that might have snuck in.
+ *
+ * XX check -- we're not patching up content-type.
*/
r->headers_out = apr_table_overlay(r->pool, r->headers_out,
r->err_headers_out);
- r->headers_out = ap_cache_cacheable_hdrs_out(r->pool, r-
>headers_out,
+ r->headers_out = ap_cache_cacheable_hdrs(r->pool, r-
>headers_out,
r->server);
apr_table_clear(r->err_headers_out);
Index: mod_disk_cache.c
===================================================================
--- mod_disk_cache.c (revision 618646)
+++ mod_disk_cache.c (working copy)
@@ -922,17 +922,8 @@
if (r->headers_out) {
apr_table_t *headers_out;
- headers_out = ap_cache_cacheable_hdrs_out(r->pool, r-
>headers_out,
- r->server);
+ headers_out = ap_cache_cacheable_hdrs_out(r);
- if (!apr_table_get(headers_out, "Content-Type")
- && r->content_type) {
- apr_table_setn(headers_out, "Content-Type",
- ap_make_content_type(r, r->content_type));
- }
-
- headers_out = apr_table_overlay(r->pool, headers_out,
- r->err_headers_out);
rv = store_table(dobj->hfd, headers_out);
if (rv != APR_SUCCESS) {
return rv;
@@ -944,8 +935,8 @@
if (r->headers_in) {
apr_table_t *headers_in;
- headers_in = ap_cache_cacheable_hdrs_out(r->pool, r-
>headers_in,
- r->server);
+ headers_in = ap_cache_cacheable_hdrs_in(r);
+
rv = store_table(dobj->hfd, headers_in);
if (rv != APR_SUCCESS) {
return rv;
Index: mod_mem_cache.c
===================================================================
--- mod_mem_cache.c (revision 618646)
+++ mod_mem_cache.c (working copy)
@@ -605,17 +605,8 @@
mobj->req_hdrs = deep_table_copy(mobj->pool, r->headers_in);
/* Precompute how much storage we need to hold the headers */
- headers_out = ap_cache_cacheable_hdrs_out(r->pool, r->headers_out,
- r->server);
+ headers_out = ap_cache_cacheable_hdrs_out(r);
- /* If not set in headers_out, set Content-Type */
- if (!apr_table_get(headers_out, "Content-Type")
- && r->content_type) {
- apr_table_setn(headers_out, "Content-Type",
- ap_make_content_type(r, r->content_type));
- }
-
- headers_out = apr_table_overlay(r->pool, headers_out, r-
>err_headers_out);
mobj->header_out = deep_table_copy(mobj->pool, headers_out);
/* Init the info struct */
Re: Patch: Centralize some Cache header handling
Posted by Dirk-Willem van Gulik <di...@webweaving.org>.
On Feb 9, 2008, at 12:17 AM, Ruediger Pluem wrote:
> Looks good except for one small nitpick below:
>>
...
>> -/* Create a new table consisting of those elements from an input
>> - * headers table that are allowed to be stored in a cache.
>> - */
>
> Nitpicking: Why not using the comment from mod_cache.h here?
Because I am notoriously bad at cut-and-paste; esp. when in vi. I
notice another leftover in/out as well. Fixing..
Thanks!
Dw
Re: Patch: Centralize some Cache header handling
Posted by Ruediger Pluem <rp...@apache.org>.
On 02/08/2008 11:23 PM, Dirk-Willem van Gulik wrote:
> On Feb 8, 2008, at 6:26 PM, Dirk-Willem van Gulik wrote:
>
>> Right now we have a nice utility function:
>>
>> ap_cache_cacheable_hdrs_out()
>> Make a copy of the headers, and remove from
>> the copy any hop-by-hop headers, as defined in Section
>> 13.5.1 of RFC 2616
>>
>> (in cache_util.c, part of mod_cache.h public headers) which we can
>> apply to both the in and outbound headers.
>>
>> Now as far as I can see -every- caching module will have to take care
>> of the error headers and the content -- and filtering both in-and-out
>> that way.
>>
>> So how about changing this to:
>>
>> - ap_cache_cacheable_hdrs_out(r);
>> calls ap_cache_cacheable_hdrs
>> AND
>> Set Content-Type if not set
>> AND
>> overlays r->err_headers_out
>>
>> - ap_cache_cacheable_hdrs_in(r);
>> calles ap_cache_cacheable_hdrs
>>
>> - ap_cache_cacheable_hdrs(pool, headers, server) (private)
>> delete/cleanse as per RFC 2616
>> and as per CacheIgnoreHeaders
>>
>> Or is there a fundamental reason as to why some caches will not want
>> to do the 'usual'* ?
>>
>> Thoughts ?
>
> Love to hear caching experts their thoughts -- esp. to hear confirmed
> that it is correct that mod_cache.c needs indeed just to touch
> headers_out (and not in) at that point and has no need to touch up
> content-type.
>
Looks good except for one small nitpick below:
>
>
> Index: mod_cache.h
> ===================================================================
> --- mod_cache.h (revision 618646)
> +++ mod_cache.h (working copy)
> @@ -289,12 +289,23 @@
> CACHE_DECLARE(const char *)ap_cache_tokstr(apr_pool_t *p, const char
> *list, const char **str);
>
> /* Create a new table consisting of those elements from a request_rec's
> - * headers_out that are allowed to be stored in a cache
> + * headers that are allowed to be stored in a cache
> */
> -CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs_out(apr_pool_t *pool,
> +CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs(apr_pool_t *pool,
> apr_table_t *t,
> server_rec *s);
>
> +/* Create a new table consisting of those elements from an input
> + * headers table that are allowed to be stored in a cache.
> + */
> +CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs_in(request_rec *);
> +
> +/* Create a new table consisting of those elements from an output
> + * headers table that are allowed to be stored in a cache;
> + * ensure there is a content type and capture any errors.
> + */
> +CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs_out(request_rec *);
> +
> /**
> * cache_storage.c
> */
> Index: cache_util.c
> ===================================================================
> --- cache_util.c (revision 618646)
> +++ cache_util.c (working copy)
> @@ -571,10 +573,7 @@
> return apr_pstrdup(p, hashfile);
> }
>
> -/* Create a new table consisting of those elements from an input
> - * headers table that are allowed to be stored in a cache.
> - */
Nitpicking: Why not using the comment from mod_cache.h here?
> -CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs_out(apr_pool_t *pool,
> +CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs(apr_pool_t *pool,
> apr_table_t *t,
> server_rec *s)
> {
> @@ -608,3 +607,34 @@
> }
> return headers_out;
> }
> +
> +/* Create a new table consisting of those elements from an input
> + * headers table that are allowed to be stored in a cache.
> + */
> +CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs_in(request_rec * r)
> +{
> + return ap_cache_cacheable_hdrs(r->pool, r->headers_in, r->server);
> +}
> +
> +/* Create a new table consisting of those elements from an output
> + * headers table that are allowed to be stored in a cache;
> + * ensure there is a content type and capture any errors.
> + */
> +CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs_out(request_rec * r)
> +{
> + apr_table_t *headers_out;
> +
> + headers_out = ap_cache_cacheable_hdrs(r->pool, r->headers_out,
> + r->server);
> +
> + if (!apr_table_get(headers_out, "Content-Type")
> + && r->content_type) {
> + apr_table_setn(headers_out, "Content-Type",
> + ap_make_content_type(r, r->content_type));
> + }
> +
> + headers_out = apr_table_overlay(r->pool, headers_out,
> + r->err_headers_out);
> +
> + return headers_out;
> +}
Regards
Rüdiger