Bug 35247

Created attachment 15319 [details]
test case trace

I noticed this when reading through the code;  the code in question
(from cache-util.c is)

   if (((smaxage != -1) && (age < (smaxage - minfresh))) ||
        ((maxage != -1) && (age < (maxage + maxstale - minfresh))) ||
        ((smaxage == -1) && (maxage == -1) && /* check expires */...

It may be a simple oversight that the second clause doesn't have
a (smaxage == 1). Certainly RFC 2616 implies that the above is wrong:

  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.

That implies to me that if s-maxage is set, it should also override any
max-age passed in the request. If s-maxage only is supposed to override 
max-age in the cached response, then a slightly more complicated fix would
be needed.

If I understand you correctly, you propose the following patch:

Index: modules/cache/cache_util.c
===================================================================
--- modules/cache/cache_util.c  (Revision 411466)
+++ modules/cache/cache_util.c  (Arbeitskopie)
@@ -297,7 +297,8 @@

     /* handle expiration */
     if (((smaxage != -1) && (age < (smaxage - minfresh))) ||
-        ((maxage != -1) && (age < (maxage + maxstale - minfresh))) ||
+        ((smaxage == -1) && (maxage != -1) &&
+         (age < (maxage + maxstale - minfresh))) ||
         ((smaxage == -1) && (maxage == -1) &&
          (info->expire != APR_DATE_BAD) &&
          (age < (apr_time_sec(info->expire - info->date) + maxstale -
minfresh)))) {

Yes, that was my proposal. Note that I'm in no way an HTTP expert, so I 
would advise doing some double-checking :-)

> That implies to me that if s-maxage is set, it should also override any
> max-age passed in the request. If s-maxage only is supposed to override 
> max-age in the cached response, then a slightly more complicated fix would
> be needed.

Not quite.

Neither change the meaning of the request max-age. It's a separate condition all
together.

If CC s-maxage is set in the response then this overrides any Expires and/or CC
max-age in the response header and their values are not relevant for the
expiry/freshness calculation.

If CC max-age is set in the response then this overrides any Expires response
header and the Expires header is not relevant for the expiry/freshness calculation. 

The reasoning behind this is to allow the content provided to tune caching
appropriately. There is situations where one wants shared caches to cache longer
than private caches, and there is siuations where the shared cache should cache
a shorter interval. Expires is the HTTP/1.0 fallback, and should consequently be
ignored if any of the HTTP/1.1 constraints is available (max-age or s-maxage).


However, CC max-age request header rmust also be respected in all conditions.
There is no response headers which overrides this request header.


There is no s-maxage request CC directive, as it would be kind of pointless to
define a such directive.


There is however a the max-stale request directive which can relax the freshness
checks, but not if any of the must-revalidate conditions is set on the response
(must-revalidate, s-maxage, proxy-revalidate, no-cache). If any of the
must-revalidate conditions is set then stale objects must be revalidated with
the origin before sent to the requesting client.


response s-max-age / max-age / expires (in that order) sets the server side
freshness expiry date.

if neither s-max-age / max-age or expires is present then heuristics MAY be used
to assign an freshness expiry date on the response.

request max-age sets the client-side upper limit on the freshness expiry date
ontop of the freshness expiry defined by the origin.

after this request max-stale applies and defines how far after the freshness
expiry date is considered acceptable by the client. The cache MAY send such
stale responses.


And yes, I do consider myself an expert on HTTP.

True, the simple change I suggested above wouldn't make sense in the case:

 request CC max-age 1 day
 response CC max-age 3 days
 response CC s-maxage 2 days

Then we should consider cached content stale if it is more than 1 day
stale; so "a slightly more complicated fix" is in fact needed.... s-maxage
needs to be taken into account before the minimum of the request and response
max-age is taken.

But in any case, the current code is certainly not right :-)

Fixed on httpd-trunk in r1069942.