[PATCH] Upstream: fix the cache duration calculation

[Maxim Dounin]

Hello!

On Fri, Nov 15, 2013 at 04:35:09PM +0100, Florent Le Coz wrote:

> Hi,
> 
> On 11/15/2013 01:44 PM, Maxim Dounin wrote:
> […]
> >
> > Perfectly correct solution would be to store a bit (likely in
> > u->headers_in) to indicate that valid_sec was set based on
> > X-Accel-Expires and shouldn't be overwritten.
> >
> 
> Since there are, in nginx, three headers that can modify the value
> of valid_sec (Expires, Cache-Control and Expires), I think it would
> be cleaner to define a priority for each of these headers and to use
> that priority to decide if we modify or not the valid_sec value.
> 
> That’s what I’ve done in the attached patch.
> When setting the value of valid_sec, each header writes its own
> priority in valid_sec_prio.
> When processing an other header, instead of checking if the
> valid_sec is already set, we check if the headers’ priority is
> higher than the one set, before setting (or not) the value found in
> the header being processed.
> 
> I’ve set the priorities as: X-Accel-Expires > Cache-Control > Expires.
> I’m not sure about what the priority of X-Accel-Expires should be
> (but the last two are well defined in the RFC as you correctly
> pointed in a previous message).

That's certainly looks like an overkill.  At most, we need just 
one bit to disambiguate the Cache-Control header processing, as it 
needs to know whether valid_sec was set by X-Accel-Expires (and 
then it shouldn't do anything) or by Expires (and then it's 
expected to override the value set).

(A side note: we might also want to do something with u->cacheable 
set to 0 by Expires.  The Expires header is expected to be 
overriden by Cache-Control, but it doesn't happen if an Expires 
header contained a date in the past and u->cacheable was set to 0 
due to it.)

[...]

> @@ -3674,6 +3675,10 @@ ngx_http_upstream_process_expires(ngx_ht
>          return NGX_OK;
>      }
>  
> +    if (u->headers_in.valid_sec_prio >= NGX_HTTP_UPSTREAM_EXPIRES_H_P) {
> +        return NGX_OK;
> +    }
> +

Just a side note: this change is a nop as the check isn't reached 
if u->cache->valid_sec is set.

-- 
Maxim Dounin
http://nginx.org/en/donation.html