fix(acp): box handler chain links to bound dispatch stack use - #306
Open
pnkfelix wants to merge 1 commit into
Open
fix(acp): box handler chain links to bound dispatch stack use#306pnkfelix wants to merge 1 commit into
pnkfelix wants to merge 1 commit into
Conversation
Consumers that register many typed handlers on one connection (a full ACP client registers 10-20) build a deep tower of nested ChainedHandler futures. In unoptimized builds each nesting level inlined a large poll frame, so dispatching a message on a small fixed stack -- such as a macOS Grand Central Dispatch worker's 512 KiB -- overflowed and aborted the process, and very deep chains failed to compile with a layout query depth overflow. Box each chained handler's future so its state lives on the heap, constructing it in an inline(never) helper so the temporary never stages in the caller's poll frame. Per-handler stack cost drops by roughly an order of magnitude and no longer inlines with depth. Adds a regression test that dispatches through a 33-handler chain on a 512 KiB thread. See zed-industries/zed#61840.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Consumers that register many typed handlers on one connection build a tower of nested
ChainedHandlerfutures. Because each link's future was inlined into its parent's, dispatching a message polled the whole tower as one deep, recursively-inlined future. In unoptimized (debug) builds every nesting level cost a large stack frame (~14 KiB), so a realistic chain of 10–20 handlers overflowed threads with small fixed stacks.This is the crash reported in zed-industries/zed#61840: Zed registers ~10–20 typed handlers, dispatch happens on a macOS Grand Central Dispatch worker thread with a fixed 512 KiB stack (which ignores
RUST_MIN_STACK), and restoring an ACP thread crash-loops with SIGILL / EXC_BAD_ACCESS on the stack guard when aRequestPermissionRequestarrives. The only downstream workaround so far has been--config 'profile.dev.package.<crate>.opt-level=1', which every consumer would otherwise have to discover for themselves.A second symptom of the same root cause: sufficiently deep chains (33+ handlers) failed to compile at all, with
error: queries overflow the depth limit!while computing the layout of theincoming_protocol_actorasync body.Fix
Box each link of the handler chain.
ChainedHandler::handle_dispatch_fromnow dispatches each sub-handler through a small#[inline(never)] boxed_dispatchhelper returningcrate::BoxFuture, so a link's future state lives on the heap rather than inline in its ancestors' poll frames, and the temporary isn't staged in the caller's frame either. The chain-traversal semantics (handler1 then handler2, retry OR-ing) are unchanged.This is always on rather than behind a feature: cargo features must be additive, one allocation per link per message is negligible next to serde parsing, and this path already boxes every user callback future (
to_future_hack→BoxFuture) — so it's a patch-level fix with no public API change.Measurements
RUSTC_BOOTSTRAP=1 cargo rustc -- -Zprint-type-sizeson a test crate with a 17-handler chain (debug), where the generics are monomorphized:incoming_protocol_actordispatch_dispatchChainedHandler::handle_dispatch_fromThese sizes are now independent of chain depth (identical at 17 and 33 handlers).
Runtime, on a
std::thread::Builder::new().stack_size(512 * 1024)thread in a debug build:Regression test
tests/jsonrpc_deep_chain_stack.rsregisters a 33-handler chain (deeper than a full ACP client/agent registration) and dispatches a request through the whole chain on a 512 KiB thread. Before this change it aborts the process withthread 'small-stack-dispatch' has overflowed its stack; after, it passes.Downstream (Zed)
The fix is unconditional, so Zed needs no code or feature change — it takes effect on the next
agent-client-protocolbump (Zed currently pins=2.0.0). For local verification ahead of a release:after which the
opt-level=1dev-profile workaround for the affected crates should be removable.