Welcome! Log In Create A New Profile

Advanced

Re: [PATCH] Upstream: prioritise Cache-Control over Expires

Sergey Kandaurov
June 06, 2022 09:44AM
On Sun, Apr 24, 2022 at 06:42:47PM +0300, Maxim Dounin wrote:
> Hello!
>
> On Sun, Apr 24, 2022 at 07:55:17AM +0300, Maxim Dounin wrote:
>
> [...]
>
> > As far as I can tell, proper behaviour, assuming we parse cache
> > control extensions independently of X-Accel-Expires, can be
> > implemented by using just one flag.
>
> No, that's wrong, as with just one flag it wouldn't be possible to
> correctly disable caching of responses with:
>
> Cache-Control: private
> Cache-Control: max-age=10
>
> So it needs at least two flags. Updated patch below, review and
> testing appreciated.
>
> # HG changeset patch
> # User Maxim Dounin <mdounin@mdounin.ru>
> # Date 1650814681 -10800
> # Sun Apr 24 18:38:01 2022 +0300
> # Node ID 940ba4317a97c72d1ee6700cbf58a543fee04c7a
> # Parent a736a7a613ea6e182ff86fbadcb98bb0f8891c0b
> Upstream: fixed X-Accel-Expires/Cache-Control/Expires handling.
>
> Previously, if caching was disabled due to Expires in the past, nginx
> failed to cache the response even if it was cacheable as per subsequently
> parsed Cache-Control header (ticket #964).
>
> Similarly, if caching was disabled due to Expires in the past,
> "Cache-Control: no-cache" or "Cache-Control: max-age=0", caching was not
> used if it was cacheable as per subsequently parsed X-Accel-Expires header.
>
> Fix is to avoid disabling caching immediately after parsing Expires in
> the past or Cache-Control, but rather set flags which are later checked by
> ngx_http_upstream_process_headers() (and cleared by "Cache-Control: max-age"
> and X-Accel-Expires).
>
> Additionally, now X-Accel-Expires does not prevent parsing of cache control
> extensions, notably stale-while-revalidate and stale-if-error. This
> ensures that order of the X-Accel-Expires and Cache-Control headers is not
> important.
>
> Prodded by Vadim Fedorenko and Yugo Horie.
>
> diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
> --- a/src/http/ngx_http_upstream.c
> +++ b/src/http/ngx_http_upstream.c
> @@ -2697,6 +2697,10 @@ ngx_http_upstream_intercept_errors(ngx_h
>
> if (r->cache) {
>
> + if (u->headers_in.no_cache || u->headers_in.expired) {
> + u->cacheable = 0;
> + }
> +
> if (u->cacheable) {
> time_t valid;
>
> @@ -2791,6 +2795,10 @@ ngx_http_upstream_process_headers(ngx_ht
>
> umcf = ngx_http_get_module_main_conf(r, ngx_http_upstream_module);
>
> + if (u->headers_in.no_cache || u->headers_in.expired) {
> + u->cacheable = 0;
> + }
> +
> if (u->headers_in.x_accel_redirect
> && !(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_XA_REDIRECT))
> {
> @@ -4735,18 +4743,18 @@ ngx_http_upstream_process_cache_control(
> return NGX_OK;
> }
>
> - if (r->cache->valid_sec != 0 && u->headers_in.x_accel_expires != NULL) {
> - return NGX_OK;
> - }
> -
> start = h->value.data;
> last = start + h->value.len;
>
> + if (r->cache->valid_sec != 0 && u->headers_in.x_accel_expires != NULL) {
> + goto extensions;
> + }
> +
> if (ngx_strlcasestrn(start, last, (u_char *) "no-cache", 8 - 1) != NULL
> || ngx_strlcasestrn(start, last, (u_char *) "no-store", 8 - 1) != NULL
> || ngx_strlcasestrn(start, last, (u_char *) "private", 7 - 1) != NULL)
> {
> - u->cacheable = 0;
> + u->headers_in.no_cache = 1;
> return NGX_OK;
> }
>
> @@ -4776,12 +4784,15 @@ ngx_http_upstream_process_cache_control(
> }
>
> if (n == 0) {
> - u->cacheable = 0;
> + u->headers_in.no_cache = 1;
> return NGX_OK;
> }
>
> r->cache->valid_sec = ngx_time() + n;
> - }
> + u->headers_in.expired = 0;
> + }
> +
> +extensions:
>
> p = ngx_strlcasestrn(start, last, (u_char *) "stale-while-revalidate=",
> 23 - 1);
> @@ -4863,7 +4874,7 @@ ngx_http_upstream_process_expires(ngx_ht
> expires = ngx_parse_http_time(h->value.data, h->value.len);
>
> if (expires == NGX_ERROR || expires < ngx_time()) {
> - u->cacheable = 0;
> + u->headers_in.expired = 1;
> return NGX_OK;
> }
>
> @@ -4914,6 +4925,8 @@ ngx_http_upstream_process_accel_expires(
>
> default:
> r->cache->valid_sec = ngx_time() + n;
> + u->headers_in.no_cache = 0;
> + u->headers_in.expired = 0;
> return NGX_OK;
> }
> }
> @@ -4925,6 +4938,8 @@ ngx_http_upstream_process_accel_expires(
>
> if (n != NGX_ERROR) {
> r->cache->valid_sec = n;
> + u->headers_in.no_cache = 0;
> + u->headers_in.expired = 0;

Handling of "X-Accel-Redirect: @0" is still missed. Per the docs,
it should set caching time to Epoch resulting in the expired response.
Instead, such header is handled different, and it can be overridden
with Cache-Control/Expires headers. Probably, it should be handled
separately and disable caching, similar to "X-Accel-Redirect: 0".

As the above note covers a corner case of X-Accel-Expires,
I believe it's fine to commit as is.

> }
> }
> #endif
> diff --git a/src/http/ngx_http_upstream.h b/src/http/ngx_http_upstream.h
> --- a/src/http/ngx_http_upstream.h
> +++ b/src/http/ngx_http_upstream.h
> @@ -297,6 +297,8 @@ typedef struct {
>
> unsigned connection_close:1;
> unsigned chunked:1;
> + unsigned no_cache:1;
> + unsigned expired:1;
> } ngx_http_upstream_headers_in_t;
>
>
>
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org
Subject Author Views Posted

[PATCH] Upstream: prioritise Cache-Control over Expires

Vadim Fedorenko via nginx-devel 1165 April 14, 2022 07:04PM

Re: [PATCH] Upstream: prioritise Cache-Control over Expires

Maxim Dounin 214 April 16, 2022 09:56PM

Re: [PATCH] Upstream: prioritise Cache-Control over Expires

Vadim Fedorenko via nginx-devel 235 April 17, 2022 03:52PM

Re: [PATCH] Upstream: prioritise Cache-Control over Expires

yugo-horie 287 April 17, 2022 09:16PM

Re: [PATCH] Upstream: prioritise Cache-Control over Expires

Vadim Fedorenko via nginx-devel 257 April 18, 2022 06:22AM

Re: [PATCH] Upstream: prioritise Cache-Control over Expires

yugo-horie 245 April 18, 2022 07:04PM

Re: [PATCH] Upstream: prioritise Cache-Control over Expires

Maxim Dounin 323 April 19, 2022 11:02AM

Re: [PATCH] Upstream: prioritise Cache-Control over Expires

Maxim Dounin 180 April 22, 2022 02:24PM

Re: [PATCH] Upstream: prioritise Cache-Control over Expires

Vadim Fedorenko via nginx-devel 299 April 22, 2022 03:08PM

Re: [PATCH] Upstream: prioritise Cache-Control over Expires

Maxim Dounin 208 April 24, 2022 12:56AM

Re: [PATCH] Upstream: prioritise Cache-Control over Expires

Maxim Dounin 191 April 24, 2022 11:44AM

Re: [PATCH] Upstream: prioritise Cache-Control over Expires

yugo-horie 237 April 25, 2022 08:28AM

Re: [PATCH] Upstream: prioritise Cache-Control over Expires

Vadim Fedorenko via nginx-devel 254 April 25, 2022 05:04PM

Re: [PATCH] Upstream: prioritise Cache-Control over Expires

Vadim Fedorenko via nginx-devel 227 April 25, 2022 05:08PM

Re: [PATCH] Upstream: prioritise Cache-Control over Expires

Sergey Kandaurov 188 June 06, 2022 09:44AM

Re: [PATCH] Upstream: prioritise Cache-Control over Expires

Sergey Kandaurov 174 June 06, 2022 10:42AM

Re: [PATCH] Upstream: prioritise Cache-Control over Expires

Maxim Dounin 186 June 06, 2022 05:10PM

Re: [PATCH] Upstream: prioritise Cache-Control over Expires

Sergey Kandaurov 217 June 07, 2022 12:02PM

Re: [PATCH] Upstream: prioritise Cache-Control over Expires

Maxim Dounin 237 June 07, 2022 09:00PM

Re: [PATCH] Upstream: prioritise Cache-Control over Expires

Sergey Kandaurov 204 June 10, 2022 07:40AM

Re: [PATCH] Upstream: prioritise Cache-Control over Expires

Maxim Dounin 182 June 11, 2022 06:54PM

Re: [PATCH] Upstream: prioritise Cache-Control over Expires

Sergey Kandaurov 219 June 13, 2022 07:42AM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 180
Record Number of Users: 8 on April 13, 2023
Record Number of Guests: 500 on July 15, 2024
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready