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
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
