You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Justin Erenkrantz <ju...@erenkrantz.com> on 2007/05/18 01:33:16 UTC
[PATCH] Stop mod_cache from being too helpful was Re: svn commit: r539063 - in /httpd/httpd/trunk: CHANGES modules/cache/mod_cache.c
On 5/17/07, Roy T. Fielding <fi...@gbiv.com> wrote:
> On May 17, 2007, at 2:53 PM, Justin Erenkrantz wrote:
> > BTW, I'm not a fan of us inventing Expires headers in this section of
> > code - I'd think it'd be far cleaner to off-load Expires response
> > header generation to mod_expires and leave the cache out of it
> > entirely - inventing Expires values deep inside of mod_cache seems
> > unclean. mod_cache, IMO, should just respect what it is told and not
> > change how things appear to downstream folks. (mod_expires is more
> > than capable of inserting in the Expires header if the admin wants
> > it.)
>
> I agree -- caches are not allowed to invent header fields like Expires.
> They can only do so by explicit override in the configuration
> (mod_expires).
> Setting Expires here is wrong. Changing max-age would be even worse.
> Age is the only thing the cache should be setting.
>
> > Does my position make sense? I'm of the opinion that we should go one
> > of two ways:
> >
> > - mod_cache shouldn't touch the response - so it stops setting Expires
> > or anything like that which affects cachability
>
> +1
>
> > - mod_cache always tweaks the response - so my CC: max-age fix needs
> > to mimic what we do for Expires.
>
> -1
With this patch, the only thing mod_cache ever touches is...Age. =)
I'll let this sit here for a bit and if we agree this is right, we can
commit. -- justin
Index: modules/cache/mod_cache.c
===================================================================
--- modules/cache/mod_cache.c (revision 539156)
+++ modules/cache/mod_cache.c (working copy)
@@ -663,21 +663,8 @@
now = apr_time_now();
if (info->date == APR_DATE_BAD) { /* No, or bad date */
- char *dates;
/* no date header (or bad header)! */
- /* add one; N.B. use the time _now_ rather than when we were checking
- * the cache
- */
- if (date_in_errhdr == 1) {
- apr_table_unset(r->err_headers_out, "Date");
- }
- date = now;
- dates = apr_pcalloc(r->pool, MAX_STRING_LEN);
- apr_rfc822_date(dates, now);
- apr_table_set(r->headers_out, "Date", dates);
- ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
- "cache: Added date header");
- info->date = date;
+ info->date = now;
}
else {
date = info->date;
@@ -709,7 +696,6 @@
* expire date = date + defaultexpire
*/
if (exp == APR_DATE_BAD) {
- char expire_hdr[APR_RFC822_DATE_LEN];
char *max_age_val;
if (ap_cache_liststr(r->pool, cc_out, "max-age", &max_age_val) &&
@@ -746,13 +732,9 @@
x = conf->maxex;
}
exp = date + x;
- apr_rfc822_date(expire_hdr, exp);
- apr_table_set(r->headers_out, "Expires", expire_hdr);
}
else {
exp = date + conf->defex;
- apr_rfc822_date(expire_hdr, exp);
- apr_table_set(r->headers_out, "Expires", expire_hdr);
}
}
info->expire = exp;
Re: [PATCH] Stop mod_cache from being too helpful was Re: svn commit: r539063 - in /httpd/httpd/trunk: CHANGES modules/cache/mod_cache.c
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 5/17/07, Ruediger Pluem <rp...@apache.org> wrote:
> Hm. In this case date would be undefined and it is used later on.
Oh. Good catch.
> Maybe we should remove the else branch and set date = info->date in
> any case so that it is either the contents of Date: or now.
Sure. That'll work.
See below. Also removed an unused variable too. -- justin
Index: modules/cache/mod_cache.c
===================================================================
--- modules/cache/mod_cache.c (revision 539156)
+++ modules/cache/mod_cache.c (working copy)
@@ -318,7 +318,6 @@
static int cache_save_filter(ap_filter_t *f, apr_bucket_brigade *in)
{
int rv = !OK;
- int date_in_errhdr = 0;
request_rec *r = f->r;
cache_request_rec *cache;
cache_server_conf *conf;
@@ -648,10 +647,7 @@
/* Read the date. Generate one if one is not supplied */
dates = apr_table_get(r->err_headers_out, "Date");
- if (dates != NULL) {
- date_in_errhdr = 1;
- }
- else {
+ if (dates == NULL) {
dates = apr_table_get(r->headers_out, "Date");
}
if (dates != NULL) {
@@ -663,25 +659,10 @@
now = apr_time_now();
if (info->date == APR_DATE_BAD) { /* No, or bad date */
- char *dates;
/* no date header (or bad header)! */
- /* add one; N.B. use the time _now_ rather than when we were checking
- * the cache
- */
- if (date_in_errhdr == 1) {
- apr_table_unset(r->err_headers_out, "Date");
- }
- date = now;
- dates = apr_pcalloc(r->pool, MAX_STRING_LEN);
- apr_rfc822_date(dates, now);
- apr_table_set(r->headers_out, "Date", dates);
- ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
- "cache: Added date header");
- info->date = date;
+ info->date = now;
}
- else {
- date = info->date;
- }
+ date = info->date;
/* set response_time for HTTP/1.1 age calculations */
info->response_time = now;
@@ -709,7 +690,6 @@
* expire date = date + defaultexpire
*/
if (exp == APR_DATE_BAD) {
- char expire_hdr[APR_RFC822_DATE_LEN];
char *max_age_val;
if (ap_cache_liststr(r->pool, cc_out, "max-age", &max_age_val) &&
@@ -746,13 +726,9 @@
x = conf->maxex;
}
exp = date + x;
- apr_rfc822_date(expire_hdr, exp);
- apr_table_set(r->headers_out, "Expires", expire_hdr);
}
else {
exp = date + conf->defex;
- apr_rfc822_date(expire_hdr, exp);
- apr_table_set(r->headers_out, "Expires", expire_hdr);
}
}
info->expire = exp;
Re: [PATCH] Stop mod_cache from being too helpful was Re: svn commit:
r539063 - in /httpd/httpd/trunk: CHANGES modules/cache/mod_cache.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 05/18/2007 01:33 AM, Justin Erenkrantz wrote:
>
> With this patch, the only thing mod_cache ever touches is...Age. =)
>
> I'll let this sit here for a bit and if we agree this is right, we can
> commit. -- justin
>
> Index: modules/cache/mod_cache.c
> ===================================================================
> --- modules/cache/mod_cache.c (revision 539156)
> +++ modules/cache/mod_cache.c (working copy)
> @@ -663,21 +663,8 @@
>
> now = apr_time_now();
> if (info->date == APR_DATE_BAD) { /* No, or bad date */
> - char *dates;
> /* no date header (or bad header)! */
> - /* add one; N.B. use the time _now_ rather than when we were
> checking
> - * the cache
> - */
> - if (date_in_errhdr == 1) {
> - apr_table_unset(r->err_headers_out, "Date");
> - }
> - date = now;
Hm. In this case date would be undefined and it is used later on.
Maybe we should remove the else branch and set date = info->date in
any case so that it is either the contents of Date: or now.
Regards
Rüdiger