[PATCH] Upstream: fix the cache duration calculation

[Maxim Dounin]

Hello!

On Thu, Nov 14, 2013 at 04:33:55PM +0100, Florent Le Coz wrote:

> Hello,
> 
> We have a "chain" of nginx servers, like this:
> A (nginx) -> B (nginx) -> C (anything)
> 
> B being A’s upstream, and C being B’s upstream
> 
> We ran into an issue where the files were being cached for a too
> long duration.  For example if we want to cache our files in this
> chain for 10 seconds, it could happen that the file actually exist
> for up to 30 seconds, because when B retrieves the file from C, even
> if it’s almost expired on C, B will cache it for 10 seconds. Same
> thing if A retrieves the file cached by B.
> 
> To fix this issue, nginx needs to properly take into account the Age
> header provided by its upstream. This is what the attached patch
> does.

While I agree with problems the patch tries to address (i.e., 
"Cache-Control: max-age" should take precedence over Expires, and 
Age should be taken into account while interpreting 
"Cache-Control: max-age"), I'm not happy with the patch provided.

Given that there are at least two problems to address, I would 
suggest there should be at least two patches: one to address 
Expires vs. Cache-Control, and another one to handle Age header.

[...]

> Upstream: fix the cache duration calculation
> 
> - Treat Expires and Cache-control headers in a well-defined order, instead
>   of treating them in the order they are received from upstream
> - If Expires and Cache-control headers are both present and indicate a
>   different cache duration, use the biggest value of the two

As per RFC2616, if Expires and Cache-Control are both present, the 
Cache-Control should be used, 
http://tools.ietf.org/html/rfc2616#section-14.9.3:

   If a response includes both an Expires header and a max-age
   directive, the max-age directive overrides the Expires header, even
   if the Expires header is more restrictive.

[...]

> @@ -229,6 +233,11 @@

Just a side note: please add 

[diff]
showfunc=1

to ~/.hgrc.  It makes review much easier.  Thanks.

>                   ngx_http_upstream_copy_header_line,
>                   offsetof(ngx_http_headers_out_t, expires), 1 },
>  
> +    { ngx_string("Age"),
> +                 ngx_http_upstream_process_age, 0,
> +                 ngx_http_upstream_copy_header_line,
> +                 offsetof(ngx_http_headers_out_t, age), 1 },
> +

As used in the patch, it looks like there is no need for a special 
process function for the Age header.

>      { ngx_string("Accept-Ranges"),
>                   ngx_http_upstream_process_header_line,
>                   offsetof(ngx_http_upstream_headers_in_t, accept_ranges),
> @@ -2255,6 +2264,7 @@
>      /* TODO: preallocate event_pipe bufs, look "Content-Length" */
>  
>  #if (NGX_HTTP_CACHE)
> +    ngx_http_upstream_set_cache_valid(r, u);
>  

Processing Cache-Control header in 
ngx_http_upstream_send_response() is at least sub-optimal, and may 
cause unneeded work if caching is disabled with, e.g., 
"Cache-Control: no-cache".

At most, this should be done in ngx_http_upstream_process_headers(), 
but even this will be too late for upcoming cache revalidation 
with conditional requests.

[...]

> +    if (!(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_CACHE_CONTROL))
> +    {
> +        u_char     *p;
> +        u_char     *last;
> +
> +        ph = u->headers_in.cache_control.elts;
> +
> +        for (i = 0; i != u->headers_in.cache_control.nelts; i++)
> +        {
> +            h = ph[i];
> +            p = h->value.data;
> +            last = p + h->value.len;
> +
> +            if (ngx_strlcasestrn(p, last, (u_char *) "no-cache", 8 - 1) != NULL
> +                || ngx_strlcasestrn(p, last, (u_char *) "no-store", 8 - 1) != NULL
> +                || ngx_strlcasestrn(p, last, (u_char *) "private", 7 - 1) != NULL)
> +            {
> +                u->cacheable = 0;
> +                return;
> +            }

This seems to introduce multiple style problems.  It dosn't really 
matter though, as it would be much better idea to don't move the 
code, see above.

> +
> +            p = ngx_strlcasestrn(p, last, (u_char *) "max-age=", 8 - 1);
> +
> +            if (p == NULL) {
> +                break;
> +            }
> +
> +            n = 0;
> +
> +            for (p += 8; p < last; p++) {
> +                if (*p == ',' || *p == ';' || *p == ' ') {
> +                    break;
> +                }
> +
> +                if (*p >= '0' && *p <= '9') {
> +                    n = n * 10 + *p - '0';
> +                    continue;
> +                }
> +
> +                u->cacheable = 0;
> +                return;
> +            }
> +
> +            if (n == 0) {
> +                u->cacheable = 0;
> +                return;
> +            }
> +            r->cache->valid_sec = ngx_max(r->cache->valid_sec, ngx_time() + n);

The ngx_max() seems to be only meaningful if there are more than one 
max-age value found in different Cache-Control headers returned by 
a backend.

It is however inconsistent with other code in the function.  E.g.,

    Cache-Control: max-age=10, max-age=20

will result in 10 seconds max-age, while

    Cache-Control: max-age=10
    Cache-Control: max-age=20

will result in 20 seconds max-age.  Moreover, in contrast to the 
previous example, 

    Cache-Control: max-age=0
    Cache-Control: max-age=20

will result in a response being non-cacheable.

> +        }
> +    }
> +    if (!(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_EXPIRES))
> +    {
> +        time_t  expires;
> +
> +        h = u->headers_in.expires;
> +        if (h)
> +        {
> +            expires = ngx_http_parse_time(h->value.data, h->value.len);
> +            if (expires == NGX_ERROR || expires < ngx_time()) {
> +                u->cacheable = 0;
> +                return;
> +            }
> +            r->cache->valid_sec = ngx_max(expires, r->cache->valid_sec);

This is incorrect and contradicts to RFC2616, see above.

> +        }
> +    }
> +
> +    h = u->headers_in.age;
> +    if (h && r->cache->valid_sec)
> +    {
> +        n = ngx_atoi(h->value.data, h->value.len);
> +        if (n > 0 && n != NGX_ERROR)
> +            r->cache->valid_sec -= n;
> +    }

This will corrupt absolute times parsed from the Expires header.  
Additionally, it may result in a valid_sec in the past, which is 
not checked.

[...]


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