<div><div style="border-color:rgb(255,255,255)" dir="auto">Hello, <br></div><div style="border-color:rgb(255,255,255)" dir="auto">I appriciate deeply with two of your cooperation.</div><div style="border-color:rgb(255,255,255)" dir="auto"><br></div><div style="border-color:rgb(255,255,255)" dir="auto">Maxim's patch looks good to see but it seems to be a bit weird about the behavior of the r->cache->valid_sec.</div><div style="border-color:rgb(255,255,255)" dir="auto">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.</div><div style="border-color:rgb(255,255,255)" dir="auto"><br></div><div style="border-color:rgb(255,255,255)" dir="auto">In contrasts, when the Cache-Control is first, it would be overwritten by X-Accel-Expires. I consider this is the right behavior.</div><div style="border-color:rgb(255,255,255)" dir="auto"><br></div><div style="border-color:rgb(255,255,255)" dir="auto">Thanks,</div><div style="border-color:rgb(255,255,255)" dir="auto">Yugo Horie</div></div><div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Apr 25, 2022 at 0:43 Maxim Dounin <<a href="mailto:mdounin@mdounin.ru">mdounin@mdounin.ru</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;padding-left:1ex;border-left-color:rgb(204,204,204)">Hello!<br>
<br>
On Sun, Apr 24, 2022 at 07:55:17AM +0300, Maxim Dounin wrote:<br>
<br>
[...]<br>
<br>
> As far as I can tell, proper behaviour, assuming we parse cache <br>
> control extensions independently of X-Accel-Expires, can be <br>
> implemented by using just one flag.<br>
<br>
No, that's wrong, as with just one flag it wouldn't be possible to <br>
correctly disable caching of responses with:<br>
<br>
Cache-Control: private<br>
Cache-Control: max-age=10<br>
<br>
So it needs at least two flags. Updated patch below, review and <br>
testing appreciated.<br>
<br>
# HG changeset patch<br>
# User Maxim Dounin <<a href="mailto:mdounin@mdounin.ru" target="_blank">mdounin@mdounin.ru</a>><br>
# Date 1650814681 -10800<br>
# Sun Apr 24 18:38:01 2022 +0300<br>
# Node ID 940ba4317a97c72d1ee6700cbf58a543fee04c7a<br>
# Parent a736a7a613ea6e182ff86fbadcb98bb0f8891c0b<br>
Upstream: fixed X-Accel-Expires/Cache-Control/Expires handling.<br>
<br>
Previously, if caching was disabled due to Expires in the past, nginx<br>
failed to cache the response even if it was cacheable as per subsequently<br>
parsed Cache-Control header (ticket #964).<br>
<br>
Similarly, if caching was disabled due to Expires in the past,<br>
"Cache-Control: no-cache" or "Cache-Control: max-age=0", caching was not<br>
used if it was cacheable as per subsequently parsed X-Accel-Expires header.<br>
<br>
Fix is to avoid disabling caching immediately after parsing Expires in<br>
the past or Cache-Control, but rather set flags which are later checked by<br>
ngx_http_upstream_process_headers() (and cleared by "Cache-Control: max-age"<br>
and X-Accel-Expires).<br>
<br>
Additionally, now X-Accel-Expires does not prevent parsing of cache control<br>
extensions, notably stale-while-revalidate and stale-if-error. This<br>
ensures that order of the X-Accel-Expires and Cache-Control headers is not<br>
important.<br>
<br>
Prodded by Vadim Fedorenko and Yugo Horie.<br>
<br>
diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c<br>
--- a/src/http/ngx_http_upstream.c<br>
+++ b/src/http/ngx_http_upstream.c<br>
@@ -2697,6 +2697,10 @@ ngx_http_upstream_intercept_errors(ngx_h<br>
<br>
if (r->cache) {<br>
<br>
+ if (u->headers_in.no_cache || u->headers_in.expired) {<br>
+ u->cacheable = 0;<br>
+ }<br>
+<br>
if (u->cacheable) {<br>
time_t valid;<br>
<br>
@@ -2791,6 +2795,10 @@ ngx_http_upstream_process_headers(ngx_ht<br>
<br>
umcf = ngx_http_get_module_main_conf(r, ngx_http_upstream_module);<br>
<br>
+ if (u->headers_in.no_cache || u->headers_in.expired) {<br>
+ u->cacheable = 0;<br>
+ }<br>
+<br>
if (u->headers_in.x_accel_redirect<br>
&& !(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_XA_REDIRECT))<br>
{<br>
@@ -4735,18 +4743,18 @@ ngx_http_upstream_process_cache_control(<br>
return NGX_OK;<br>
}<br>
<br>
- if (r->cache->valid_sec != 0 && u->headers_in.x_accel_expires != NULL) {<br>
- return NGX_OK;<br>
- }<br>
-<br>
start = h->value.data;<br>
last = start + h->value.len;<br>
<br>
+ if (r->cache->valid_sec != 0 && u->headers_in.x_accel_expires != NULL) {<br>
+ goto extensions;<br>
+ }<br>
+<br>
if (ngx_strlcasestrn(start, last, (u_char *) "no-cache", 8 - 1) != NULL<br>
|| ngx_strlcasestrn(start, last, (u_char *) "no-store", 8 - 1) != NULL<br>
|| ngx_strlcasestrn(start, last, (u_char *) "private", 7 - 1) != NULL)<br>
{<br>
- u->cacheable = 0;<br>
+ u->headers_in.no_cache = 1;<br>
return NGX_OK;<br>
}<br>
<br>
@@ -4776,12 +4784,15 @@ ngx_http_upstream_process_cache_control(<br>
}<br>
<br>
if (n == 0) {<br>
- u->cacheable = 0;<br>
+ u->headers_in.no_cache = 1;<br>
return NGX_OK;<br>
}<br>
<br>
r->cache->valid_sec = ngx_time() + n;<br>
- }<br>
+ u->headers_in.expired = 0;<br>
+ }<br>
+<br>
+extensions:<br>
<br>
p = ngx_strlcasestrn(start, last, (u_char *) "stale-while-revalidate=",<br>
23 - 1);<br>
@@ -4863,7 +4874,7 @@ ngx_http_upstream_process_expires(ngx_ht<br>
expires = ngx_parse_http_time(h->value.data, h->value.len);<br>
<br>
if (expires == NGX_ERROR || expires < ngx_time()) {<br>
- u->cacheable = 0;<br>
+ u->headers_in.expired = 1;<br>
return NGX_OK;<br>
}<br>
<br>
@@ -4914,6 +4925,8 @@ ngx_http_upstream_process_accel_expires(<br>
<br>
default:<br>
r->cache->valid_sec = ngx_time() + n;<br>
+ u->headers_in.no_cache = 0;<br>
+ u->headers_in.expired = 0;<br>
return NGX_OK;<br>
}<br>
}<br>
@@ -4925,6 +4938,8 @@ ngx_http_upstream_process_accel_expires(<br>
<br>
if (n != NGX_ERROR) {<br>
r->cache->valid_sec = n;<br>
+ u->headers_in.no_cache = 0;<br>
+ u->headers_in.expired = 0;<br>
}<br>
}<br>
#endif<br>
diff --git a/src/http/ngx_http_upstream.h b/src/http/ngx_http_upstream.h<br>
--- a/src/http/ngx_http_upstream.h<br>
+++ b/src/http/ngx_http_upstream.h<br>
@@ -297,6 +297,8 @@ typedef struct {<br>
<br>
unsigned connection_close:1;<br>
unsigned chunked:1;<br>
+ unsigned no_cache:1;<br>
+ unsigned expired:1;<br>
} ngx_http_upstream_headers_in_t;<br>
<br>
<br>
<br>
-- <br>
Maxim Dounin<br>
<a href="http://mdounin.ru/" rel="noreferrer" target="_blank">http://mdounin.ru/</a><br>
_______________________________________________<br>
nginx-devel mailing list -- <a href="mailto:nginx-devel@nginx.org" target="_blank">nginx-devel@nginx.org</a><br>
To unsubscribe send an email to <a href="mailto:nginx-devel-leave@nginx.org" target="_blank">nginx-devel-leave@nginx.org</a><br>
</blockquote></div></div>