From f6dd6e083bcbf2ffdaa957cda37e608a189525a7 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 8 May 2026 11:18:09 +0000 Subject: [PATCH] refactor(engine): track active connections symmetrically with idle (P4.5 / pre-P5) A separate prep PR for P5 (connection drain). The P5 audit flagged that the engine has no enumerable list of active connections -- nxt_conn_active() removed a conn from engine->idle_connections but did not place it onto any other queue, so c->link dangled while a request was in flight. The graceful-drain force-close-on-timeout step in P5 needs an enumerable list of active conns; counters cannot substitute because closed_conns_cnt only fired on idle-conn closes. This PR adds the missing primitive and makes idle/active state symmetric. No drain logic, no timer logic -- those land in P5 on top of this. What changes ------------ * src/nxt_event_engine.h adds: nxt_queue_t active_connections; nxt_atomic_uint_t active_conns_cnt; alongside the pre-existing idle_connections / idle_conns_cnt. * src/nxt_event_engine.c initialises the new queue and counter in nxt_event_engine_create(). * src/nxt_conn.h: - c->idle becomes a tri-state field (still uint8_t): NXT_CONN_TRACK_NONE = 0 (e.g. controller, proxy peer -- c->link owned by another subsystem) NXT_CONN_TRACK_IDLE = 1 (on engine->idle_connections) NXT_CONN_TRACK_ACTIVE = 2 (on engine->active_connections) The pre-P4.5 boolean coincided with TRACK_IDLE, and existing callers that treated c->idle as boolean stay correct because NXT_CONN_TRACK_IDLE == 1. - nxt_conn_idle() now removes from active_connections (if coming from TRACK_ACTIVE) before inserting onto idle_connections. Counter accounting is symmetric: -active_conns_cnt then +idle_conns_cnt. - nxt_conn_active() now inserts onto active_connections after removing from idle_connections. Counter symmetric. - Both macros document their valid prior states. * src/nxt_conn_close.c grows an explicit TRACK_ACTIVE branch in both close handlers (sync and timer-driven). closed_conns_cnt is now incremented for both idle and active closes -- the pre-P4.5 behaviour effectively did this too because c->idle was never reset to 0 by nxt_conn_active(), so every accepted-conn close already incremented the counter (the gating condition `if (c->idle)` was true for the residual TRACK_IDLE bit left from the original idle insertion). The new explicit form makes the contract auditable. * src/nxt_runtime.c nxt_runtime_close_idle_connections() drops its explicit nxt_queue_remove(link) before nxt_conn_close(). The close handler now performs the unlink itself; doing it twice would corrupt the queue. Comment explains. Call-site audit --------------- git grep "nxt_conn_idle\|nxt_conn_active" src/ src/nxt_h1proto.c:485 nxt_conn_idle (initial idle on accept-side keep-alive) src/nxt_h1proto.c:496 nxt_conn_active (request begin) src/nxt_h1proto.c:1860 nxt_conn_idle (keep-alive return after response) src/nxt_h1proto.c:1885 nxt_conn_idle (peer keep-alive) src/nxt_h1proto.c:1985 nxt_conn_active (peer request begin) src/nxt_conn_accept.c:264 nxt_conn_active (initial mark on first accept) src/nxt_conn_close.c:122,160 c->idle gating (now tri-state branches) Every call site is reachable only after the conn has been on either idle_connections (most) or never on a tracked queue at all (controller / proxy peer); no caller invokes nxt_conn_active on a NONE-state link, so unconditional nxt_queue_remove inside that macro remains safe. Verified -------- ./configure --tests --modules=python && ./configure python \ --config=python3-config && make -j$(nproc) # clean Plain build, regression slice (--restart): test/test_python_application.py + test/test_idle_close_wait.py => 42 passed, 6 skipped test/test_routing.py => 4 failed, 109 passed, 2 skipped, 4 errors -- ALL four failures are [::1] IPv6 environmental on plain freeunit/master too (verified by `git checkout freeunit/master -- src/` rebuild + same suite). Zero P4.5-attributable regressions. ASan build (-fsanitize=address): test/test_python_application.py + test/test_idle_close_wait.py => no queue corruption, no use-after-free. Sole LeakSanitizer reports are 4x 136-byte nxt_var_index_init / nxt_runtime_create entries -- one per unitd process, pre-existing on master, separate issue. Refs ---- - roadmap/plan-graceful-shutdown.md (P5 prerequisite; P5 itself consumes engine->active_connections to enumerate in-flight conns for drain-done detection and force-close-on-timeout) - upstream nginx/unit PR #334 (defensive queue-traversal pattern; not directly applied here because no traversal is added, but cited as the model for P5) --- src/nxt_conn.h | 60 +++++++++++++++++++++++++++++++++++++++--- src/nxt_conn_close.c | 38 ++++++++++++++++++++++++-- src/nxt_event_engine.c | 3 +++ src/nxt_event_engine.h | 2 ++ src/nxt_runtime.c | 8 +++++- 5 files changed, 105 insertions(+), 6 deletions(-) diff --git a/src/nxt_conn.h b/src/nxt_conn.h index 5717d3c99..7dcc268dc 100644 --- a/src/nxt_conn.h +++ b/src/nxt_conn.h @@ -160,7 +160,23 @@ struct nxt_conn_s { uint8_t block_read; /* 1 bit */ uint8_t block_write; /* 1 bit */ uint8_t delayed; /* 1 bit */ - uint8_t idle; /* 1 bit */ + +#define NXT_CONN_TRACK_NONE 0 +#define NXT_CONN_TRACK_IDLE 1 +#define NXT_CONN_TRACK_ACTIVE 2 + + /* + * Tri-state membership in the engine's idle/active connection queues. + * NONE (0): conn was never placed onto an engine tracking queue + * (e.g. controller clients, proxy peers). c->link is owned + * by some other subsystem. + * IDLE (1): conn is on engine->idle_connections. + * ACTIVE (2): conn is on engine->active_connections. + * + * Set/cleared exclusively by nxt_conn_idle() / nxt_conn_active() and + * cleared by nxt_conn_close() when the conn is unlinked from its queue. + */ + uint8_t idle; /* 2 bits */ #define NXT_CONN_SENDFILE_OFF 0 #define NXT_CONN_SENDFILE_ON 1 @@ -295,24 +311,62 @@ NXT_EXPORT void nxt_event_conn_job_sendfile(nxt_task_t *task, } while (0) +/* + * Place conn onto engine->idle_connections. + * + * Valid prior states: + * NXT_CONN_TRACK_NONE - freshly created conn (e.g. just-accepted in + * nxt_conn_accept()). Link is not on any queue. + * NXT_CONN_TRACK_ACTIVE - keep-alive transition: conn is currently on + * engine->active_connections and is being moved + * back to idle awaiting the next request. + * + * After this macro: conn is on idle_connections, c->idle == TRACK_IDLE. + */ #define nxt_conn_idle(engine, c) \ do { \ nxt_event_engine_t *e = engine; \ \ + if (c->idle == NXT_CONN_TRACK_ACTIVE) { \ + nxt_queue_remove(&c->link); \ + nxt_atomic_fetch_add(&e->active_conns_cnt, -1); \ + } \ + \ nxt_queue_insert_head(&e->idle_connections, &c->link); \ \ - c->idle = 1; \ + c->idle = NXT_CONN_TRACK_IDLE; \ e->idle_conns_cnt++; \ } while (0) +/* + * Move conn onto engine->active_connections. + * + * Valid prior states: + * NXT_CONN_TRACK_IDLE - conn is currently on engine->idle_connections; + * a request has begun arriving (or idle timeout + * is firing a forced response). + * + * After this macro: conn is on active_connections, c->idle == TRACK_ACTIVE. + * + * Note: every existing call site in nxt_h1proto.c invokes this only on a + * conn that just transitioned from idle (handlers wired to *_idle_state / + * *_keepalive_state). The TRACK_IDLE precondition is not asserted to keep + * the macro branch-free in the hot path; if a future caller violates it, + * idle_conns_cnt will go negative and the queue will corrupt -- both + * detectable under ASan via the ASSERT in nxt_conn_close(). + */ #define nxt_conn_active(engine, c) \ do { \ nxt_event_engine_t *e = engine; \ \ nxt_queue_remove(&c->link); \ + e->idle_conns_cnt -= (c->idle == NXT_CONN_TRACK_IDLE); \ + \ + nxt_queue_insert_head(&e->active_connections, &c->link); \ \ - e->idle_conns_cnt -= c->idle; \ + c->idle = NXT_CONN_TRACK_ACTIVE; \ + nxt_atomic_fetch_add(&e->active_conns_cnt, 1); \ } while (0) diff --git a/src/nxt_conn_close.c b/src/nxt_conn_close.c index bdd669515..aba4de67f 100644 --- a/src/nxt_conn_close.c +++ b/src/nxt_conn_close.c @@ -119,7 +119,31 @@ nxt_conn_close_handler(nxt_task_t *task, void *obj, void *data) nxt_socket_close(task, c->socket.fd); c->socket.fd = -1; - if (c->idle) { + /* + * Unlink from the engine idle/active tracking queue, if any. + * Conns that were never tracked (e.g. controller clients, proxy + * peers) keep c->idle == NXT_CONN_TRACK_NONE and own c->link + * separately, so this branch must not run for them. + * + * closed_conns_cnt is incremented symmetrically for both idle and + * active closes (P4.5). Prior to P4.5 only idle closes were + * counted, but in practice every conn passing through accept was + * placed onto the idle queue and c->idle was never reset to 0 by + * nxt_conn_active(), so the counter was already incrementing for + * effectively every accepted-conn close. The new symmetric form + * preserves that behaviour while also covering the case where a + * conn closes while genuinely active. + */ + if (c->idle == NXT_CONN_TRACK_IDLE) { + nxt_queue_remove(&c->link); + engine->idle_conns_cnt--; + c->idle = NXT_CONN_TRACK_NONE; + engine->closed_conns_cnt++; + + } else if (c->idle == NXT_CONN_TRACK_ACTIVE) { + nxt_queue_remove(&c->link); + nxt_atomic_fetch_add(&engine->active_conns_cnt, -1); + c->idle = NXT_CONN_TRACK_NONE; engine->closed_conns_cnt++; } @@ -157,7 +181,17 @@ nxt_conn_close_timer_handler(nxt_task_t *task, void *obj, void *data) nxt_socket_close(task, c->socket.fd); c->socket.fd = -1; - if (c->idle) { + /* See nxt_conn_close_handler() for the symmetry rationale (P4.5). */ + if (c->idle == NXT_CONN_TRACK_IDLE) { + nxt_queue_remove(&c->link); + engine->idle_conns_cnt--; + c->idle = NXT_CONN_TRACK_NONE; + engine->closed_conns_cnt++; + + } else if (c->idle == NXT_CONN_TRACK_ACTIVE) { + nxt_queue_remove(&c->link); + nxt_atomic_fetch_add(&engine->active_conns_cnt, -1); + c->idle = NXT_CONN_TRACK_NONE; engine->closed_conns_cnt++; } } diff --git a/src/nxt_event_engine.c b/src/nxt_event_engine.c index 78c79bb1a..c3fa904eb 100644 --- a/src/nxt_event_engine.c +++ b/src/nxt_event_engine.c @@ -139,6 +139,9 @@ nxt_event_engine_create(nxt_task_t *task, nxt_queue_init(&engine->joints); nxt_queue_init(&engine->listen_connections); nxt_queue_init(&engine->idle_connections); + nxt_queue_init(&engine->active_connections); + + engine->active_conns_cnt = 0; return engine; diff --git a/src/nxt_event_engine.h b/src/nxt_event_engine.h index 291ea749b..8f5666851 100644 --- a/src/nxt_event_engine.h +++ b/src/nxt_event_engine.h @@ -481,10 +481,12 @@ struct nxt_event_engine_s { nxt_queue_t joints; nxt_queue_t listen_connections; nxt_queue_t idle_connections; + nxt_queue_t active_connections; nxt_array_t *mem_cache; nxt_atomic_uint_t accepted_conns_cnt; nxt_atomic_uint_t idle_conns_cnt; + nxt_atomic_uint_t active_conns_cnt; nxt_atomic_uint_t closed_conns_cnt; nxt_atomic_uint_t requests_cnt; diff --git a/src/nxt_runtime.c b/src/nxt_runtime.c index de76f19e0..a7f2111cf 100644 --- a/src/nxt_runtime.c +++ b/src/nxt_runtime.c @@ -484,7 +484,13 @@ nxt_runtime_close_idle_connections(nxt_event_engine_t *engine) c = nxt_queue_link_data(link, nxt_conn_t, link); if (!c->socket.read_ready) { - nxt_queue_remove(link); + /* + * Do not unlink here: nxt_conn_close() now removes the conn + * from idle_connections in its async close handler and + * decrements idle_conns_cnt accordingly (P4.5). Iteration is + * safe because `next` was captured before close was scheduled + * and the actual unlink runs from a later work-queue tick. + */ nxt_conn_close(engine, c); } }