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


# HG changeset patch
# User Florent Le Coz <florent.lecoz@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;
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel