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

[Maxim Dounin]

Hello!

On Mon, Nov 24, 2014 at 07:27:53PM -0800, Piotr Sikora wrote:

> # HG changeset patch
> # User Piotr Sikora <piotr at cloudflare.com>
> # Date 1416886025 28800
> #      Mon Nov 24 19:27:05 2014 -0800
> # Node ID 01f07fc7932b64f261c9e6cb778c87279fabcde2
> # Parent  2c10db908b8c4a9c0532c58830275d5ad84ae686
> Cache: don't update cache if revalidated response is not cacheable.
> 
> Signed-off-by: Piotr Sikora <piotr at cloudflare.com>
> 
> diff -r 2c10db908b8c -r 01f07fc7932b src/http/ngx_http_upstream.c
> --- a/src/http/ngx_http_upstream.c	Fri Nov 21 22:51:49 2014 +0300
> +++ b/src/http/ngx_http_upstream.c	Mon Nov 24 19:27:05 2014 -0800
> @@ -2002,14 +2002,13 @@ ngx_http_upstream_test_next(ngx_http_req
>          && u->cache_status == NGX_HTTP_CACHE_EXPIRED
>          && u->conf->cache_revalidate)
>      {
> -        time_t     now, valid;
> +        time_t     valid;
>          ngx_int_t  rc;
>  
>          ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
>                         "http upstream not modified");
>  
> -        now = ngx_time();
> -        valid = r->cache->valid_sec;
> +        valid = u->cacheable ? r->cache->valid_sec : 0;
>  
>          rc = u->reinit_request(r);
>  
> @@ -2021,25 +2020,31 @@ ngx_http_upstream_test_next(ngx_http_req
>          u->cache_status = NGX_HTTP_CACHE_REVALIDATED;
>          rc = ngx_http_upstream_cache_send(r, u);
>  
> -        if (valid == 0) {
> -            valid = r->cache->valid_sec;
> -        }
> -
> -        if (valid == 0) {
> -            valid = ngx_http_file_cache_valid(u->conf->cache_valid,
> -                                              u->headers_in.status_n);
> +        if (u->cacheable || valid) {

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.

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.

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.

> +            time_t  now;

Please do not put declarations inside blocks, with the exception 
of dedicated conditionally compiled blocks.

> +
> +            now = ngx_time();
> +
> +            if (valid == 0) {
> +                valid = r->cache->valid_sec;
> +            }
> +
> +            if (valid == 0) {
> +                valid = ngx_http_file_cache_valid(u->conf->cache_valid,
> +                                                  u->headers_in.status_n);
> +                if (valid) {
> +                    valid = now + valid;
> +                }
> +            }
> +
>              if (valid) {
> -                valid = now + valid;
> +                r->cache->valid_sec = valid;
> +                r->cache->date = now;
> +
> +                ngx_http_file_cache_update_header(r);
>              }
>          }
>  
> -        if (valid) {
> -            r->cache->valid_sec = valid;
> -            r->cache->date = now;
> -
> -            ngx_http_file_cache_update_header(r);
> -        }
> -
>          ngx_http_upstream_finalize_request(r, u, rc);
>          return NGX_OK;
>      }
> 
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel

-- 
Maxim Dounin
http://nginx.org/