[AxisNow-Dev]

I hereby granted the copyright of the changes in this pull request
to the authors of this lua-nginx-module project.

With a connection pool backlog, the pool can hold far more than pool_size connections.

setkeepalive() puts the connection back in the pool, where spool->connections still counts it, and calls ngx_http_lua_socket_tcp_resume_conn_op(), which posts the resumption of the next queued operation so that it takes this idle connection. A connect() made before that event runs calls ngx_http_lua_get_keepalive_peer() first and takes the idle connection. When the resumed operation runs, ngx_http_lua_get_keepalive_peer() finds nothing, and since a resumed operation skips the backlog check in ngx_http_lua_socket_tcp_connect_helper(), it opens a new connection. Each time this happens the pool holds one more connection than pool_size, up to pool_size + backlog.

This existed before #2542, but there only one operation was posted at a time, so it was rare. Since #2542 every freed slot posts its own operation, which leaves many more windows for a new connect() to take a connection handed to a waiter. One worker with pool_size = 100, backlog = 500, serving a closed-loop load of 550 and 2000 concurrent requests, each making one cosocket query to a local backend over a unix domain socket and putting the connection back with setkeepalive(), held up to 310 to 536 connections to the backend in content and rewrite handlers. An earlier build without #2542 held up to 111.

The pull request has twelve commits. The design that lands is the direct handoff from b965a88, written by the maintainer on top of the first five. The earlier commits are kept as history: the list below says, for each one, whether it was replaced, kept as a rule, or is a separate fix.

Final design

After b965a88 a pool keeps two counts:

• connections is the slots handed out: active sockets, cached idle connections, and posted connect operations.
• waiting is the operations still queued in wait_connect_op, which hold no slot.

The backlog check is waiting >= backlog. Posted operations move to their own posted_connect_op queue, which replaces the next_connect_op cursor.

When setkeepalive() returns a connection while someone waits, the connection does not go to cache. It is attached to the head waiter (op->item, with item->owner pointing back), and the waiter takes it with ngx_http_lua_socket_tcp_reuse_peer() when its resumption runs in its own request. New connects look at cache only, so they cannot take it.

Every place that frees a slot goes through ngx_http_lua_socket_tcp_release_slot(), whether or not a connection comes with it: setkeepalive(), finalize(), the keepalive close handler, cancellation of a posted operation, a failed connect, and an aborted resolve. If someone waits, it resumes the head waiter with the slot and the connection, if any. Otherwise it puts the connection in cache, or decrements connections when there is no connection.

A connection closed while it waits for its owner only clears the owner's item; the owner then connects in its slot.

An operation resumed with an empty slot, with no connection handed to it, first takes the head of cache when it runs. If it gets one, it gives its spare slot back through release_slot(). Otherwise it connects in its slot (12e9595).

Commits

1. 824e4a5 bugfix: new connects took the idle connection handed to a queued connect, which then went past the pool size. The first approach: a new connect() skipped every idle connection while any operation waited. Replaced by the second commit.
2. 1b91986 bugfix: hold idle connections for resumed connects in a reserved queue. Moved the connection handed to a resumed operation to a reserved queue, out of reach of new connects. Replaced by b965a88.
3. c929bd2 bugfix: cancelling a queued connect resumed another one beyond the pool size. Cancelling an operation still queued, which holds no slot, resumed the next waiter, which then connected beyond the pool size. The rule stays in the final design: cancelling a queued operation only decrements waiting.
4. 08f7177 bugfix: a resumed connect cancelled after its reserved idle connection closed left the freed slot to nobody. A gap in the reserved queue, found in review: if the connection held for a resumed operation closed and the operation was then cancelled, the slot went to nobody. The final design covers this case through release_slot() without a connection, and test 65 stays.
5. ea39682 bugfix: preserve cosocket pool capacity across queued connects. Built on the reserved queue. It counted reserved connections apart, so that each slot is counted once. It also closed connections made before the pool existed, which the pool does not count, when no slot was free. Superseded by b965a88; its tests stay.
6. b965a88 change: hand cosocket connections directly to waiting operations. The final design above. Connections made before the pool existed cannot evict a connection handed to a waiter. Cancelling a posted operation before it runs releases its slot and connection.
7. fbbcf41 bugfix: keep queued cosockets busy until their connect resumes. A socket stays busy connecting while its operation is queued or posted, so another connect() on it is rejected before it can change the pool key.
8. 3e21572 bugfix: close partially initialized cosockets on connect failure. One cleanup path for failed connects: it closes a partly set up peer and releases the slot once.
9. 990a29a change: fix cosocket source formatting.
10. 971d890 tests: cover cosocket queue cleanup across phases and protocols. Adds t/196-socket-keepalive-queue.t, which covers handoff, cancellation and timeouts in rewrite, access and content, and has a fixed-seed stress case. Also adds tests 66 to 84 to t/068.
11. 6516e3f tests: cover cosocket slot release on stale, failed, unclosed and cancelled connects. See "Additional tests" below.
12. 12e9595 bugfix: a connect resumed with an empty slot opened a new connection next to an idle one. Found while writing the tests above. An operation resumed with an empty slot skipped cache when it ran, because connect_helper() is called with resuming set. If a connection was put back in the same cycle, with nobody left waiting, the operation opened a new connection while that idle one stayed unused. The pool stayed within pool_size, but opened one connection it did not need. The operation now takes the head of cache first and gives its spare slot back. Test 92: with pool_size = 2, a:close() hands an empty slot to w, then b:setkeepalive() runs before w does. w now gets b's connection (reused 1), and exactly one more connection fits. If you would rather keep the old behaviour, this commit can be dropped on its own.

Approaches considered

The overflow has two ingredients: a new connect() can take an idle connection meant for a posted operation, and a resumed operation that finds none opens a new one without the backlog check. Each approach below removes one of them in a different way.

	Approach	New connect() while operations wait	Work per new connect()	Spare idle connections	Notes
A	Skip every idle connection while any operation is queued or posted (first commit)	always queues	O(1)	left unused: a new connect() can time out next to one	smallest change
B	Reuse an idle connection when the pool holds more of them than posted operations, found by walking cache and the posted operations together	reuses the surplus	O(min(idle, posted))	reused	same result as D, but walks lists on the path #2542 made constant-time
C	Same rule as B with two counters, idle connections and posted operations	reuses the surplus	O(1)	reused	not implemented: the counters must be kept right at about eight places (setkeepalive, eviction, get_keepalive_peer, the keepalive close handler, resume, cancellation, shutdown), and a drifting counter is the kind of bug this PR fixes
D	Reserved queue (second commit, replaced by b965a88)	reuses the surplus	O(1)	reused	the connection held for a posted operation is out of reach by construction, with no counters, but cancellation, peer close, eviction and shutdown each need their own handling, and the fourth commit fixed a gap there
E	Let a resumed operation that finds no idle connection re-check the limit and queue again instead of connecting	keeps taking idle connections	O(1)	reused	rejected: under load, new connects keep taking the connections handed to waiters, so waiters lose their turn again and again and time out, which is what #2542 fixed
F	Hand the connection to the posted operation itself, which attaches it when it runs (b965a88, final)	cannot see it	O(1)	reused	the slot and the connection travel together through one function, and ownership is explicit (item and owner), so no special queue is needed. An earlier version of this table rejected F in the form "set up the connection on the waiter's upstream in setkeepalive()", outside the waiter's request. b965a88 avoids that: the connection is attached only when the operation runs in its own request.

Measured on the load above at 550 concurrent requests in content and rewrite handlers, 6 seconds per run, before b965a88. The final design was not measured on this load:

	master	A	B	D
Max connections to the backend	310 to 385	100	100	100
too many waiting connect operations	none, extra connections were opened instead	69 to 2337	6 to 13	17 to 125

B was measured in one run per handler, D in two; out of 68,000 to 222,000 requests per run, both stay at or below 0.15% of the requests, and on my machine throughput itself varied several times over between identical runs, so I would not read a difference between them. The much higher count of A comes from its new connects queueing behind waiters while idle connections sit unused.

Tests added to t/068-socket-keepalive.t by the first four commits:

Test	Case	Expected	master	first commit	this PR
61	pool_size = 1, one waiter, setkeepalive() and a new connect() in the same cycle	waiter: reused 1, newcomer: reused 2	fails (waiter: reused 0, newcomer: reused 1)	passes	passes
62	pool_size = 2, two connections come back with one waiter queued, new connect() with a 200ms timeout	newcomer: reused 1	passes	fails (newcomer: timeout)	passes
63	the only connection is handed to w1, w1 is killed before it runs, then a new connect()	w2: reused 1, newcomer: reused 2	fails	passes	passes
64	the only slot busy, two waiters queued, the first one killed	w2: timeout	fails (w2: reused 0)	fails	passes
65	the only connection is handed to w1, closed by a 1ms keepalive timeout, then w1 is killed, all before w1's resumption runs	killed: 1, w2: reused 0	n/a (no reserved queue)	n/a (no reserved queue)	passes (fails with the first three commits: w2: timeout)

Tests 61 to 64 passed over HTTP/1.1 and HTTP/2, and under valgrind with no errors, on the reserved queue.

Test 65 needs the keepalive timer and the cancellation to come before the posted resumption in one cycle. To set that up:

1. A light thread first yields with coroutine.yield(), so that it runs after the entry thread has started ngx.sleep(0.002).
2. It then calls setkeepalive(1) and moves the clock on with ngx.update_time().
3. Both timers then expire ahead of the posted events.

Test 65 is skipped over HTTP/3, where the request itself runs from posted events and that order cannot be set up.

With all twelve commits, tests 53 to 92 and t/196 pass on my machine over HTTP/1.1 and HTTP/2. The other tests in the file fail or pass the same with and without the change. Those failures come from my local setup: a Server header of a different length, some services I do not have, and 127.0.0.2:12345 being refused at once instead of timing out.

With the reserved queue (second to fourth commits), the load above held exactly 100 connections in every case. A few requests near pool_size + backlog got too many waiting connect operations: 17 to 125 out of 68,000 to 132,000 requests in 6 seconds at 550 concurrent requests. They were rejected because a posted operation and the idle connection it was about to take were both counted until its event ran. The final design counts queued operations apart from slots, which should remove that double count by construction. I have not rerun the load on it.

Tested against nginx 1.31.8 in an OpenResty build with this module at current master.

Additional tests for the direct handoff (6516e3f)

These cover paths of the direct handoff in the later commits that no test reached or asserted. I found them by reading the tests against the code and by a gcov run of t/068-socket-keepalive.t and t/196-socket-keepalive-queue.t.

Test	Case	Expected
85	a connection goes back to a full pool while an unread response makes it readable, with a waiter queued	setkeepalive: connection in dubious state, waiter: reused 0, then exactly one connection fits in the pool
86	two waiters queued for a port nothing listens on, then the holder closes	both get connection refused without sitting out the connect timeout, then exactly one connection fits
87	the request ends without closing its pooled socket while a timer waits in the queue	the timer connects (reused 0) and a second connect times out
88	a light thread queued in a subrequest is cancelled when the subrequest exits	its backlog place is free again: a new connect queues and gets reused 1
89	connect() with another pool on a socket that is busy reading	socket busy reading, and setkeepalive() still uses the original pool (reused 1)
90	in a full pool with a free item left, a connection made before the pool existed is put back	it replaces the idle connection: the next connect gets it (reused 1) and one more times out
91	the worker gets SIGTERM with connects queued in one pool and an idle connection in another	ngx_http_lua_socket_shutdown_pool_helper() frees both, and the Lua VM is closed afterwards (a __gc marker logs from lua_close())

Each test fails when the branch it covers is broken by a one-line change. Examples:

• dropping the slot release in the keepalive close handler (85);
• going on after the "connection in dubious state" return (85);
• dropping the release in ngx_http_lua_socket_tcp_finalize() (86, 87);
• dropping waiting-- for a cancelled queued operation (88);
• writing the pool key before the busy checks, as before fbbcf41 (89);
• dropping the full-pool condition for connections made before the pool existed (90);
• freezing either loop of the shutdown helper (91).

The tests passed five runs out of five over HTTP/1.1 and over HTTP/2, and under valgrind with no errors from this module. I did not run them over HTTP/3. With them added, the full file fails the same environment-dependent tests as before on my machine, which has no memcached, and t/196 passes.

Some paths remain uncovered:

• ngx_http_lua_socket_tcp_conn_op_ctx_cleanup() looks unreachable from a test: its cleanup is registered and freed within the same synchronous run of the phases.
• In the shutdown helper, the branches for posted operations and the connections handed to them look unreachable too. Posted events run within the cycle that posts them, and the worker checks ngx_terminate only between cycles.
• The ctx == NULL checks and the out-of-memory branches have no tests.

The stream counterpart is openresty/stream-lua-nginx-module#418, now ported to the same design: the direct handoff and its fixes, 12e9595, and the tests, including t/196.

Contributed by AxisNow.

[perfgao]

One small thing I noticed while writing the tests. It's not a bug, just something you may or may not want to change.

A waiter that gets resumed with an empty slot (because someone close()d rather than setkeepalive()d) always opens a new connection when it runs. It doesn't look at cache, because connect_helper() is called with resuming = 1. If someone else puts a connection back into cache in the same cycle, before the waiter runs, that idle connection just sits there while the waiter dials a new one.

A quick way to see it, with pool_size = 2, backlog = 1:

1. A and B are connected. W is queued.
2. a:close() hands an empty slot to W.
3. b:setkeepalive() in the same cycle: nobody is waiting any more, so B's connection goes to cache.
4. W runs and gets reused 0 (a new connection), and the next connect() gets B's connection (reused 1).

The pool never goes over pool_size. W's slot and B's cached connection are both counted, so it's only one extra connection opened at that moment. If you think it's worth it, the empty-slot path could take the head of cache first, and give the spare slot back with release_slot(spool, NULL) (which wakes the next waiter, or decrements connections). I'm also fine leaving it as is.

[perfgao]

12e9595