[PATCH 1 of 2] Cache: don't update cache if revalidated response is not cacheable

[Piotr Sikora]

Hey Maxim,

> I don't really understand the logic you suggest.
>
> Previous behaviour is as follows:
>
> - We don't check if the response is cacheable or not, but rather
>   rely on the fact that it's already in the cache (hence it, at
>   least was, cacheable).
>
> This kind of agree with what nginx normally does for cache items -
> cacheability is determined when we are placing a response into the
> cache, and not changed after this even if settings are changed.

Which is strange, because the 304 response is used to signal validity
and cacheability of the original (cached) response, so ignoring that
feels wrong.

Also, right now, validity, but not cacheability of the 304 response
takes priority over cached response, which means that in most cases,
the decision is based partially on the 304 and partially on the cached
response.

One of the reasons for this change is to avoid requests with
Authorization header and/or responses with Set-Cookie header
revalidating cached responses for anonymous users.

Another, more technical reason, is that this is the only place in the
code where r->cache->valid_sec isn't guarded by u->cacheable, which is
protection assumed by rest of the code that doesn't clear this value
on errors, just sets u->cacheable to 0, so ignoring it can result in
odd validity values that wouldn't be applied originally on cache miss.

> With the suggested change, you do the following:
>
> - If the 304 response is cacheable and there is a validity known in
>   advance (from Expires, Cache-Control or X-Accel-Expires), the
>   response will be cached.
>
> - If the 304 response is cacheable and the cached response is
>   cacheable according to current settings, the response will be
>   cached.

Correct, so the only difference between existing and proposed behavior
is that in case 304 response (which, according to RFC7232, must
include all cache-controlling headers, other than the specific to
nginx X-Accel-Expires) isn't cacheable, nginx won't update the
validity of the cached item. I don't see anything wrong with this
logic.

> This doesn't look consistent.  In particular,
>
> - If the cached response is not cacheable according to current
>   settings, it's still can be cached if the 304 response can be
>   cached and have a validity explicitly set.
>
> Doesn't look like a good change to me.

I don't see how this is different from the current behavior, which is
less strict than the proposed one.

Maybe look at the diff ignoring white spaces? This patch just adds
requirement for 304 to be cacheable, that's it.

>> +            time_t  now;
>
> Please do not put declarations inside blocks, with the exception
> of dedicated conditionally compiled blocks.

I only wrote it like that to be consistent with rest of the code,
where "now" and/or "valid" are defined inside u->cacheable conditional
blocks. See: ngx_http_upstream.c lines 2553 and 3763.

Best regards,
Piotr Sikora