Updating r->cache in ngx_http_upstream_process_cache_control()

[Maxim Dounin]

Hello!

On Fri, Jul 26, 2024 at 01:14:39PM -0700, Maksim Yevmenkin wrote:

> Hello,
> 
> I would like to understand the rationale for updating
> r->cache->valid_sec (and other fields in r->cache) in
> ngx_http_upstream_process_cache_control().
> 
> It appears that ngx_http_upstream_process_cache_control() can be
> called (via ngx_http_proxy_process_header()) in two scenarios: when
> sending a cached response to the client, and when receiving a response
> from the upstream server.
> 
> Obviously, calling ngx_http_upstream_process_cache_control() when
> receiving a response from the upstream server is completely expected.
> However, calling ngx_http_upstream_process_cache_control(), and more
> specifically, modifying r->cache fields while sending a cached
> (NGX_HTTP_CACHE_HIT and possibly others) response is not completely
> clear to me.

Parsing cache validity time from cached responses is used at least 
when sending revalidated responses (NGX_HTTP_CACHE_REVALIDATED).

For cached responses with other cache statuses 
(NGX_HTTP_CACHE_HIT, NGX_HTTP_CACHE_STALE) this probably can be 
avoided, at least with current code, but it is generally safe, and 
avoiding parsing would be at most minor optimization.  Further, 
such an optimization might not be trivial to maintain, since cache 
statuses might be added or changed over time.

In general, though, updating r->cache->valid_sec from headers 
processing functions as currently implemented is wrong, and can 
lead to incorrect results when switching to other upstream servers 
per proxy_next_upstream after parsing some headers.  It should be 
either explicitly cleared in ngx_http_upstream_reinit(), or the 
parsed value should be kept in u->headers_in till actually used 
(so it will be cleared by existing generic code in 
ngx_http_upstream_reinit()).

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