Repository navigation
Report asynchronous JSON-RPC body write failures with message context - #1946
Open
Shobhit Gupta (shobhit26-09) wants to merge 2 commits into
Open
Shobhit Gupta (shobhit26-09) wants to merge 2 commits into
Shobhit Gupta (shobhit26-09) wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues block the JSON-RPC body-write error-handling change.
No open issues 🎉
What changed in this PR
Ensures asynchronous JSON-RPC body-write failures reach the writer’s existing error handler, preserving message context and incrementing the error count. Framing, write ordering, and the original rejection error remain unchanged; broader transport-error and restart behavior is outside this PR’s scope.
Changes:
- Await body writes so rejected promises reach
handleError(error, msg). - Add deterministic header- and body-failure tests checking error identity, message context, and count.
| File | Description |
|---|---|
jsonrpc/src/node/test/messages.test.ts |
Adds header- and body-write failure regression tests with stream cleanup. |
jsonrpc/src/common/messageWriter.ts |
Awaits the body write inside the existing error-handling block. |
Review effort: Balanced
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.
Summary
Await the message-body write inside
WriteableStreamMessageWriter.doWriteso its asynchronous rejection reaches the existing catch block andhandleError(error, msg).The header write is awaited, but the body write currently returns its promise directly. If the header succeeds and the body rejects, the caller still receives the rejection, but the writer skips the message-associated error event and error-count increment. A transport-level error event may still fire without that context.
This makes body failure reporting consistent with header failure reporting without changing framing, write ordering, or the rejected error object.
Tests
Added two deterministic tests using real Node
Writablestreams: fail the header callback, or let the header succeed and fail the body callback. Both assert rejection with the original error and anonErrorevent carrying the original message and count 1. The body test fails before the change; the header test is a passing control. Streams and writers are cleaned up infinally.Validation on Linux / Node 22.23.4:
npm run compile:jsonrpc: passes.npm --prefix jsonrpc run test:node: all 81 tests pass.npm --prefix jsonrpc run lint: passes (existing ESLint option-deprecation notice only).git diff --check: passes.Related investigation: #1919 discusses missing context on transport errors and restart decisions. This PR does not fix or close that broader issue: transport-level count-less error forwarding and language-client restart policy are unchanged. It also does not change the separate request-rejection cleanup fixed by #1855.
AI assistance was used for investigation, implementation, and testing. Full monorepo/editor integration tests, Firefox, Safari, macOS and Windows were not run. Hosted CI has not yet run.