[PATCH] Upstream: fixed proxy_no_cache when caching errors

Maxim Dounin mdounin at mdounin.ru
Fri Jun 21 01:11:00 UTC 2024


# HG changeset patch
# User Maxim Dounin <mdounin at mdounin.ru>
# Date 1718929130 -10800
#      Fri Jun 21 03:18:50 2024 +0300
# Node ID 0ba8dda4d6833d566b8e429f5f14d3a9ac2a07d5
# Parent  ea0eef2dd12c2d41349d63c532e942cf95fc4d7b
Upstream: fixed proxy_no_cache when caching errors.

Caching errors, notably intercepted errors and internally generated
502/504 errors, as well as handling of cache revalidation with 304,
did not take into account u->conf->no_cache predicates configured.
As a result, an error might be cached even if configuration explicitly
says not to.  Fix is to check u->conf->no_cache in these cases.

To simplify usage in multiple places, checking u->conf->no_cache is now
done in a separate function.  As a minor optimization, u->conf->no_cache
is only checked if u->cacheable is set.

As a side effect, this change also fixes caching errors after
proxy_cache_bypass.  Also, during cache revalidation u->cacheable is
now tested, so 304 responses which disable caching won't extend
cacheability of stored responses.

Additionally, when caching internally generated 502/504 errors
u->cacheable is now explicitly updated from u->headers_in.no_cache and
u->headers_in.expired, restoring the behaviour before 8041:0784ab86ad08
(1.23.0) when an error happens while reading the response headers.

Reported by Kirill A. Korinsky,
https://freenginx.org/pipermail/nginx/2024-April/000082.html

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
@@ -21,6 +21,8 @@ static ngx_int_t ngx_http_upstream_cache
     ngx_http_request_t *r, ngx_http_upstream_t *u);
 static ngx_int_t ngx_http_upstream_cache_check_range(ngx_http_request_t *r,
     ngx_http_upstream_t *u);
+static ngx_int_t ngx_http_upstream_no_cache(ngx_http_request_t *r,
+    ngx_http_upstream_t *u);
 static ngx_int_t ngx_http_upstream_cache_status(ngx_http_request_t *r,
     ngx_http_variable_value_t *v, uintptr_t data);
 static ngx_int_t ngx_http_upstream_cache_key(ngx_http_request_t *r,
@@ -2632,6 +2634,12 @@ ngx_http_upstream_test_next(ngx_http_req
         updating = r->cache->updating_sec;
         error = r->cache->error_sec;
 
+        if (ngx_http_upstream_no_cache(r, u) != NGX_OK) {
+            ngx_http_upstream_finalize_request(r, u,
+                                               NGX_HTTP_INTERNAL_SERVER_ERROR);
+            return NGX_OK;
+        }
+
         rc = u->reinit_request(r);
 
         if (rc != NGX_OK) {
@@ -2650,30 +2658,33 @@ ngx_http_upstream_test_next(ngx_http_req
             rc = NGX_HTTP_INTERNAL_SERVER_ERROR;
         }
 
-        if (valid == 0) {
-            valid = r->cache->valid_sec;
-            updating = r->cache->updating_sec;
-            error = r->cache->error_sec;
-        }
-
-        if (valid == 0) {
-            valid = ngx_http_file_cache_valid(u->conf->cache_valid,
-                                              u->headers_in.status_n);
+        if (u->cacheable) {
+
+            if (valid == 0) {
+                valid = r->cache->valid_sec;
+                updating = r->cache->updating_sec;
+                error = r->cache->error_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->updating_sec = updating;
+                r->cache->error_sec = error;
+
+                r->cache->date = now;
+
+                ngx_http_file_cache_update_header(r);
             }
         }
 
-        if (valid) {
-            r->cache->valid_sec = valid;
-            r->cache->updating_sec = updating;
-            r->cache->error_sec = error;
-
-            r->cache->date = now;
-
-            ngx_http_file_cache_update_header(r);
-        }
-
         ngx_http_upstream_finalize_request(r, u, rc);
         return NGX_OK;
     }
@@ -2745,8 +2756,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 (ngx_http_upstream_no_cache(r, u) != NGX_OK) {
+                    ngx_http_upstream_finalize_request(r, u,
+                                               NGX_HTTP_INTERNAL_SERVER_ERROR);
+                    return NGX_OK;
                 }
 
                 if (u->cacheable) {
@@ -3159,29 +3172,8 @@ ngx_http_upstream_send_response(ngx_http
         r->cache->file.fd = NGX_INVALID_FILE;
     }
 
-    switch (ngx_http_test_predicates(r, u->conf->no_cache)) {
-
-    case NGX_ERROR:
+    if (ngx_http_upstream_no_cache(r, u) != NGX_OK) {
         ngx_http_upstream_finalize_request(r, u, NGX_ERROR);
-        return;
-
-    case NGX_DECLINED:
-        u->cacheable = 0;
-        break;
-
-    default: /* NGX_OK */
-
-        if (u->cache_status == NGX_HTTP_CACHE_BYPASS) {
-
-            /* create cache if previously bypassed */
-
-            if (ngx_http_file_cache_create(r) != NGX_OK) {
-                ngx_http_upstream_finalize_request(r, u, NGX_ERROR);
-                return;
-            }
-        }
-
-        break;
     }
 
     if (u->cacheable) {
@@ -3372,6 +3364,50 @@ ngx_http_upstream_send_response(ngx_http
 }
 
 
+#if (NGX_HTTP_CACHE)
+
+static ngx_int_t
+ngx_http_upstream_no_cache(ngx_http_request_t *r, ngx_http_upstream_t *u)
+{
+    ngx_int_t  rc;
+
+    if (!u->cacheable) {
+        return NGX_OK;
+    }
+
+    if (u->headers_in.no_cache || u->headers_in.expired) {
+        u->cacheable = 0;
+        return NGX_OK;
+    }
+
+    rc = ngx_http_test_predicates(r, u->conf->no_cache);
+
+    if (rc == NGX_ERROR) {
+        return NGX_ERROR;
+    }
+
+    if (rc == NGX_DECLINED) {
+        u->cacheable = 0;
+        return NGX_OK;
+    }
+
+    /* rc == NGX_OK */
+
+    if (u->cache_status == NGX_HTTP_CACHE_BYPASS) {
+
+        /* create cache if previously bypassed */
+
+        if (ngx_http_file_cache_create(r) != NGX_OK) {
+            return NGX_ERROR;
+        }
+    }
+
+    return NGX_OK;
+}
+
+#endif
+
+
 static void
 ngx_http_upstream_upgrade(ngx_http_request_t *r, ngx_http_upstream_t *u)
 {
@@ -4619,9 +4655,15 @@ ngx_http_upstream_finalize_request(ngx_h
 
     if (r->cache) {
 
-        if (u->cacheable) {
-
-            if (rc == NGX_HTTP_BAD_GATEWAY || rc == NGX_HTTP_GATEWAY_TIME_OUT) {
+        if (rc == NGX_HTTP_BAD_GATEWAY || rc == NGX_HTTP_GATEWAY_TIME_OUT) {
+
+            if (!u->header_sent) {
+                if (ngx_http_upstream_no_cache(r, u) != NGX_OK) {
+                    u->cacheable = 0;
+                }
+            }
+
+            if (u->cacheable) {
                 time_t  valid;
 
                 valid = ngx_http_file_cache_valid(u->conf->cache_valid, rc);




More information about the nginx-devel mailing list