[PATCH] Upstream: prioritise Cache-Control over Expires

[Maxim Dounin]

Hello!

On Fri, Apr 15, 2022 at 02:02:53AM +0300, Vadim Fedorenko via nginx-devel wrote:

> # HG changeset patch
> # User Vadim Fedorenko <vadim.fedorenko at cdnnow.ru>
> # Date 1649889268 -10800
> #      Thu Apr 14 01:34:28 2022 +0300
> # Node ID ed7a2c031475bcb252952a467c184c94652b926a
> # Parent  a736a7a613ea6e182ff86fbadcb98bb0f8891c0b
> Upstream: prioritise Cache-Control over Expires.
> 
> RFC7234 explicitly says that cache recipient MUST ignore Expires
> header if response includes Cache-Control header with max-age or
> s-maxage directives. Previously Cache-Control was ignored if it
> was after Expires in reply headers.
> 
> At the same time this patch makes more stable behaviour of using
> latest value of header Cache-Control even if previous value was 0.
> Ticket #964 for more information.
> ---
>  src/http/ngx_http_upstream.c | 25 +++++++++++++++++++++++++
>  src/http/ngx_http_upstream.h | 15 +++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff -r a736a7a613ea -r ed7a2c031475 src/http/ngx_http_upstream.c
> --- a/src/http/ngx_http_upstream.c	Tue Feb 08 17:35:27 2022 +0300
> +++ b/src/http/ngx_http_upstream.c	Thu Apr 14 01:34:28 2022 +0300
> @@ -868,6 +868,7 @@
>          }
>  
>          u->cacheable = 1;
> +        u->cacheable_reason |= NGX_HTTP_CACHEABLE_DEFAULT;
>  
>          c = r->cache;
>  
> @@ -970,6 +971,7 @@
>      case NGX_HTTP_CACHE_SCARCE:
>  
>          u->cacheable = 0;
> +        u->cacheable_reason |= NGX_HTTP_CACHEABLE_SCARCE;
>  
>          break;
>  
> @@ -992,6 +994,7 @@
>  
>      if (ngx_http_upstream_cache_check_range(r, u) == NGX_DECLINED) {
>          u->cacheable = 0;
> +        u->cacheable_reason |= NGX_HTTP_CACHEABLE_RANGE;
>      }
>  
>      r->cached = 0;
> @@ -3105,6 +3108,7 @@
>  
>      case NGX_DECLINED:
>          u->cacheable = 0;
> +        u->cacheable_reason |= NGX_HTTP_CACHEABLE_NO_CACHE;
>          break;
>  
>      default: /* NGX_OK */
> @@ -3165,6 +3169,7 @@
>  
>          } else {
>              u->cacheable = 0;
> +            u->cacheable_reason |= NGX_HTTP_CACHEABLE_INVALID;
>          }
>      }
>  
> @@ -4690,6 +4695,7 @@
>  #if (NGX_HTTP_CACHE)
>      if (!(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_SET_COOKIE)) {
>          u->cacheable = 0;
> +        u->cacheable_reason |= NGX_HTTP_CACHEABLE_COOKIE;
>      }
>  #endif
>  
> @@ -4747,6 +4753,7 @@
>          || ngx_strlcasestrn(start, last, (u_char *) "private", 7 - 1) != NULL)
>      {
>          u->cacheable = 0;
> +        u->cacheable_reason |= NGX_HTTP_CACHEABLE_NO_CACHE;
>          return NGX_OK;
>      }
>  
> @@ -4772,15 +4779,21 @@
>              }
>  
>              u->cacheable = 0;
> +            u->cacheable_reason |= NGX_HTTP_CACHEABLE_CACHECTRL0;
>              return NGX_OK;
>          }
>  
>          if (n == 0) {
>              u->cacheable = 0;
> +            u->cacheable_reason |= NGX_HTTP_CACHEABLE_CACHECTRL0;
>              return NGX_OK;
>          }
>  
>          r->cache->valid_sec = ngx_time() + n;
> +        u->cacheable_reason &= ~(NGX_HTTP_CACHEABLE_CACHECTRL0|NGX_HTTP_CACHEABLE_EXPIRES);
> +        if (u->cacheable_reason == NGX_HTTP_CACHEABLE_DEFAULT) {
> +            u->cacheable = 1;
> +        }
>      }
>  
>      p = ngx_strlcasestrn(start, last, (u_char *) "stale-while-revalidate=",

[...]

Thanks for the patch.

I'm quite sceptical about attempts to fix this by introducing 
various flags and reverting cacheable status back to 1.  This is 
not how it should be fixed.

-- 
Maxim Dounin
http://mdounin.ru/