[oowl]

Related PRs

PR	What it does
#2536	C side: pool_size in the balancer keepalive FFI API, per-pool descriptors
openresty/lua-resty-core#544	Lua side: balancer.enable_keepalive(idle_timeout?, max_requests?, pool_size?)
openresty/test-nginx#190	Fixes the ASan CI hang in HUP tests on Ubuntu 22.04. Independent of the feature

Suggested merge order:

1. bugfix: send the HUP mode signals from perl instead of running kill(1). test-nginx#190, then drop the TEMPORARY CI commit from feature: added the pool_size argument to the balancer keepalive FFI API. #2536.
2. feature: added the pool_size argument to the balancer keepalive FFI API. #2536 together with feature: ngx.balancer: added the pool_size argument to enable_keepalive(). lua-resty-core#544 (the FFI signature changes). Drop the TEMPORARY CI commit from feature: ngx.balancer: added the pool_size argument to enable_keepalive(). lua-resty-core#544 once feature: added the pool_size argument to the balancer keepalive FFI API. #2536 is on master.

---

Problem

The balancer_by_lua* keepalive cache has a single capacity per upstream {} block (balancer_keepalive, 32 by default) and evicts the least recently used idle connection, no matter which pool it belongs to. You cannot put a limit on one pool. When many pools share an upstream, which is common when peers are picked dynamically in balancer_by_lua*, a burst of traffic to one peer can push out the idle connections of every other peer.

Change

ngx_http_lua_ffi_balancer_enable_keepalive() gets a new pool_size argument.

• A pool is the key the cache already uses for reuse: peer address, the host given to set_current_peer(), and the local address (pc->local, so both bind_to_local_addr() and proxy_bind are covered).
• Before a connection is cached, if its pool already holds pool_size idle connections, that pool's own least recently used idle connection is closed and its slot is reused. The global LRU slot is left alone.
• pool_size == 0 keeps the current behaviour. pool_size never raises the balancer_keepalive total. It only limits one pool inside that total.

Per-pool descriptors (O(1))

A first version counted a pool's idle connections by scanning its hash bucket. I measured about 10ns per idle connection of the pool, so with a full pool of 512 (Kong's default size) each released connection cost about 5us. To avoid that, the hash buckets now hold one small descriptor per pool instead of the cached connections themselves:

• A descriptor (ngx_http_lua_balancer_ka_pool_t) keeps the pool's idle connections, most recently used first, and their count.
• Reuse takes the head of the pool's list. The "pool full?" check reads the count. Evicting the pool's oldest connection takes the tail of the list. None of these scan the other cached connections, so the cost no longer depends on the pool size.
• Descriptors are preallocated next to the cache items, one per item, because a pool only exists while it holds idle connections. They return to a free list when their pool becomes empty.
• Unchanged: the global LRU and the balancer_keepalive total, exact key matching through ngx_http_lua_balancer_item_match(), and reuse of the most recently used connection.

Compatibility

The FFI signature changes, so this needs the matching lua-resty-core PR: openresty/lua-resty-core#544. The two PRs should be merged together.

Tests

t/195-balancer-keepalive-pool-size.t (new) calls the C function through an explicit 5-argument prototype (ffi.cast). The tests therefore run with either the current or the updated lua-resty-core. Assertions check which backend connection serves each request (ngx.var.connection on the backend). They do not rely on debug logs.

• TEST 1: pool_size = 1. Of 3 connections released into one pool, only 1 stays idle.
• TEST 2: with balancer_keepalive 2, a full pool evicts its own oldest connection. Another pool's idle connection is left alone.
• TEST 3: the same scenario with pool_size = 0 shows the old behaviour, where the other pool's connection is evicted.
• TEST 4: error paths still write to err with the new signature (no current peer set).
• TEST 5: 8 pools churning through balancer_keepalive 4, with sequential and concurrent requests. This stresses descriptor allocation and release. A counting bug trips an assertion, and breaking the count in detach was verified to fail this test.
• TEST 6: an idle connection closed by the peer leaves its pool through the close handler, and the pool keeps working afterwards.
• TEST 7: a pool holding exactly pool_size connections evicts nothing (boundary).
• TEST 8: pool_size larger than balancer_keepalive never raises the total. The global LRU still applies.
• TEST 9: the host and the peer address (port) each split pools.
• TEST 10: the bound local address (bind_to_local_addr) splits pools, and the reused connection really comes from the bound address.
• TEST 11: an idle connection expiring on its own idle timeout leaves its pool (TEST 6 covers the peer closing it).
• TEST 12: a connection reaching max_requests is not cached, and the pool keeps working.
• TEST 13: a failed try is not cached, and the retry's connection is.

Mutation check: I built nginx with one deliberate bug at a time. Each one makes at least one test fail: ignore the host in the pool key (TEST 2, 9), ignore the peer address (TEST 9), ignore the local address (TEST 10), off-by-one in the "pool full" check (TEST 7), and not decrementing the pool count (TEST 5, assertion).

TEST 1 and 2 fail if pool_size is changed to 0. Note that the upstream blocks need keepalive 0;, because otherwise nginx's own upstream keepalive module (enabled by default in recent nginx) takes the connection before the lua balancer sees it.

The Lua API side and its tests are in openresty/lua-resty-core#544 (t/balancer-keepalive.t TEST 29–33, t/balancer-keepalive-localaddr.t TEST 8).

Ran locally against nginx 1.31.8 built with this branch, both as a normal --with-debug build and as an ASan build (NGX_BUILD_ASAN=1):

• lua-resty-core t/balancer*.t, t/stream/balancer*.t: all pass
• lua-nginx-module t/195-balancer-keepalive-pool-size.t, t/138-balancer.t, t/138-balancer-upstream-bind.t, t/188-balancer_keepalive_pool_max_retry.t: all pass, with both lua-resty-core master and the optimize: reduce memory alloc when get peer from socket_pool #544 branch
• no ASan reports
• t/195-balancer-keepalive-pool-size.t and t/balancer-keepalive.t (lua-resty-core) each passed 10 runs in a row with TEST_NGINX_RANDOMIZE=1
• built with -DNGX_LUA_USE_ASSERT, as CI does, so the descriptor assertions are active
• ngx-releng output is identical to master
• the full CI workflow (all 5 jobs, including the whole suite under ASan) was run on my fork before pushing: https://github.com/oowl/lua-nginx-module/actions/runs/40211217018 (all 5 jobs passed, before TEST 7-13 were added)

Temporary CI commit

The last commit (tests: TEMPORARY: ...) makes CI clone the branch of openresty/test-nginx#190 (oowl/test-nginx@bugfix/hup-signal-without-kill) instead of openresty/test-nginx master. Drop it once openresty/test-nginx#190 is merged.

Why: since CI moved to Ubuntu 22.04, the ASan job hangs until the 6h limit in the first TEST_NGINX_USE_HUP test (t/078-hup-vars.t). master's own ASan job does the same, e.g. https://github.com/openresty/lua-nginx-module/actions/runs/39918204085. Test::Nginx sends USR1/HUP with system("kill ..."). On 22.04, procps' kill (and ps) deadlock at startup in a futex wait when gcc 11's libasan is in LD_PRELOAD, which the ASan job sets. true, sh, env and perl start fine under the same preload. The test-nginx branch sends the signals with perl's kill() instead. With it, all 8 HUP test files pass under ASan.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.