[PATCH] Upstream: fix the cache duration calculation

[Florent Le Coz]

Hi,

On 11/15/2013 01:44 PM, Maxim Dounin wrote:
[…]
 >
 > Perfectly correct solution would be to store a bit (likely in
 > u->headers_in) to indicate that valid_sec was set based on
 > X-Accel-Expires and shouldn't be overwritten.
 >

Since there are, in nginx, three headers that can modify the value of 
valid_sec (Expires, Cache-Control and Expires), I think it would be 
cleaner to define a priority for each of these headers and to use that 
priority to decide if we modify or not the valid_sec value.

That’s what I’ve done in the attached patch.
When setting the value of valid_sec, each header writes its own priority 
in valid_sec_prio.
When processing an other header, instead of checking if the valid_sec is 
already set, we check if the headers’ priority is higher than the one 
set, before setting (or not) the value found in the header being processed.

I’ve set the priorities as: X-Accel-Expires > Cache-Control > Expires.
I’m not sure about what the priority of X-Accel-Expires should be (but 
the last two are well defined in the RFC as you correctly pointed in a 
previous message).

 > I'm not sure it worth the effort though.

I personally think it’s much better to have a well-defined priority 
between headers modifying the same value, but well…

Regards,

-- 
Florent Le Coz
Smartjog


-------------- next part --------------
# HG changeset patch
# User Florent Le Coz <florent.lecoz at smartjog.com>
# Date 1384527955 0
# Node ID c26efc188ac84a8b8f2bb4478efb795635a3498d
# Parent  cbb9a6c7493c3c01323fbc4a61be4a9f0af55ef2
Upstream: implement a priority for headers modifying the cache behaviour

Instead of setting the cache duration (or disabling it) based on the order
in which the headers (X-Accel-Expires, Expires, Cache-Control) are received,
each of these header now has a priority that we use to determine which
header should be trusted.

diff -r cbb9a6c7493c -r c26efc188ac8 src/http/ngx_http_upstream.c
--- a/src/http/ngx_http_upstream.c	Mon Nov 11 18:49:35 2013 +0400
+++ b/src/http/ngx_http_upstream.c	Fri Nov 15 15:05:55 2013 +0000
@@ -3599,7 +3599,7 @@ ngx_http_upstream_process_cache_control(
         return NGX_OK;
     }
 
-    if (r->cache->valid_sec != 0) {
+    if (u->headers_in.valid_sec_prio >= NGX_HTTP_UPSTREAM_CACHE_CONTROL_H_P) {
         return NGX_OK;
     }
 
@@ -3642,6 +3642,7 @@ ngx_http_upstream_process_cache_control(
     }
 
     r->cache->valid_sec = ngx_time() + n;
+    u->headers_in.valid_sec_prio = NGX_HTTP_UPSTREAM_CACHE_CONTROL_H_P;
     }
 #endif
 
@@ -3674,6 +3675,10 @@ ngx_http_upstream_process_expires(ngx_ht
         return NGX_OK;
     }
 
+    if (u->headers_in.valid_sec_prio >= NGX_HTTP_UPSTREAM_EXPIRES_H_P) {
+        return NGX_OK;
+    }
+
     expires = ngx_http_parse_time(h->value.data, h->value.len);
 
     if (expires == NGX_ERROR || expires < ngx_time()) {
@@ -3682,6 +3687,7 @@ ngx_http_upstream_process_expires(ngx_ht
     }
 
     r->cache->valid_sec = expires;
+    u->headers_in.valid_sec_prio = NGX_HTTP_UPSTREAM_EXPIRES_H_P;
     }
 #endif
 
@@ -3728,6 +3734,7 @@ ngx_http_upstream_process_accel_expires(
 
         default:
             r->cache->valid_sec = ngx_time() + n;
+            u->headers_in.valid_sec_prio = NGX_HTTP_UPSTREAM_XA_EXPIRES_H_P;
             return NGX_OK;
         }
     }
@@ -3739,6 +3746,7 @@ ngx_http_upstream_process_accel_expires(
 
     if (n != NGX_ERROR) {
         r->cache->valid_sec = n;
+        u->headers_in.valid_sec_prio = NGX_HTTP_UPSTREAM_XA_EXPIRES_H_P;
     }
     }
 #endif
diff -r cbb9a6c7493c -r c26efc188ac8 src/http/ngx_http_upstream.h
--- a/src/http/ngx_http_upstream.h	Mon Nov 11 18:49:35 2013 +0400
+++ b/src/http/ngx_http_upstream.h	Fri Nov 15 15:05:55 2013 +0000
@@ -105,6 +105,10 @@ typedef struct {
 #define NGX_HTTP_UPSTREAM_DOWN          0x0010
 #define NGX_HTTP_UPSTREAM_BACKUP        0x0020
 
+/* Headers priority to control the cache behaviour */
+#define NGX_HTTP_UPSTREAM_EXPIRES_H_P         1
+#define NGX_HTTP_UPSTREAM_CACHE_CONTROL_H_P   2
+#define NGX_HTTP_UPSTREAM_XA_EXPIRES_H_P      3
 
 struct ngx_http_upstream_srv_conf_s {
     ngx_http_upstream_peer_t         peer;
@@ -245,6 +249,13 @@ typedef struct {
 
     unsigned                         connection_close:1;
     unsigned                         chunked:1;
+#if (NGX_HTTP_CACHE)
+    /* Defines the priority (see NGX_HTTP_UPSTREAM_*_H_P) of the
+     ngx_http_cache_t.valid_sec value currently set.  It is used to decide
+     if a new header should overwrite this value due to the priority of this
+     header being higher than the one currently set. */
+    unsigned                         valid_sec_prio:2;
+#endif
 } ngx_http_upstream_headers_in_t;