You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2007/05/17 22:46:38 UTC
Re: svn commit: r539063 - in /httpd/httpd/trunk: CHANGES modules/cache/mod_cache.c
On 05/17/2007 08:25 PM, jerenkrantz@apache.org wrote:
> Author: jerenkrantz
> Date: Thu May 17 11:25:13 2007
> New Revision: 539063
> Modified: httpd/httpd/trunk/modules/cache/mod_cache.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cache/mod_cache.c?view=diff&rev=539063&r1=539062&r2=539063
> ==============================================================================
> --- httpd/httpd/trunk/modules/cache/mod_cache.c (original)
> +++ httpd/httpd/trunk/modules/cache/mod_cache.c Thu May 17 11:25:13 2007
> @@ -701,19 +701,42 @@
> }
>
> /* if no expiry date then
> - * if lastmod
> + * if Cache-Control: max-age
> + * expiry date = date + max-age
> + * else if lastmod
> * expiry date = date + min((date - lastmod) * factor, maxexpire)
> * else
> * expire date = date + defaultexpire
> */
> if (exp == APR_DATE_BAD) {
> char expire_hdr[APR_RFC822_DATE_LEN];
> + char *max_age_val;
>
> - /* if lastmod == date then you get 0*conf->factor which results in
> - * an expiration time of now. This causes some problems with
> - * freshness calculations, so we choose the else path...
> - */
> - if ((lastmod != APR_DATE_BAD) && (lastmod < date)) {
> + if (ap_cache_liststr(r->pool, cc_out, "max-age", &max_age_val) &&
> + max_age_val != NULL) {
> + apr_int64_t x;
> +
> + errno = 0;
> + x = apr_atoi64(max_age_val);
> + if (errno) {
> + x = conf->defex;
Hm. Shouldn't we adjust the bogus max-age field value in the Cache-Control header in r->headers_out
/ r->err_headers_out to conf->defex (in seconds of course) in this case?
Regards
Rüdiger
Re: svn commit: r539063 - in /httpd/httpd/trunk: CHANGES modules/cache/mod_cache.c
Posted by "Roy T. Fielding" <fi...@gbiv.com>.
On May 17, 2007, at 4:37 PM, Justin Erenkrantz wrote:
> See the patch I just posted - we should also remove the Date
> manipulation too, IMO.
Probably. If Date is added, it should be done by the protocol filter
(mod_proxy) when the message is received, not the cache.
>> Just for clarification: It is still ok for us to define an cache
>> internal expiration
>> date (default / calculated) if there is nothing valid (max-age,
>> Expires) in the
>> response, right?
>
> Yes.
>
>> We just do not tell the client about it as we do not adjust the
>> headers in the response
>> accordingly.
>
> Exactly.
>
>> BTW: What about s-max-age here? Shouldn't we use s-maxage instead
>> of max-age in the
>> case it is present in the response?
>
> Probably. I've only dug into the max-age case so far - I haven't
> chased down the semantics for us supporting s-maxage yet. Though
> max-age is explicitly called out as an explicit expiration time, while
> s-maxage isn't. Again, I'd need to refresh in my head what s-maxage
> is for... -- justin
s-maxage is for use when the value for a shared cache would differ
from the value for a private cache.
s-maxage
If a response includes an s-maxage directive, then for a shared
cache (but not for a private cache), the maximum age
specified by
this directive overrides the maximum age specified by either the
max-age directive or the Expires header. The s-maxage directive
also implies the semantics of the proxy-revalidate directive
(see
section 14.9.4), i.e., that the shared cache must not use the
entry after it becomes stale to respond to a subsequent request
without first revalidating it with the origin server. The s-
maxage directive is always ignored by a private cache.
We should probably have a directive somewhere to indicate whether
we are operating a shared cache (default) or private cache.
....Roy
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:
> I don't want to pick on you, but ironically it was you who introduced this :-)
> (http://svn.apache.org/viewvc?view=rev&revision=152973).
Oh, it probably was. What can I say? I'm less stupid now. =)
> Ok. So we should remove the
>
> apr_rfc822_date(expire_hdr, exp);
> apr_table_set(r->headers_out, "Expires", expire_hdr);
>
> lines there.
See the patch I just posted - we should also remove the Date
manipulation too, IMO.
> Just for clarification: It is still ok for us to define an cache internal expiration
> date (default / calculated) if there is nothing valid (max-age, Expires) in the
> response, right?
Yes.
> We just do not tell the client about it as we do not adjust the headers in the response
> accordingly.
Exactly.
> BTW: What about s-max-age here? Shouldn't we use s-maxage instead of max-age in the
> case it is present in the response?
Probably. I've only dug into the max-age case so far - I haven't
chased down the semantics for us supporting s-maxage yet. Though
max-age is explicitly called out as an explicit expiration time, while
s-maxage isn't. Again, I'd need to refresh in my head what s-maxage
is for... -- justin
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 12:19 AM, Roy T. Fielding 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
I don't want to pick on you, but ironically it was you who introduced this :-)
(http://svn.apache.org/viewvc?view=rev&revision=152973).
>> 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
Ok. So we should remove the
apr_rfc822_date(expire_hdr, exp);
apr_table_set(r->headers_out, "Expires", expire_hdr);
lines there.
Just for clarification: It is still ok for us to define an cache internal expiration
date (default / calculated) if there is nothing valid (max-age, Expires) in the
response, right?
We just do not tell the client about it as we do not adjust the headers in the response
accordingly.
BTW: What about s-max-age here? Shouldn't we use s-maxage instead of max-age in the
case it is present in the response?
Regards
Rüdiger
Re: svn commit: r539063 - in /httpd/httpd/trunk: CHANGES modules/cache/mod_cache.c
Posted by "Roy T. Fielding" <fi...@gbiv.com>.
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
....Roy
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:
> But it already does in the case of the Expires header. If the Expires
> header is missing or bogus and no max-age field is present (valid or invalid),
> we set one with the expires date the cache calculated (either heuristic or
> default). And this creates the next question for me: Why always use the default
> expire setting in the case of a bogus max-age? In the case of a bogus Expire
> header we try to calculate a heuristic expiration date using the Last-Modified
> header (if present and valid).
> I think we should handle Expires and max-age in the same way here.
My concern with falling back to the Last-Modified date on a bogus
Cache-Control max-age field is that the server was *trying* to tell us
something via the max-age field. So, falling back to Last-Modified
seems wrong-ish. I'd rather just stay silent here and go with our
default value.
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.)
> > shouldn't tweak the value under any circumstances.
> >
> > I can be convinced that's the wrong approach and that we should indeed
> > normalize it in the event of an error, but my gut says letting is just
> > pass-through seems right. -- justin
>
> But shouldn't the client have the same expiration expectation as the cache?
> In the case of an invalid max-age field I would see it as a correct decision
> by a client to ignore it completely for its local cache and thus possibly
> do not cache it at all.
Since the cache is not authoritative, I think it's slightly sketchy
for us to tweak any downstream value based on our local policy
decisions.
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
- mod_cache always tweaks the response - so my CC: max-age fix needs
to mimic what we do for Expires.
WDYT? -- justin
Re: svn commit: r539063 - in /httpd/httpd/trunk: CHANGES modules/cache/mod_cache.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 05/17/2007 10:56 PM, Justin Erenkrantz wrote:
> On 5/17/07, Ruediger Pluem <rp...@apache.org> wrote:
>
>> > + x = apr_atoi64(max_age_val);
>> > + if (errno) {
>> > + x = conf->defex;
>>
>> Hm. Shouldn't we adjust the bogus max-age field value in the
>> Cache-Control header in r->headers_out
>> / r->err_headers_out to conf->defex (in seconds of course) in this case?
>
>
> I thought about doing that - but I came to the idea that our local
> cache is doing its own validation, but it shouldn't make its local
> expiration decisions binding upon downstream requestors - i.e. it
But it already does in the case of the Expires header. If the Expires
header is missing or bogus and no max-age field is present (valid or invalid),
we set one with the expires date the cache calculated (either heuristic or
default). And this creates the next question for me: Why always use the default
expire setting in the case of a bogus max-age? In the case of a bogus Expire
header we try to calculate a heuristic expiration date using the Last-Modified
header (if present and valid).
I think we should handle Expires and max-age in the same way here.
> shouldn't tweak the value under any circumstances.
>
> I can be convinced that's the wrong approach and that we should indeed
> normalize it in the event of an error, but my gut says letting is just
> pass-through seems right. -- justin
But shouldn't the client have the same expiration expectation as the cache?
In the case of an invalid max-age field I would see it as a correct decision
by a client to ignore it completely for its local cache and thus possibly
do not cache it at all.
Regards
Rüdiger
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:
> > + x = apr_atoi64(max_age_val);
> > + if (errno) {
> > + x = conf->defex;
>
> Hm. Shouldn't we adjust the bogus max-age field value in the Cache-Control header in r->headers_out
> / r->err_headers_out to conf->defex (in seconds of course) in this case?
I thought about doing that - but I came to the idea that our local
cache is doing its own validation, but it shouldn't make its local
expiration decisions binding upon downstream requestors - i.e. it
shouldn't tweak the value under any circumstances.
I can be convinced that's the wrong approach and that we should indeed
normalize it in the event of an error, but my gut says letting is just
pass-through seems right. -- justin