Possible error on revalidate in ngx_http_upstream

[Maxim Dounin]

Hello!

On Fri, Oct 15, 2021 at 12:14:38PM +0200, Jiří Setnička wrote:

> Hello!
> 
> Thanks for your reply. I didn't realize the implications arising from 
> the RFC mentioned.
> 
> >> In ngx_http_upstream_test_next(...) in revalidate part there is firstly
> >> cache time from upstream 304 response saved to temporal variable (valid
> >> = r->cache->valid_sec) and then request is reinited and
> >> r->cache->valid_sec is set according to headers in the cached file.
> >> Problem is when value == 0 (no caching info from upstream) and there is
> >> an absolute time in the cached file headers.
> >>
> >> This patch should fix this behavior - time computed from cached file is
> >> used only when it is in the future otherwise, time calculated by
> >> ngx_http_file_cache_valid(...) is used.
> > As long as Expires is in the past, r->cache->valid_sec is not set
> > and remains 0, see ngx_http_upstream_process_expires().  As such,
> > suggested patch is a nop as long as standard Expires and
> > Cache-Control headers are used: nginx will ignore Expires from the
> > original response automatically, and will use proxy_cache_valid
> > instead.
> 
> As you mentioned below, I deal with the X-Accel-Expires header. I didn't 
> explicitly check ngx_http_upstream_process_expires() and I thought that 
> the behavior is similar as X-Accel-Expires, sorry for that.
> 
> > Are you trying to address X-Accel-Expires with an absolute time in
> > the past?  Note that it is known to be specifically used to
> > achieve the "revalidate on each request" behaviour, and the
> > suggested change will break this.  (Also, changing the
> > X-Accel-Expires behaviour is better to be done in
> > ngx_http_upstream_process_accel_expires(), rather than indirectly,
> > in 304 response handling code.)
> 
> Ok I will look into implementation in 
> ngx_http_upstream_process_accel_expires().
> 
> Is "revalidate on each request" behaviour intended as the right one, or 
> it is considered as a hack because there is no other way to do 
> "revalidate on each request"? I did not find it in any documentation, 
> only in some email threads and tickets [1].
> 
> Would you be interested in the updated patch or should I patch it only 
> locally for my own usecase?

I don't think that X-Accel-Expires behaviour should be changed: 
while not really documented[*], it is known to be used as it is 
now, and currently there are no other ways to request revalidation 
on each request.

[*] The only X-Accel-Expires documentation I'm aware of is in the 
original mod_accel docs (http://sysoev.ru/mod_accel/readme.html), 
and it only accepted relative time at that time.

-- 
Maxim Dounin
http://mdounin.ru/