Skip to content

libunit: run the detached finish retry from an embedder event loop - #419

Open
andypost wants to merge 1 commit into
masterfrom
fix/libunit-detached-retry-embedder-loop
Open

andypost wants to merge 1 commit into
masterfrom
fix/libunit-detached-retry-embedder-loop

Conversation

@andypost

Copy link
Copy Markdown

Reviewed for #405 but not taken by its merge: #405 merged as 7545594f from head
031cc9b3, and this fold landed after that head.

A failed FINISH report was retried only from libunit's own read loops
(nxt_unit_read_buf(), nxt_unit_run_ctx()). An application that drives its own
event loop never enters them, so the router held such a worker detached — out of
the idle economy, its slot counted against "processes": {"max"} — until
unrelated traffic happened to arrive.

nxt_unit_process_port_msg_impl() and nxt_unit_run_shared() now run a pending
retry, and nxt_unit_shared_port_recv() bounds its wait while one is pending;
without that the run_shared retry never fires, because the loop blocks until a
request arrives. A retry that completes a deferred graceful quit returns
NXT_UNIT_AGAIN, which Node and Python ASGI already treat as "nothing to do";
the give-up returns NXT_UNIT_ERROR. nxt_unit.h now states that an integration
driving its own event loop must give libunit a periodic wake-up while it uses
nxt_unit_request_done_detached().

No live exposure today: PHP is the only caller of that API and it uses
nxt_unit_run(). This is an API-contract fix for the Node, Python ASGI and Go
integrations.

Validation: debug and release builds, 0 warnings; build/tests 50 passed;
build/unit_port_recv_test 23 passed (22 before this commit); test_php_detached_max.py
and test_app_start_timeout.py 24 passed. The new nxt_port_recv_test_deferred_quit_port_msg()
was mutation-checked against reverted production code. The run_shared retry and its
bounded wait have no test: no in-tree application both uses that loop and calls the
detached API, and the harness has no shared-port fixture; they mirror the tested
per-context path.

🤖 Generated with Claude Code

A FINISH report that fails is retried, but only from libunit's own read
loops.  An application that drives its own event loop never enters one: it
calls nxt_unit_process_port_msg() per readable descriptor.  Its failed
FINISH was therefore never re-sent, and the router held the worker
detached -- out of the idle queues, counting against "processes": {"max"}
-- until unrelated traffic arrived, if it ever did.

Run a pending retry at the top of nxt_unit_process_port_msg_impl(), before
anything that can return early, and guard it on ctx_impl->online.  A retry
may complete a graceful quit that was deferred on the detached state,
which removes the read port; return NXT_UNIT_AGAIN in that case, because a
receive would then wait for a message the router will never send, and the
embedder stops rescheduling on NXT_UNIT_AGAIN.  A give-up propagates as
NXT_UNIT_ERROR.

nxt_unit_run_shared() gets the same retry: a request delivered on the
shared port runs the application handler too, so that loop can end
detached work.  For it to fire, nxt_unit_shared_port_recv() has to bound
its wait while a retry is pending, the way nxt_unit_ctx_port_recv()
already does; without that the loop blocks in the shared-port read until a
request arrives and the retry never runs.

nxt_unit.h states that an integration driving its own event loop must give
libunit a periodic wake-up while it uses
nxt_unit_request_done_detached().  New test hook
nxt_unit_test_ctx_read_port(), and a test that
nxt_unit_process_port_msg() runs a pending retry and reports "no message"
when that retry completes a deferred quit.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 026468b104

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

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".

Comment thread src/nxt_unit.c
Comment on lines +5814 to +5815
if (nxt_slow_path(ctx_impl->detached_retries > 0)) {
rc = nxt_unit_ctx_detached_retry(ctx);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Run shared retries even after readiness is cleared

When a shared-port request reaches request_limit, the generated graceful QUIT can be consumed by the main read loop while the detached handler is still running, clearing ready but leaving the context online because it is detached. If the handler then returns and its FINISH send fails, nxt_unit_process_msg() arms detached_retries, but the outer while (nxt_unit_chk_ready(ctx)) exits before this new retry block is reached; the main loop may already be blocked again, so no retry occurs until unrelated traffic arrives and the router continues holding the worker detached. The shared loop needs to execute pending retries while the context remains online, even after readiness has been cleared.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant