Skip to content

Commit cb88ce2

Browse files
wan9chiclaude
andcommitted
refactor(server): drop Rc; borrow handler from async state
The driver's async fn owns `handler: H` as a local. Per-client futures borrow `&handler` from the same async state — Rust's async-fn state machine makes the self-borrow sound (state is pinned, never moves). When drain completes, the outer async returns `handler` by move. Removes the internal `Rc<H>` + `Rc::try_unwrap` path (and the related panic doc). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent bc3a66e commit cb88ce2

2 files changed

Lines changed: 13 additions & 30 deletions

File tree

crates/vite_task_server/src/lib.rs

Lines changed: 7 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
use std::{
22
ffi::{OsStr, OsString},
33
io,
4-
rc::Rc,
54
sync::Arc,
65
};
76

@@ -59,24 +58,14 @@ impl StopAccepting {
5958
///
6059
/// Returns an error if creating the listener fails (on Unix, this includes
6160
/// creating the temp socket path).
62-
///
63-
/// # Panics
64-
///
65-
/// The driver future panics if, after drain, the handler still has other
66-
/// `Rc` references — which would indicate a logic bug in the server
67-
/// (per-client futures should all have dropped their clones by then).
6861
pub fn serve<'h, H: Handler + 'h>(handler: H) -> io::Result<(OsString, ServerHandle<'h, H>)> {
69-
let handler_rc = Rc::new(handler);
7062
let stop_token = CancellationToken::new();
7163
let (name, bound) = bind_listener()?;
7264

73-
let run_handler = Rc::clone(&handler_rc);
7465
let run_stop = stop_token.clone();
7566
let driver = async move {
76-
run(bound, run_handler, run_stop).await;
77-
Rc::try_unwrap(handler_rc)
78-
.ok()
79-
.expect("drain completed but handler still has other Rc references")
67+
run(bound, &handler, run_stop).await;
68+
handler
8069
}
8170
.boxed_local();
8271

@@ -120,20 +109,18 @@ const fn listener_of(bound: &Bound) -> &Listener {
120109
bound
121110
}
122111

123-
async fn run<H: Handler>(bound: Bound, handler: Rc<H>, shutdown: CancellationToken) {
112+
async fn run<H: Handler>(bound: Bound, handler: &H, shutdown: CancellationToken) {
124113
let mut clients = FuturesUnordered::new();
125114

126-
// Accept phase: accept new clients until shutdown fires. Each per-client
127-
// future gets its own `Rc<H>` clone; when they all complete the last
128-
// clone drops inside this scope along with `handler`.
115+
// Accept phase: accept new clients until shutdown fires.
129116
loop {
130117
let listener = listener_of(&bound);
131118
tokio::select! {
132119
() = shutdown.cancelled() => break,
133120
accept_result = listener.accept() => {
134121
match accept_result {
135122
Ok(stream) => {
136-
clients.push(handle_client(stream, Rc::clone(&handler)).boxed_local());
123+
clients.push(handle_client(stream, handler).boxed_local());
137124
}
138125
Err(err) => {
139126
tracing::warn!(?err, "vite_task_server: accept failed");
@@ -147,16 +134,12 @@ async fn run<H: Handler>(bound: Bound, handler: Rc<H>, shutdown: CancellationTok
147134
// Stop accepting: drop the listener (and on Unix unlink the socket file).
148135
// Existing client streams continue to work.
149136
drop(bound);
150-
// Drop our direct reference so only per-client futures hold clones.
151-
drop(handler);
152137

153-
// Drain phase: wait for all in-flight per-client tasks to finish. Each
154-
// ends when its client closes the stream (e.g., the task process exits
155-
// and its sockets are closed by the OS).
138+
// Drain phase: wait for all in-flight per-client tasks to finish.
156139
while clients.next().await.is_some() {}
157140
}
158141

159-
async fn handle_client<H: Handler>(mut stream: Stream, handler: Rc<H>) {
142+
async fn handle_client<H: Handler>(mut stream: Stream, handler: &H) {
160143
let mut buf = Vec::new();
161144
loop {
162145
match read_frame(&mut stream, &mut buf).await {

docs/runner-task-ipc/server-design.md

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ The IPC server runs per spawn execution **only when fspy is enabled**, letting t
88

99
1. **Server doesn't take a cancellation token.** The caller signals "stop accepting" via `StopAccepting::signal()`. The server has no awareness of external cancellation.
1010
2. **Handler is moved in, returned out.** The caller doesn't keep a reference. The driver owns the handler; on drain completion it returns it by value. No self-reference, no `&H` lifetime.
11-
3. **`Rc<H>` + `CancellationToken` internally**both hidden from the public API.
11+
3. **`CancellationToken` is internal** — hidden from the public API (exposed only via `StopAccepting`).
1212
4. **Driver is `!Send`**, lifetime bounded by `H`'s lifetime — if `H: 'static`, the driver is `'static`; if `H` borrows, the driver respects that.
1313

1414
## Server API
@@ -43,7 +43,7 @@ The driver future, when polled:
4343
1. **Accept phase** — accepts new clients and pumps per-client futures (`FuturesUnordered`) until `StopAccepting::signal()` fires.
4444
2. **Listener teardown** — drops listener; Unix socket file auto-cleaned via `tempfile::NamedTempFile`.
4545
3. **Drain phase** — waits for in-flight per-client futures to complete naturally (each ends on client EOF).
46-
4. **Returns `H`**via `Rc::try_unwrap` since all per-client clones have dropped.
46+
4. **Returns `H`**the owned handler that was moved in at `serve()`.
4747

4848
Dropping the driver before it resolves tears everything down immediately. Handler is dropped without being returned.
4949

@@ -81,7 +81,7 @@ struct FspyState {
8181
**Not stored:**
8282

8383
- `name` — consumed once to build envs, dropped immediately.
84-
- `handler` — lives inside `server.driver` via `Rc`; recovered by value when driver resolves.
84+
- `handler` — lives inside `server.driver`'s async state; recovered by value when the driver resolves.
8585

8686
### Driving the server during `pipe_stdio` / `child.wait`
8787

@@ -121,11 +121,11 @@ The driver future _owns_ the handler (via `Rc<H>` internally). It doesn't need t
121121

122122
If the caller wants to store `ServerHandle` in a struct without a lifetime parameter, they can use a `'static` handler (naturally satisfied by handlers that own all their state via `RefCell<...>` + cloned data).
123123

124-
### Why `Rc<H>` internally instead of `&H`?
124+
### Handler is owned by the driver, not shared via `Rc`
125125

126-
`&H` → per-client futures borrow from a single `H`. Storing both the handler and the driver in the same `FspyState` creates a self-reference.
126+
The driver's async function owns `handler: H` as a local. Per-client futures borrow `&handler` from that same async state; Rust's async-fn state machine makes this self-borrow sound (the state is pinned and never moves). All per-client futures live inside `FuturesUnordered` which is also part of the same state — borrow scopes are contained.
127127

128-
`Rc<H>`per-client futures share ownership. The driver captures `Rc<H>`, clones into each per-client future. When drain completes, `Rc::try_unwrap` succeeds because all per-client clones have dropped, and the driver returns the owned `H`.
128+
When drain completes and all per-client futures have been dropped, the outer async returns `handler` by move. No `Rc`, no `try_unwrap`, no panic possible.
129129

130130
### Why return `H` from the driver?
131131

0 commit comments

Comments
 (0)