[PATCH] Upstream: prioritise Cache-Control over Expires

Maxim Dounin mdounin at mdounin.ru
Sun Apr 24 04:55:17 UTC 2022


Hello!

On Fri, Apr 22, 2022 at 08:06:08PM +0100, Vadim Fedorenko via nginx-devel wrote:

> Hi Maxim!
> 
> Thanks for the feedback. As you might have already seen, I sent another version
> of this patch which addresses some of style and readability issues as well as
> X-Accel-Expires issue in v3.

Ah, sorry, I've missed that you've removed the 
"u->headers_in.x_accel_expires != NULL" check in the 
ngx_http_upstream_process_cache_control().  So the 
stale-while-revalidate and stale-if-error Cache-Control directive 
are always applied now, whenever X-Accel-Expires is present or 
not.

Not sure this is a good approach: simply ignoring Cache-Control if 
X-Accel-Expires is present might be a better and easier to 
understand solution.

The basic idea behind X-Accel-Expires is that site administrators 
might want to provide distinct caching parameters to caches they 
control (and can purge or force-update when needed), and these 
caching parameters are different from caching parameters sent to 
external caches.  Still using some caching parameters from 
Cache-Control does not seem to align well with this usage.

On the other hand, as per RFC 2616/7234, the Expires header is 
only expected to be ignored when "Cache-Control: max-age" is 
present, but not other Cache-Control directives.  So probably you 
are right and this is correct approach to use.

> Regarding RFC issue. RFC 7234 Section 5.1 explicitly states that Expires MUST be
> ignored if reponse contains Cache-Control header with max-age or s-maxage
> directives. That's why I said that Nginx server configured to cache responses
> and to rely on headers to determine the freshness violates RFC. And this is true
> for a subset of absolutely legitimate responses, and it was done intended to
> actually optimize the internal processing but not because of configuration
> optimizations.

Sure.  Though it is also completely legitimate to don't cache a 
response, regardless of the particular headers.  Switching of 
caching for any reason is generally safe, and makes further RFC 
7234 requirements void.

Rather, I would say that nginx violates RFC by caching a response 
based on "Cache-Control: max-age" when there is an Expires header 
which prevents caching, as nginx does not support the Age header 
and therefore can incorrectly cache an expired response.

> Hope you will have some time to review version 3 of my patch even though it
> might not clearly apply on top of the patchset you sent couple of days ago.

Unfortunately, style and readability issues are still there.  
Review below.

(Note that it might be a good idea to keep patches in a single 
thread.  When sending with "hg email", the "--in-reply-to" option 
ensures proper threading.)

: # HG changeset patch
: # User Vadim Fedorenko <vadim.fedorenko at cdnnow.ru>
: # Date 1650406016 -10800
: #      Wed Apr 20 01:06:56 2022 +0300
: # Node ID e04dac22328020cf8d8abcc4863b982b513b0c80
: # Parent  a736a7a613ea6e182ff86fbadcb98bb0f8891c0b
: Upstream: prioritise X-Accel-Expire over Cache-Control and Expire

Note missing "s" in "Expires".
Style: should be a trailing dot.

: 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.

Note that this is an inaccurate and misleading statement: 
Cache-Control wasn't ignored.  The only specific case to address 
is when the Expires header comes first and disables caching.

: At the same time documentation states that there is special header
: X-Accel-Expires that controls the behavior of nginx and should be
: prioritized over other cache-specific headers and this patch
: implements this priority.

Again, the X-Accel-Expires header is already prioritized over any 
other cache control headers.

: More informantion could be found in ticket #964.

Typo: "information".

References in the "(ticket #964)" form are automatically linked in 
Trac and generally recommended unless there are specific reasons 
to avoid linking.

: ---
:  src/http/ngx_http_upstream.c | 62 +++++++++++++++++++++++++-----------
:  src/http/ngx_http_upstream.h |  6 ++++
:  2 files changed, 50 insertions(+), 18 deletions(-)
: 
: diff -r a736a7a613ea -r e04dac223280 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	Wed Apr 20 01:06:56 2022 +0300
: @@ -2350,6 +2350,32 @@
:  
:  
:  static void
: +ngx_http_upstream_validate_cache_headers(ngx_http_request_t *r, ngx_http_upstream_t *u)
: +{
: +    ngx_http_upstream_headers_in_t *uh = &u->headers_in;
: +    if (uh->x_accel_expires != NULL  &&
: +        !(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_XA_EXPIRES)) {

Style issues: more than 80 characters in a line, no empty line 
between variable definition and code, less than two spaces between 
type and variable name, variable initialization should be on a 
separate line, two spaces before "&&", operator should be on the 
continuation line, "{" should be on its own line, no function 
declaration, function definition should follow first usage unless 
there are specific reasons to place it elsewhere.

Also, we don't generally use ngx_http_upstream_headers_in_t local 
variables, but rather use direct references via u->headers_in.

: +        u->cacheable = uh->x_accel_expires_c;
: +        r->cache->valid_sec = uh->x_accel_expires_n;

This uses r->cache without appropriate conditional compilation 
directives, so will result in compilation errors without HTTP 
cache compiled in ("./configure --without-http-cache").

: +        return;
: +    }
: +
: +    if (uh->cache_control.elts != NULL &&
: +        !(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_CACHE_CONTROL)) {
: +        u->cacheable = uh->cache_control_c;
: +        r->cache->valid_sec = uh->cache_control_n;

This seems to be incorrect, given that something like

Cache-Control: public
Expires: <date>

is exactly equivalent to just "Expires: <date>", without 
Cache-Control at all.

: +        return;
: +    }
: +
: +    if (uh->expires != NULL  &&
: +        !(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_EXPIRES)) {
: +        u->cacheable = uh->expires_c;
: +        r->cache->valid_sec = uh->expires_n;
: +    }
: +}
: +
: +
: +static void
:  ngx_http_upstream_process_header(ngx_http_request_t *r, ngx_http_upstream_t *u)
:  {
:      ssize_t            n;
: @@ -2468,6 +2494,11 @@
:  
:              continue;
:          }
: +#if (NGX_HTTP_CACHE)

Conditional compilation here would result in a function which is 
never used without HTTP cache compiled in, and hence "unused 
function" warnings.

: +        if (u->cacheable) {
: +            ngx_http_upstream_validate_cache_headers(r, u);
: +        }
: +#endif

Caching also needs to be properly adjusted somewhere in 
ngx_http_upstream_intercept_errors(), as it does not call 
ngx_http_upstream_process_headers().

:  
:          break;
:      }
: @@ -4735,10 +4766,6 @@
:          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;
:  
: @@ -4746,7 +4773,7 @@
:          || 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.cache_control_c = 0;
:          return NGX_OK;
:      }
:  
: @@ -4771,16 +4798,15 @@
:                  continue;
:              }
:  
: -            u->cacheable = 0;
:              return NGX_OK;
:          }
:  
:          if (n == 0) {
: -            u->cacheable = 0;
:              return NGX_OK;
:          }
:  
: -        r->cache->valid_sec = ngx_time() + n;
: +        u->headers_in.cache_control_c = 1;
: +        u->headers_in.cache_control_n = ngx_time() + n;
:      }
:  
:      p = ngx_strlcasestrn(start, last, (u_char *) "stale-while-revalidate=",
: @@ -4856,18 +4882,18 @@
:          return NGX_OK;
:      }
:  
: -    if (r->cache->valid_sec != 0) {
: +    expires = ngx_parse_http_time(h->value.data, h->value.len);
: +
: +    if (expires == NGX_ERROR || expires < ngx_time()) {
:          return NGX_OK;
:      }
:  
: -    expires = ngx_parse_http_time(h->value.data, h->value.len);
: -
: -    if (expires == NGX_ERROR || expires < ngx_time()) {
: -        u->cacheable = 0;
: -        return NGX_OK;
: -    }
: -
: -    r->cache->valid_sec = expires;
: +    if (u->headers_in.expires_c) {
: +        expires = ngx_min(expires, u->headers_in.expires_n);
: +    }

The patch series recently posted will take care of duplicate 
headers, there is no need to introduce additional ad-hoc 
solutions.

: +    u->headers_in.expires_n = expires;
: +    u->headers_in.expires_c = 1;
: +
:      }
:  #endif
:  
: @@ -4906,14 +4932,12 @@
:  
:          switch (n) {
:          case 0:
: -            u->cacheable = 0;
: -            /* fall through */
: -
:          case NGX_ERROR:
:              return NGX_OK;
:  
:          default:
: -            r->cache->valid_sec = ngx_time() + n;
: +            u->headers_in.x_accel_expires_c = 1;
: +            u->headers_in.x_accel_expires_n = ngx_time() + n;
:              return NGX_OK;
:          }
:      }
: @@ -4924,7 +4948,8 @@
:      n = ngx_atoi(p, len);
:  
:      if (n != NGX_ERROR) {
: -        r->cache->valid_sec = n;
: +            u->headers_in.x_accel_expires_c = 1;
: +            u->headers_in.x_accel_expires_n = ngx_time() + n;
:      }
:      }
:  #endif
: diff -r a736a7a613ea -r e04dac223280 src/http/ngx_http_upstream.h
: --- a/src/http/ngx_http_upstream.h	Tue Feb 08 17:35:27 2022 +0300
: +++ b/src/http/ngx_http_upstream.h	Wed Apr 20 01:06:56 2022 +0300
: @@ -294,9 +294,15 @@
:  
:      off_t                            content_length_n;
:      time_t                           last_modified_time;
: +    ngx_int_t                        cache_control_n;
: +    ngx_int_t                        expires_n;
: +    ngx_int_t                        x_accel_expires_n;
:  
:      unsigned                         connection_close:1;
:      unsigned                         chunked:1;
: +    unsigned                         cache_control_c;
: +    unsigned                         expires_c;
: +    unsigned                         x_accel_expires_c;
:  } ngx_http_upstream_headers_in_t;
:  
:  

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.

Patch below, review and testing appreciated:

# HG changeset patch
# User Maxim Dounin <mdounin at mdounin.ru>
# Date 1650775261 -10800
#      Sun Apr 24 07:41:01 2022 +0300
# Node ID f9c6d561b510e6008bfb4d7989f358dba33b38cd
# 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, but rather set a flag which is 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->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->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.no_cache = 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.no_cache = 1;
         return NGX_OK;
     }
 
@@ -4914,6 +4925,7 @@ ngx_http_upstream_process_accel_expires(
 
         default:
             r->cache->valid_sec = ngx_time() + n;
+            u->headers_in.no_cache = 0;
             return NGX_OK;
         }
     }
@@ -4925,6 +4937,7 @@ ngx_http_upstream_process_accel_expires(
 
     if (n != NGX_ERROR) {
         r->cache->valid_sec = n;
+        u->headers_in.no_cache = 0;
     }
     }
 #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,7 @@ typedef struct {
 
     unsigned                         connection_close:1;
     unsigned                         chunked:1;
+    unsigned                         no_cache:1;
 } ngx_http_upstream_headers_in_t;
 
 
 
-- 
Maxim Dounin
http://mdounin.ru/



More information about the nginx-devel mailing list