"proxy_cache_background_update on" ignored using subrequest (more exactly: nested subrequest)

[dfex55@…]

The ticket was originally opened for nginx/njs, but it is a problem with the Nginx subrequest mechanism.
See: ​https://github.com/nginx/njs/issues/573

Text from the original issue:

Requesting a location through njs subrequest that has caching enabled with proxy_cache_background_update on and proxy_cache_use_stale updating will never get stale data.

That seems to be a bug, because requesting the same location directly, the cache returns stale data.

In the provided example there are three locations:

/longRunningRequest/:
Has a js handler delaying the result for two seconds

/longRunningRequestCached/:
Has a cache that is valid for one second, allowing stale data while updating and proxy passes to /longRunningRequest/

/longRunningRequestThroughNjs/:
Has a js handler making a subrequest to /longRunningRequestCached/

Expected behavior:
/longRunningRequestCached/ and /longRunningRequestThroughNjs/ will always return immediately (except for the first request) and serving stale data if needed.

What actually happens:
/longRunningRequestCached/ works as described above
/longRunningRequestThroughNjs/ is blocking every time the cache entry has to be renewed.

Issue explanation by Dmitry Volyntsev:

What you see, is the limitation of an nginx subrequest mechanism. r.subrequest() creates an internal nginx subrequest with NGX_HTTP_SUBREQUEST_BACKGROUND and NGX_HTTP_SUBREQUEST_IN_MEMORY flags.

proxy_cache_background_update directive is also relies on subrequests. The main problem is: nginx does not support nested subrequest at this point.

Spefically the code of the cache update below checks the r->background flag, which is 1 for r.subrequest() and does not try to make a background update

example:

nginx config:

    js_import myjs.js;

    server {

        location = /longRunningRequestThroughNjs/ {
                js_content myjs.requestToCachedLongRunningRequest;
        }

        location = /longRunningRequest/ {
                js_content myjs.simulateLongRunningRequest;
        }

        location = /longRunningRequestCached/ {
                proxy_connect_timeout 1s;
                proxy_read_timeout 12s;
                proxy_set_header Content-Length "";
                proxy_pass http://127.0.0.1/longRunningRequest/;
                proxy_ignore_headers X-Accel-Expires Expires Cache-Control Set-Cookie;
                proxy_hide_header Set-Cookie;
                proxy_buffering on;

                proxy_cache_use_stale http_404 error timeout invalid_header updating
                                        http_500 http_502 http_503 http_504;

                proxy_cache_background_update on;
                proxy_cache_lock on;
                proxy_cache_lock_age 50s;
                proxy_cache_lock_timeout 60s;
                proxy_cache_revalidate on;
                proxy_cache MYCACHE;
                proxy_cache_valid 200 1s;
                proxy_cache_key $http_accept_encoding$uri;

                add_header x-cache-status $upstream_cache_status;
        }
    }

myjs.js:

function delayed(r) {
  r.return(200);
}

function simulateLongRunningRequest(r) {
  setTimeout(delayed, 2000, r);
}

function requestToCachedLongRunningRequest(r) {
  r.subrequest('/longRunningRequestCached/')
    .then(reply => {
      r.return(200);
    })
    .catch(_ => r.return(401));
}

export default { simulateLongRunningRequest, requestToCachedLongRunningRequest };

[Maxim Dounin]

Background subrequests are expected to be executed in the background, and were developed specifically to make it possible to update cache in the background. Using a background subrequests essentially tells nginx that the subrequest is already executed in the background, hence there is no need to switch to a background subrequest once again.

It is not clear why njs tries to use background subrequests, but the resulting proxy_cache_background_update behaviour looks perfectly correct. Switching njs to non-background subrequests instead is probably the way to go.

[xeioex]

The background flag is not the only problem here, even if it is removed, we have the following issue:

2022/09/08 08:59:57 [debug] 161199#161199: *2 http file cache expired: 4 1662617065 1662652797                                                
2022/09/08 08:59:57 [debug] 161199#161199: *2 http upstream cache: 4                                                                          
2022/09/08 08:59:57 [error] 161199#161199: *2 nested in-memory subrequest "/longRunningRequestCached/", client: 127.0.0.1, server: localhost, 
request: "GET /longRunningRequestThroughNjs/ HTTP/1.1", subrequest: "/longRunningRequestCached/", host: "localhost:8000"

Do we have any reason to block nested in-memory subrequest?

[xeioex]

It seems, that background flag can be dropped indeed. At least, all njs nginx-tests pass without it.

The problem is, if only njs is fixed here this use case going to fail with 500, whereas now it only slows down.

[Maxim Dounin]

Do we have any reason to block nested in-memory subrequest?

It's better to ask Roman Arutyunyan, who's the author of the code (see 20f139e9ffa8), and yourself, who was the reviewer. But I think the generic idea was that in-memory subrequests don't have an hierarchy, and spawning additional subrequests from them is not generally possible, since there is no sequence point where subrequest results should be sent to the client. This restriction probably can be relaxed to allow other in-memory subrequests and/or background subrequests to be created nevertheless.

[xeioex]

Consider the following patch:

# HG changeset patch
# User Dmitry Volyntsev <xeioex@nginx.com>
# Date 1662697852 25200
#      Thu Sep 08 21:30:52 2022 -0700
# Node ID 11a4e327c6c980b8db7349c1587a23e6a89ca884
# Parent  ba5cf8f73a2d0a3615565bf9545f3d65216a0530
Relaxed restriction for a subrequest from subrequests in memory.

Now, it is possible to create in-memory or background subrequest from
in-memory subrequests.

This reduces the number of restriction imposed on in-memory subrequests
thus making them more generic.

diff --git a/src/http/ngx_http_core_module.c b/src/http/ngx_http_core_module.c
--- a/src/http/ngx_http_core_module.c
+++ b/src/http/ngx_http_core_module.c
@@ -2341,9 +2341,12 @@ ngx_http_subrequest(ngx_http_request_t *
         return NGX_ERROR;
     }
 
-    if (r->subrequest_in_memory) {
+    if (r->subrequest_in_memory
+        && !(flags & (NGX_HTTP_SUBREQUEST_IN_MEMORY
+                      | NGX_HTTP_SUBREQUEST_BACKGROUND)))
+    {
         ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
-                      "nested in-memory subrequest \"%V\"", uri);
+                      "unsupported nested subrequest \"%V\"", uri);
         return NGX_ERROR;
     }

[Maxim Dounin]

I'm certainly against the error message change, as it is very confusing, much like from your initial explanation. In particular, it is very easy to incorrectly assume nested subrequests are not supported. In fact nested subrequests are perfectly supported, with the exception of in-memory subrequests, which are very limited (before nginx 1.13.10, these were only usable to obtain proxied responses, see ​here).

It might also make sense to change the condition to test NGX_HTTP_SUBREQUEST_IN_MEMORY and NGX_HTTP_SUBREQUEST_BACKGROUND separately, mostly from style point of view.

As for the rest of the change, consider submitting it to nginx-devel, I'll ask Roman to review it.

A better solution would be to properly support nesting for in-memory subrequests, this should make it possible to compose in-memory subrequests from multiple subrequests. This will be much more complex change though.

[Jack Bauer]

Hi,
we are also affected by this limitation. Is there a plan to fix it?
--jack

[maximk-1@…]

Hi!
Any updates/future plans to fix it?