fix: error in the response body should destroy the response stream in Node#3012
fix: error in the response body should destroy the response stream in Node#3012gmaclennan wants to merge 2 commits intoardatan:masterfrom
Conversation
…nt unhandled rejections When a response stream is errored, the reader.read() promise rejects. Previously, this rejection was unhandled, causing 'Unexpected error while handling request' to be logged. This fix: - Catches error and destroys the response stream / closes the socket - Catches any error from reader.cancel() to prevent unhandled rejections fixes ardatan#3011
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThe changes enhance error handling in the streaming response pipeline by destroying the server response when downstream errors occur during chunk writing, and by gracefully ignoring failures from reader cancellation. A test verifies that aborting a response stream properly closes the underlying socket. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/server/src/utils.ts(2 hunks)packages/server/test/abort.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/server/test/abort.spec.ts (2)
packages/promise-helpers/src/index.ts (2)
resolve(114-116)reject(117-119)packages/server/src/createServerAdapter.ts (1)
createServerAdapter(556-556)
🪛 GitHub Check: unit / bun
packages/server/test/abort.spec.ts
[failure] 85-85: error: Expected reader to be closed
at <anonymous> (/home/runner/work/whatwg-node/whatwg-node/packages/server/test/abort.spec.ts:85:30)
[failure] 44-44: AbortError: The operation was aborted.
at /home/runner/work/whatwg-node/whatwg-node/packages/server/test/abort.spec.ts:44
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: server (undici)
- GitHub Check: unit / node 24
- GitHub Check: unit / node 18
- GitHub Check: unit / node 25
- GitHub Check: unit / node 20
- GitHub Check: server (ponyfill)
- GitHub Check: node-fetch (noConsumeBody)
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: unit / deno
- GitHub Check: server (native)
- GitHub Check: server (uws)
🔇 Additional comments (2)
packages/server/src/utils.ts (1)
400-401: LGTM! Prevents unhandled rejections from reader cancellation.The
.catch(() => {})properly handles cases wherereader.cancel()fails (e.g., stream already closed or errored), preventing unhandled promise rejections.packages/server/test/abort.spec.ts (1)
40-100: Test expectations may not align with stream error propagation behavior.The test expects
reader.read()to throw an error after abort, but aborting a pipeTo operation cancels the source and aborts the destination stream, which may cause the reader to complete normally (done=true) rather than throwing. Verify whether the abort signal propagation through the transform stream properly errors the client-side reader, or if it only causes a graceful EOF. Check if serverResponse destruction sends an error frame (e.g., RST_STREAM with error code) or just closes the connection, as this affects whether the client reader receives an error or normal completion.
| ) | ||
| .catch(err => { | ||
| if (!serverResponse.destroyed) { | ||
| serverResponse.destroy(err); | ||
| } else { | ||
| // If the serverResponse is already destroyed, re-throw the error to be handled upstream | ||
| throw err; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Potential unhandled rejection when re-throwing after serverResponse is already destroyed.
The re-throw at line 416 could cause an unhandled rejection because sendReadableStream() returns this promise (line 420), but the caller at line 385 in sendNodeResponse() doesn't handle the returned promise. When serverResponse is already destroyed and the error is re-thrown, there's no upstream catch handler.
Consider removing the else branch and the re-throw. If serverResponse is already destroyed, there's nothing more to do—the socket is closed and further error propagation won't help.
🔎 Proposed fix
.catch(err => {
if (!serverResponse.destroyed) {
serverResponse.destroy(err);
- } else {
- // If the serverResponse is already destroyed, re-throw the error to be handled upstream
- throw err;
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ) | |
| .catch(err => { | |
| if (!serverResponse.destroyed) { | |
| serverResponse.destroy(err); | |
| } else { | |
| // If the serverResponse is already destroyed, re-throw the error to be handled upstream | |
| throw err; | |
| } | |
| }); | |
| ) | |
| .catch(err => { | |
| if (!serverResponse.destroyed) { | |
| serverResponse.destroy(err); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In packages/server/src/utils.ts around lines 410 to 418, the catch handler
re-throws the error when serverResponse is already destroyed which can cause an
unhandled promise rejection because the caller doesn't await or catch the
returned promise; remove the else branch and the re-throw so that when
serverResponse.destroyed is true the error is not re-thrown (optionally
log/debug it instead), leaving only the serverResponse.destroy(err) path and
ensuring the promise settles without propagating the error upstream.
There was a problem hiding this comment.
@ardatan not sure what you want to do here. Previously the error was thrown (potentially unhandled), and this change ensures the serverResponse is destroyed when there is an error, but I re-throw to maintain existing behaviour (this thrown error is caught by the wrapper). I could change this to swallow the error, but not sure that's the right approach?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
1 participant
Description
When a response stream is errored, the
reader.read() promise rejects. Previously, this
rejection was unhandled, causing an unhandled rejection. Also, this normally results in the request stream closing with an error, which was resulting in an unhandled error when trying to cancel (the already errored) response body stream.
This fix:
Related #3011
Type of change
Please delete options that are not relevant.
expected)
Screenshots/Sandbox (if appropriate/relevant):
Adding links to sandbox or providing screenshots can help us understand more about this PR and take
action on it as appropriate
How Has This Been Tested?
I have tested this in our codebase and it fixes the issue which was being caught in our tests.
I have tried to write a test for this but I was unable to get the tests running locally and I could not find any instructions in the repo.
Test Environment:
package-name:@whatwg-node/serverChecklist:
CONTRIBUTING doc and the
style guidelines of this project
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose
the solution you did and what alternatives you considered, etc...