Cancel WebSocket handlers on disconnect#146
Conversation
Wire WebSocket connections to a cancellation token so long-running message handlers stop when the client closes, while preserving per-connection interpreter state. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughPR implements WebSocket handler cancellation aligned with HTTP behavior introduced in PR ChangesWebSocket Handler Cancellation
Sequence DiagramsequenceDiagram
participant Client
participant Receiver as Receiver Task
participant Channel as Message Channel
participant MainLoop as Main Loop
participant BlockPool as Blocking Pool
participant Interp as Interpreter
rect rgba(100, 150, 255, 0.5)
Note over Client,Interp: WebSocket Connection Established
Client->>Receiver: WebSocket upgrade
Receiver->>Interp: Fork interpreter, set Arc<AtomicBool>
end
rect rgba(100, 200, 150, 0.5)
Note over Client,Interp: Message Processing Loop
loop Active Connection
Receiver->>Receiver: Poll socket for Text/Close/Error
Receiver->>Channel: try_send(text_message)
MainLoop->>Channel: recv message
MainLoop->>BlockPool: spawn_blocking task
BlockPool->>Interp: enter span, lock interp
BlockPool->>Interp: call_ws_handler(text)
Interp-->>BlockPool: response
BlockPool->>Client: send response
end
end
rect rgba(255, 150, 100, 0.5)
Note over Client,Interp: Client Disconnect
Client->>Receiver: Close frame or drop
Receiver->>Interp: set Arc<AtomicBool> (cancelled = true)
Receiver->>Receiver: propagate disconnect
MainLoop->>BlockPool: running task detects cancelled
BlockPool->>Interp: exit handler at safe point
MainLoop->>Receiver: abort receiver task
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 8/10 reviews remaining, refill in 6 minutes and 7 seconds. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcdcfcdc8f
ℹ️ 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".
| if text_tx.try_send(text.to_string()).is_err() { | ||
| cancel_for_receiver | ||
| .store(true, Ordering::Release); | ||
| break; |
There was a problem hiding this comment.
Avoid dropping WS connections on brief message bursts
Using try_send on a channel of capacity 1 makes the server treat a temporarily full queue as a disconnect and immediately set the cancel flag. In practice, a client that sends two text frames back-to-back (or before the async loop has polled text_rx) can have its connection cancelled even though the server could have processed both sequentially; this is a behavioral regression from the previous loop that handled all incoming messages in order.
Useful? React with 👍 / 👎.
Summary
Closes #111.
Test plan
cargo fmtcargo test --test server_concurrency websocket_handler_cancelled_on_client_disconnectcargo test --test server_concurrencycargo testMade with Cursor
Summary by CodeRabbit
Bug Fixes
Tests