httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Justin Erenkrantz" <jus...@erenkrantz.com>
Subject [PATCH] Stop mod_cache from being too helpful was Re: svn commit: r539063 - in /httpd/httpd/trunk: CHANGES modules/cache/mod_cache.c
Date Thu, 17 May 2007 23:33:16 GMT
On 5/17/07, Roy T. Fielding <fielding@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;

Mime
View raw message