Conversation
libunit tracks detached work per context, the router keys it per process,
and the START and FINISH edges carry neither a request nor a context id. A
worker running several contexts at once -- python, ruby and java all do --
could therefore have the first context's FINISH hand a still-busy process
back to the idle economy: the port returns to the idle queues, the reaper
may take it, and its slot returns to "processes": {"max"} while the other
contexts are still executing.
Make port->detached_app a uint32_t count, the way port->detached_router
already is. START raises it, FINISH lowers it, and the aggregate
port->detached, app->detached_processes and the application reference move
only on the transitions that matter: the first reason taken and the last one
settled. /status keeps reporting the processes and nxt_router_free_app()
still sees zero. A START at UINT32_MAX saturates with an alert rather than
wrapping to "no detached work", and a FINISH at zero -- a worker whose death
has already settled the count, or a forged edge -- alerts and changes
nothing instead of underflowing. nxt_router_app_port_close() settles
whatever the count holds in one go, as it already did.
The wire format is unchanged: the same two one-byte edges on the same
message type, so old and new workers interoperate in either direction.
nxt_port.h states the protocol contract next to nxt_port_detached_t: the
edges are a balanced pair, they name no request or context, and the payload
may grow. nxt_unit.h says libunit counts per context while the router
counts per worker. nxt_router_detached_test.c becomes table-driven over 10
edges, adding two starts against one finish, the second finish that releases
the worker, and an unmatched finish.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@claude review |
|
Claude finished @andypost's task in 5m 1s —— View job Review:
|
| sequence | state ref owner at each step | net |
|---|---|---|
| START, START, FINISH, FINISH | start#1 takes it; inner pair net 0; last FINISH drop=2 |
0 |
| START, abandon, FINISH, settle | start takes it; FINISH changed=0→drop=1; settle ours=1→ −2 |
0 |
| abandon, START, settle, FINISH | abandon takes it; settle ours=0 (detached_app≠0)→ −1; FINISH changed=1→ −2 |
0 |
| abandonA, abandonB, START, settleA, FINISH, settleB | abandonA takes it; settleB ours=1 releases it |
0 |
| START, abandon, port_close, settle | port_close zeroes both counts, drops 1; settle sees detached_router==0→ −1 |
0 |
port->detached_app is touched only under app->mutex at all four sites, and detached == 0 ⟹ detached_app == 0 is preserved everywhere, so the defensive port->detached != 0 check at src/nxt_router.c:8121 stays a no-op. libunit's per-context guard (nxt_unit_ctx_detached_start(), src/nxt_unit.c:3653) bounds the count by the worker's context count, which is the premise the saturation comment rests on. The nxt_port.h protocol note matches what nxt_router_detached_handler() actually enforces (≥1 byte, reads the first).
Notes below, none of them blocking.
1. An unmatched FINISH is now an application-triggerable, unbounded alert — src/nxt_router.c:8110
A worker can send well-formed _NXT_PORT_MSG_DETACHED FINISH edges in a loop for its own pid and get one nxt_alert() line per edge. The handler already alerts on forged pids and invalid state bytes, so there's precedent — but those are malformed messages, while this is a valid message on a valid type, so it is the cheapest of the three to spam.
Worth noting the old header comment justified the no-op as "the worker's death has already settled the state". I checked whether that path is reachable and it is not: nxt_runtime_port_find(rt, pid, 0) fails first, and nxt_port_close() → nxt_router_app_port_close() and nxt_runtime_port_remove() run back-to-back in the same main-thread tick (src/nxt_router.c:5327, src/nxt_router.c:1459), so no edge interleaves between them. So promoting it to an alert is defensible — just be aware it also means any benign desync becomes log noise. nxt_debug, or alerting once per port, would bound it.
2. Counting makes a duplicate FINISH harmful where latching made it idempotent
This is the trade-off the anonymous edge forces, and the PR body is upfront about it, but it deserves a line in the code: with a flag, a second FINISH did nothing; with a count, it decrements some other context's START and hands back a worker that is still executing. The one place that could produce a duplicate is nxt_unit_ctx_detached_retry() (src/nxt_unit.c:3770), which resends FINISH whenever nxt_unit_send_detached() reports failure. I don't think it's reachable today — nxt_unit_send_detached() only fails on a NULL router port or nxt_unit_port_send() != sizeof(m), and a 1-byte-payload send on the router socket is all-or-nothing — but the safety of the retry now depends on that, where before it didn't. A comment at the retry saying so, and/or a context id in the payload later (the nxt_port.h note already reserves room for the payload to grow), would close it.
3. detached_router has no saturation guard, detached_app now does — src/nxt_port.h:370
nxt_router_app_abandon() does a bare port->detached_router++. The field comment argues it's bounded by active_requests / validated threads, which is sound, but the two fields now read asymmetrically for the same "a wrap reads as no detached work" hazard. A sentence in the detached_app comment pointing at why only it saturates would save the next reader the trip.
4. Test: nxt_atomic_t in the expectation table — src/test/nxt_router_detached_test.c:50
nxt_atomic_t is volatile nxt_atomic_uint_t (src/nxt_atomic.h:21), so the static const table ends up with a const volatile member holding what is a plain compile-time constant, and each comparison is a volatile read. nxt_atomic_int_t or uint32_t expresses the intent better. (Field order also leaves 3 bytes of padding after detached — irrelevant for a test table, but the file otherwise follows the by-size convention.)
5. Test gap: the two cases the reference accounting actually turns on
The new table covers the count well, but not the two branches where a wrong drop would leak or over-release app->use_count:
- START while
detached_router != 0—changed == 0,drop == 1, and the state reference is deliberately left tonxt_router_app_abandoned_settle(). This is exactly what the reworked comment atsrc/nxt_router.c:8161-8168describes, and it's untested. Adding a per-edge "presetdetached_router" field tonxt_router_detached_test_edge_twould cover it within the existing structure. - The
UINT32_MAXsaturation branch — reachable in a test by presettingport->detached_appbefore the edge; cheap to assert that the count doesn't wrap anddetachedstays set.
Nits
const char *alert;atsrc/nxt_router.c:7990sits betweenint drop;andnxt_app_t *app;; the file orders declarations by type length, which would put it withnxt_bool_t/nxt_port_t.- No
CHANGES/docs/changes.xmlentry, unlike the three preceding commits in this series. That looks intentional — the existing 1.36.2 entry ("Such a worker now reports itself busy until the work ends") is only made more true by this change — but flagging in case that section is already released.
Caveat: I reviewed by inspection only. ./configure isn't permitted in this sandbox, so I could not reproduce the build or run build/tests — I'm taking the PR's build and test results at their word.
· branch fix/detached-app-count
1 participant
Make the router count the application's detached work instead of latching it.
libunit tracks detached work per context and the router keys it per process,
while the START and FINISH edges name neither a request nor a context. With
port->detached_appa flag, a worker running several contexts at once --python, ruby and java all do -- had the first context's FINISH clear it: the
port went back into the idle queues, the reaper could QUIT a process that was
still executing, and its slot returned to
"processes": {"max"}while theother contexts ran on.
detached_appbecomes auint32_t, matchingdetached_routerfrom #412.START raises it, FINISH lowers it, and the aggregate
port->detached,app->detached_processesand the application reference move only on the firstreason taken and the last one settled -- so
/statusstill reports processesand
nxt_router_free_app()still sees zero. A START atUINT32_MAXsaturateswith an alert instead of wrapping to "no detached work"; a FINISH with nothing
to match alerts and changes nothing instead of underflowing.
nxt_router_app_port_close()settles whatever the count holds, once.The wire format is unchanged: the same two one-byte edges on the same message
type, so old and new peers interoperate in either direction.
nxt_port.hnowstates the protocol contract next to
nxt_port_detached_t, and the router Ctest is table-driven over ten edges, covering two starts against one finish,
the finish that releases the worker, and an unmatched finish.
This was reviewed as part of #405 and is the half its merge did not take, now
reapplied to the field set #412 introduced. The multi-context failure is not
reproduced end to end here: PHP is the only caller of
nxt_unit_request_done_detached()and runs one context, so the case is coveredby the C test and by libunit's per-context guard, not by the python suite.
Debug and release build with 0 warnings;
build/testsandbuild/unit_port_recv_testmatch master (39 / 22);test_php_detached_max.pyand
test_app_start_timeout.py: 25 passed. Each new assertion was checked byreverting the production change.
🤖 Generated with Claude Code