> On 6 Jun 2022, at 17:42, Sergey Kandaurov <pluknet@nginx.com> wrote:
>
> On Sun, Apr 24, 2022 at 06:42:47PM +0300, Maxim Dounin 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 <mdounin@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;
>
> Handling of "X-Accel-Redirect: @0" is still missed. Per the docs,
> it should set caching time to Epoch resulting in the expired response.
> Instead, such header is handled different, and it can be overridden
> with Cache-Control/Expires headers. Probably, it should be handled
> separately and disable caching, similar to "X-Accel-Redirect: 0".
>

Of course, this should be read as "X-Accel-Expires", bad copy'n'paste.

> As the above note covers a corner case of X-Accel-Expires,
> I believe it's fine to commit as is.
>
>> }
>> }
>> #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;
>>
>>
>>
> _______________________________________________
> nginx-devel mailing list -- nginx-devel@nginx.org
> To unsubscribe send an email to nginx-devel-leave@nginx.org

--
Sergey Kandaurov

_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org