Skip to content

serve: keep the JS wrapper alive until no dispatch can fire - #32214

Closed
alii wants to merge 9 commits into
mainfrom
ali/serve-wrapper-outlives-dispatch
Closed

serve: keep the JS wrapper alive until no dispatch can fire#32214
alii wants to merge 9 commits into
mainfrom
ali/serve-wrapper-outlives-dispatch

Conversation

@alii

@alii alii commented Jun 12, 2026

Copy link
Copy Markdown
Member

Per-route handlers are stored as WriteBarriers reachable only via the Server wrapper's m_routeList slot. stop() previously downgraded js_value immediately, so a late keep-alive request after stop+drop+GC could panic at js_value_assert_alive() or dispatch with an unreachable route list.

  • js_value.downgrade() moves from stop() into deinit_if_we_can()'s idle predicate (matching the JSNodeHTTPServerSocket clear-after-last-callback pattern)
  • Live-websocket count moves onto NewServer (so reload's context swap can't reset it); Handler gains an AnyServer backref so on_close calls deinit_if_we_can() when the last socket drains
  • stop_listening(false) also closes idle keep-alive connections so a late request can't arrive in the dead-unswept window where JsRef::Weak holds a stale address
  • Dispatch trampolines refuse with 503 once js_value is no longer strong
  • on_close copies handler.on_close to a stack local before sig.signal() so a GC there can't collect the value before the call

Tests: late keep-alive request to a route after stop+drop+GC doesn't crash; server wrapper survives GC while a websocket is connected after stop, then collects after close (fails on the released binary).

alii added 8 commits June 12, 2026 12:15
Per-route handlers are stored as WriteBarriers reachable only via the
Server JS wrapper. stop() previously downgraded js_value immediately,
so a late keep-alive request after stop+drop+GC would hit
js_value_assert_alive() with a Finalized ref and panic. Downgrade
inside the deinit_if_we_can idle predicate instead so the wrapper stays
rooted until pending_requests/listener/active websockets are all clear.

The websocket close path does not yet call deinit_if_we_can; the next
commit threads an AnyServer backref through Handler so the last close
can trigger it.
The previous commit moved the JsRef downgrade into deinit_if_we_can's
idle predicate, but nothing calls that when the last websocket closes
after a graceful stop. Thread an AnyServer backref through Handler so
on_close can trigger it; move the live-socket count onto NewServer
(where reload's context swap can no longer reset it). on_close also
copies the close handler to a stack local before sig.signal() so a GC
between the test and the call cannot collect it.
…per finalize

Idle keep-alive sockets are not counted in pending_requests, so the
wrapper can downgrade and be collected while one such socket can still
deliver another request. js_value_assert_alive() panics on Finalized;
the dispatch entry points now check first and close the connection
with 503 instead.
JsRef::Weak holds a raw JSValue, not a JSC::Weak: try_get() can return
the address of a dead-but-unswept cell, so the Finalized→503 check
alone leaves a window where dispatch reads an unrooted handler shadow.
Closing idle connections at stop() removes the late-request source;
in-flight requests are not idle and drain normally.

The on_open error-path websocket-close accounting already runs after
run_error_callback as of 65c76a2, so the live-socket count stays
nonzero across that read; no further change needed there.
JsRef::Weak holds a raw JSValue: try_get() on Weak returns the address
even when the cell is dead-but-unswept. Gating on Strong means
trampolines refuse the moment the server goes idle (downgrade) rather
than only after the wrapper destructor has run.
@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Caution

Review failed

Pull request was closed or merged during review

Walkthrough

This PR refactors WebSocket connection lifecycle management from per-handler counters to per-server accounting, adds dispatch safety guards to prevent routing to unrooted JS wrappers, defers garbage-collection-safe JS value downgrades, and includes two regression tests for GC and graceful stop scenarios.

Changes

WebSocket Lifecycle & Dispatch Safety

Layer / File(s) Summary
WebSocket handler contract and initialization
src/runtime/server/WebSocketServerContext.rs
Handler struct gains an optional server backreference field (Option<super::AnyServer>) to replace per-handler active-connection counters. The old active_connections_saturating_add/sub public methods are removed, and Handler::from_js initializes server: None.
Server-level WebSocket counting and dispatch safety
src/runtime/server/mod.rs
NewServer gains active_websocket_count: Cell<u32> to track live WebSockets at the server level. Adds js_value_for_dispatch() to check if the JS wrapper is still strongly rooted; returns None when dispatch is unsafe. Implements note_websocket_opened() and note_websocket_closed() to manage the counter. AnyServer gains on_websocket_opened() and on_websocket_closed() which decrement and trigger deinit_if_we_can() when sockets drain.
Dispatch safety guards on request paths
src/runtime/server/mod.rs, src/runtime/server/server_body.rs
Early-return guards added to on_request, on_user_route_request, and WebSocket upgrade entry points (on_user_route_request_for, on_request_for, upgrade_web_socket_user_route, on_web_socket_upgrade). When js_value_for_dispatch() returns None, handlers respond with HTTP 503 and close without invoking user code.
Server initialization and WebSocket handler wiring
src/runtime/server/mod.rs
Initialize active_websocket_count to zero in NewServer::init(). During set_routes(), wire the WebSocket handler's server backreference to point to the owning AnyServer for per-socket callback notification.
WebSocket open/close lifecycle callbacks
src/runtime/server/ServerWebSocket.rs
on_open now captures handler.server and calls server.on_websocket_opened() instead of incrementing a handler counter. on_open error path introduces a closed_here flag to track whether the socket was closed due to exception and conditionally calls server.on_websocket_closed() afterward. on_close snapshots handler.server before deferred logic, calls server.on_websocket_closed() in the defer block, and copies handler.on_close callback before borrow expiry to use in the callback invocation.
Server shutdown and lifecycle cleanup
src/runtime/server/mod.rs
During graceful stop_listening, close idle keep-alive connections via close_idle_connections() on the uWS app. Defer js_value.downgrade() to the deinit_if_we_can() path once nothing can dispatch further, replacing the previous immediate downgrade in stop().
GC and stop regression tests
test/js/bun/http/bun-server.test.ts
Two subprocess-based tests validate that the server handles GC and graceful stop correctly. One test ensures a late keep-alive request after stop() and GC cycles does not crash on stale handler dispatch. The other confirms the server wrapper survives GC while a WebSocket remains open but becomes collectable once the socket closes.
🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Description check ❓ Inconclusive The PR description provides a concise explanation of the problem (per-route handlers panic on late requests after stop+GC), describes the key changes made, and lists the tests added. However, it does not follow the required template structure with explicit 'What does this PR do?' and 'How did you verify your code works?' sections. Restructure the description to follow the required template by adding explicit 'What does this PR do?' and 'How did you verify your code works?' section headers and organizing content accordingly.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'serve: keep the JS wrapper alive until no dispatch can fire' clearly and concisely summarizes the main objective of the PR—ensuring the JavaScript wrapper persists until all potential dispatch operations are complete, which directly matches the core changes across all modified files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@robobun

robobun commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator
Updated 1:48 PM PT - Jun 12th, 2026

@autofix-ci[bot], your commit 0564a7b has 2 failures in Build #62175 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 32214

That installs a local version of the PR into your bun-32214 executable, so you can run:

bun-32214 --bun

@alii

alii commented Jun 12, 2026

Copy link
Copy Markdown
Member Author

Folded into #32215.

@alii alii closed this Jun 12, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. Release Bun.serve handler Strongs once the server is idle (native↔JS cycle leak) #32086 - Same author, same problem (server wrapper GC/lifetime after stop): moves Strong release to deinit_if_we_can, adds active_websocket_count tracking, adds 503 dispatch guards, calls close_idle_connections(). serve: keep the JS wrapper alive until no dispatch can fire #32214 appears to be a refined extraction of this PR.
  2. serve: trace handler callbacks from the wrapper instead of rooting them as Strong #32215 - Same author, substantial overlap addressing the same GC cycle problem via tracing instead of Strong release. Explicitly states it conflicts with serve: keep the JS wrapper alive until no dispatch can fire #32214 and supersedes Release Bun.serve handler Strongs once the server is idle (native↔JS cycle leak) #32086.

🤖 Generated with Claude Code

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.

2 participants