fix(lsp): dispatch notifications off read loop#759
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a deadlock risk in the LSP client by moving server notification handling off the single protocol read-loop goroutine, enabling notification handlers to safely perform re-entrant Client.Call requests without blocking response delivery.
Changes:
- Added a per-client notification worker loop and bounded queue to serialize notification dispatch off the read loop.
- Updated the client read loop to enqueue notifications instead of invoking the handler inline.
- Added regression tests for re-entrant calls from notification handlers and notification ordering.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/lsp/client.go | Introduces a notification queue + worker goroutine so notification handlers don’t run on the protocol read loop. |
| internal/lsp/client_test.go | Adds tests for re-entrant calls inside notification handlers and for preserving notification ordering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case msg.Method != "": | ||
| c.mu.Lock() | ||
| handler := c.handler | ||
| c.mu.Unlock() | ||
| if handler != nil { | ||
| handler(msg.Method, msg.Params) | ||
| select { | ||
| case c.notify <- notification{method: msg.Method, params: msg.Params}: | ||
| case <-c.closed: | ||
| return | ||
| } |
| func TestClientNotificationHandlersPreserveOrder(t *testing.T) { | ||
| clientReader, serverWriter := io.Pipe() | ||
| client := NewClient(clientReader, io.Discard) | ||
| defer client.Close() | ||
| defer serverWriter.Close() |
|
Warning Review limit reached
Next review available in: 28 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughClient notification handling now uses a buffered queue and dedicated dispatch goroutine, allowing handlers to make client calls while preserving notification order. Tests cover re-entrant calls and ordered delivery. ChangesLSP notification dispatch
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant LSPServer
participant readLoop
participant notificationLoop
participant NotificationHandler
LSPServer->>readLoop: Send notification
readLoop->>notificationLoop: Queue method and params
notificationLoop->>NotificationHandler: Invoke handler
NotificationHandler->>readLoop: Issue Client.Call
readLoop-->>NotificationHandler: Return response
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/lsp/client_test.go`:
- Around line 230-282: Run the concurrent test
TestClientNotificationHandlerCanCallClient with Go’s race detector, using go
test -race ./... locally or in a supported CI environment, and verify that
notificationLoop and Client.Call are race-free.
In `@internal/lsp/client.go`:
- Around line 43-44: Replace the fixed-size notification channel controlled by
notificationQueueSize with an unbounded queue pattern, using an intermediate
goroutine and dynamically growing slice to decouple readLoop from slow
notification handlers. Preserve notification execution order while ensuring
protocol reads never block, including when handlers call Client.Call and more
than 64 notifications arrive before the response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 35a90c72-04a7-499a-9369-97226e4696f5
📒 Files selected for processing (2)
internal/lsp/client.gointernal/lsp/client_test.go
| func TestClientNotificationHandlerCanCallClient(t *testing.T) { | ||
| clientReader, serverWriter := io.Pipe() | ||
| serverReader, clientWriter := io.Pipe() | ||
| client := NewClient(clientReader, clientWriter) | ||
| defer client.Close() | ||
| defer serverWriter.Close() | ||
| defer clientWriter.Close() | ||
|
|
||
| serverDone := make(chan error, 1) | ||
| go func() { | ||
| body, err := readMessage(bufio.NewReader(serverReader)) | ||
| if err != nil { | ||
| serverDone <- err | ||
| return | ||
| } | ||
| var request incomingMessage | ||
| if err := json.Unmarshal(body, &request); err != nil { | ||
| serverDone <- err | ||
| return | ||
| } | ||
| serverDone <- writeMessage(serverWriter, map[string]any{ | ||
| "jsonrpc": "2.0", | ||
| "id": request.ID, | ||
| "result": map[string]bool{"applied": true}, | ||
| }) | ||
| }() | ||
|
|
||
| handlerDone := make(chan error, 1) | ||
| client.SetNotificationHandler(func(_ string, _ json.RawMessage) { | ||
| ctx, cancel := context.WithTimeout(context.Background(), time.Second) | ||
| defer cancel() | ||
| _, err := client.Call(ctx, "workspace/applyEdit", nil) | ||
| handlerDone <- err | ||
| }) | ||
| if err := writeMessage(serverWriter, map[string]any{ | ||
| "jsonrpc": "2.0", | ||
| "method": "workspace/requestEdit", | ||
| }); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| select { | ||
| case err := <-handlerDone: | ||
| if err != nil { | ||
| t.Fatalf("notification handler call failed: %v", err) | ||
| } | ||
| case <-time.After(2 * time.Second): | ||
| t.Fatal("notification handler deadlocked waiting for its response") | ||
| } | ||
| if err := <-serverDone; err != nil { | ||
| t.Fatalf("server failed: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Ensure tests are run under the race detector.
As per coding guidelines, please ensure that these new concurrent tests are executed with the Go race detector (e.g., go test -race ./...). Since the PR objectives noted that environment limitations prevented it from running, it's crucial to verify this locally or in a supported CI environment to confirm the synchronization around notificationLoop and Client.Call is completely race-free.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/lsp/client_test.go` around lines 230 - 282, Run the concurrent test
TestClientNotificationHandlerCanCallClient with Go’s race detector, using go
test -race ./... locally or in a supported CI environment, and verify that
notificationLoop and Client.Call are race-free.
Source: Coding guidelines
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P1] Preserve notifications when the dispatch queue overflows
internal/lsp/client.go:214
When a notification handler is blocked (including in the re-entrantClient.Callcase this PR adds), a server can send more than 64 notifications before replying.enqueueNotificationdiscards the oldest message, so atextDocument/publishDiagnosticsfor a different URI can be lost permanently. The correspondingsession.waitForDiagnosticsthen times out andManager.Checkreturns no diagnostics even though the server published them. This is the resolved CodeRabbit request in a different form: the follow-up commit trades the read-loop deadlock for silent protocol-event loss, andTestClientNotificationOverflowDropsOldestInOrderexplicitly codifies that loss. Use a lossless non-blocking queue (or another explicit, observable policy that preserves required diagnostics) and add an overflow regression test that asserts delivery of all relevant notifications.
Summary
Verification
n- go vet ./...n- go test ./...n- go run ./cmd/zero-release buildn- go run ./cmd/zero-release smoken- git diff HEAD --checknEnvironment limitations
Fixes fix(lsp): re-entrant notification handlers can deadlock #758
Summary by CodeRabbit