Hi Yugo!
I will disagree with your comment. If X-Accel-Expires has value 0 then
r->cacheable will be zeroed and no caching will occur anyway and the valid_sec
will never be checked. Overall the patch looks like a simplified version of what
we already had, which is great!
Best wishes,
Vadim
On 25.04.2022 13:27, Yugo Horie wrote:
Hello,
I appriciate deeply with two of your cooperation.
Maxim's patch looks good to see but it seems to be a bit weird about the
behavior of the r->cache->valid_sec.
In case of the `X-Accel-Expires: 0` header comes first than the `Cache-Control:
max-age=10` header, the r->cache->valid_sec could not be overwritten by the
process_accel_expires which has a weird switch (if it is case 0, it would be
fall through and return NGX_OK) statements, and then it would be overwritten by
the process_cache_control(thus Cache-Control:max-age). In other word, it cannot
kick out the max-age parsing with your brand new extension flag because it
cannot clear if statements of r->cache-valid_sec !=0.
In contrasts, when the Cache-Control is first, it would be overwritten by
X-Accel-Expires. I consider this is the right behavior.
Thanks,
Yugo Horie
On Mon, Apr 25, 2022 at 0:43 Maxim Dounin <mdou...@mdounin.ru
<mailto:mdou...@mdounin.ru>> 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 <mdou...@mdounin.ru <mailto:mdou...@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;
}
}
#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;
--
Maxim Dounin
http://mdounin.ru/ <http://mdounin.ru/>
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
<mailto:nginx-devel@nginx.org>
To unsubscribe send an email to nginx-devel-le...@nginx.org
<mailto:nginx-devel-le...@nginx.org>
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-le...@nginx.org
