fix(server): do not hang when server adapter's request is leaked and attempted to parse outside#2208
fix(server): do not hang when server adapter's request is leaked and attempted to parse outside#2208
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis update patches an issue in the Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
🚀 Snapshot Release (
|
| Package | Version | Info |
|---|---|---|
@whatwg-node/node-fetch |
0.7.15-alpha-20250324224943-fe2e257e455b2c5d9f69fb4b30dd1be84d2217e1 |
npm ↗︎ unpkg ↗︎ |
@whatwg-node/server |
0.10.3-alpha-20250324224943-fe2e257e455b2c5d9f69fb4b30dd1be84d2217e1 |
npm ↗︎ unpkg ↗︎ |
✅
|
✅
|
✅
|
✅
|
✅
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/heavy-worms-battle.md (1)
5-6: Fix the grammatical issue in the description.The sentence structure is incorrect. The verb "prevent" should be followed by "from" + gerund, not "to" + verb.
-This PR prevents it to hang and throw an error if the readable stream is destroyed earlier. +This PR prevents it from hanging and throws an error if the readable stream is destroyed earlier.🧰 Tools
🪛 LanguageTool
[uncategorized] ~6-~6: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...it used to hang. This PR prevents it to hang and throw an error if the readable stre...(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/heavy-worms-battle.md(1 hunks)packages/node-fetch/src/Body.ts(2 hunks)packages/server/test/reproductions.spec.ts(2 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
packages/server/test/reproductions.spec.ts (1)
packages/server/src/utils.ts (1) (1)
createDeferredPromise(13-13)
packages/node-fetch/src/Body.ts (1)
packages/promise-helpers/src/index.ts (2) (2)
fakePromise(47-94)fakeRejectPromise(166-202)
🪛 LanguageTool
.changeset/heavy-worms-battle.md
[uncategorized] ~6-~6: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...it used to hang. This PR prevents it to hang and throw an error if the readable stre...
(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: unit / deno
- GitHub Check: unit / bun
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: unit / node 20
- GitHub Check: unit / node 18
- GitHub Check: e2e / azure-function
🔇 Additional comments (7)
.changeset/heavy-worms-battle.md (1)
8-15: Example demonstrates the fix correctly.Good example showing how the behavior has changed from hanging to throwing an error when a request method is called outside the server adapter scope.
packages/node-fetch/src/Body.ts (3)
5-5: Import of fakeRejectPromise is correctly added.The import of
fakeRejectPromiseis necessary for the fix and aligns with the existing architecture.
171-171: Important initialization added.Initializing
this._chunksto an empty array at the beginning of the method ensures consistent behavior regardless of the code path taken.
175-179:Details
✅ Verification successful
Critical fix for the hanging issue.
This check is the core of the fix - detecting when a readable stream has been destroyed and throwing an appropriate error instead of hanging indefinitely. The error message clearly indicates what happened to help with debugging.
🏁 Script executed:
#!/bin/bash # Let's verify if this error message is consistent with other similar checks in the codebase rg -A 2 -B 2 "stream has (already been consumed|been destroyed)" --type ts --no-filenameLength of output: 421
Critical fix verified: Readable stream handling is consistent.
The error message in the code segment (lines 175-179 in
packages/node-fetch/src/Body.ts) correctly detects when a readable stream has already been destroyed or consumed, and it provides a clear, consistent error message. The verification usingrgconfirms that this approach aligns with similar checks elsewhere in the codebase, ensuring proper rejection to avoid hanging issues.packages/server/test/reproductions.spec.ts (3)
54-81: Good test for validating the fix.This test case properly validates that the fix works as expected by verifying that
req.text()throws the appropriate error message when called outside the server adapter scope. The test follows the same pattern as the example in the changeset.A few observations:
- The test is conditionally included based on environment (excluding Bun and Deno)
- It uses modern JavaScript features like
await usingfor resource management- It correctly sets up a minimal reproduction case
56-56: Properly utilizing createDeferredPromise.The deferred promise pattern is appropriate here to capture the request object outside of the handler scope.
75-79: Proper error handling and assertion.The test correctly verifies that the expected error is thrown with the appropriate message, validating the fix implemented in Body.ts.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/node-fetch/src/fetchNodeHttp.ts (1)
128-146: Enhanced body handling with buffer optimization.This change introduces a performance optimization by directly writing the buffer to the request when available, instead of always creating a readable stream. This is a good improvement that simplifies the request sending process in many cases.
For better readability, consider extracting the body handling logic into separate helper functions:
- if (fetchRequest['_buffer'] != null) { - nodeRequest.write(fetchRequest['_buffer']); - // @ts-expect-error Avoid arguments adaptor trampoline https://v8.dev/blog/adaptor-frame - nodeRequest.end(null, null, null); - } else { - const nodeReadable = ( - fetchRequest.body != null - ? isNodeReadable(fetchRequest.body) - ? fetchRequest.body - : Readable.from(fetchRequest.body) - : null - ) as Readable | null; - if (nodeReadable) { - nodeReadable.pipe(nodeRequest); - } else { - // @ts-expect-error Avoid arguments adaptor trampoline https://v8.dev/blog/adaptor-frame - nodeRequest.end(null, null, null); - } - } + sendRequestBody(fetchRequest, nodeRequest);And define the function at the bottom of the file:
function sendRequestBody(fetchRequest: PonyfillRequest<any>, nodeRequest: any) { if (fetchRequest['_buffer'] != null) { nodeRequest.write(fetchRequest['_buffer']); // @ts-expect-error Avoid arguments adaptor trampoline https://v8.dev/blog/adaptor-frame nodeRequest.end(null, null, null); return; } const nodeReadable = ( fetchRequest.body != null ? isNodeReadable(fetchRequest.body) ? fetchRequest.body : Readable.from(fetchRequest.body) : null ) as Readable | null; if (nodeReadable) { nodeReadable.pipe(nodeRequest); } else { // @ts-expect-error Avoid arguments adaptor trampoline https://v8.dev/blog/adaptor-frame nodeRequest.end(null, null, null); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/node-fetch/src/Body.ts(1 hunks)packages/node-fetch/src/fetchNodeHttp.ts(1 hunks)packages/server/test/reproductions.spec.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/server/test/reproductions.spec.ts
🧰 Additional context used
🧬 Code Definitions (1)
packages/node-fetch/src/fetchNodeHttp.ts (1)
packages/node-fetch/src/utils.ts (1) (1)
isNodeReadable(34-36)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: e2e / azure-function
- GitHub Check: e2e / aws-lambda
- GitHub Check: node-fetch (noConsumeBody)
🔇 Additional comments (1)
packages/node-fetch/src/Body.ts (1)
171-172:Details
✅ Verification successful
Good fix for the hanging issue.
This change properly handles the case when the readable stream is destroyed before method calls, which resolves the hanging issue described in the PR. By initializing
this._chunksat the beginning and checking if the readable stream is destroyed, you ensure that an empty array is returned rather than hanging indefinitely.Let's verify that this change properly handles destroyed readable streams:
🏁 Script executed:
#!/bin/bash # Check if there are other places in the codebase that handle destroyed readable streams rg "readable\?\.\s*destroyed" --type tsLength of output: 220
Fix Verified: Improved Handling for Destroyed Streams
The updated code now initializes
this._chunksand checks for a destroyed readable stream usingif (!_body || _body.readable?.destroyed), which prevents the hanging issue as intended. This approach is consistent with similar checks elsewhere in the codebase (e.g., the checkif (this._generatedBody?.readable?.destroyed && this._buffer)in the same file).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/node-fetch/src/Body.ts (1)
474-488: Graceful handling ofIncomingMessagewithPassThroughand abort signals.The new code properly pipes the
IncomingMessagethrough aPassThroughwithsignal. Be aware that@ts-expect-errormay conceal type mismatches if Node typings evolve. Regularly confirm thatsignalusage is properly recognized by the environment.One minor suggestion: consider adding explicit error handling or logging if the
IncomingMessageis destroyed prematurely, for easier debugging.packages/server/src/utils.ts (1)
226-229: Aborting piped PassThrough streams.Piping
rawRequestto aPassThroughwith{ signal: controller.signal }correctly enables request abortion. However, ensure that any unhandled rejections or errors onrawRequestalso close the PassThrough to prevent memory leaks.Consider adding explicit error event listeners on
rawRequestor the PassThrough stream to handle abrupt termination, ensuring resources are released promptly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/node-fetch/src/Body.ts(8 hunks)packages/server/src/utils.ts(2 hunks)packages/server/test/reproductions.spec.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/server/test/reproductions.spec.ts
🧰 Additional context used
🪛 GitHub Actions: e2e
packages/server/src/utils.ts
[warning] 1-1: The package 'node:stream' wasn't found on the file system but is built into node. Your Worker may throw errors at runtime unless you enable the 'nodejs_compat' compatibility flag.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: unit / node 20
- GitHub Check: unit / node 18
🔇 Additional comments (9)
packages/node-fetch/src/Body.ts (8)
3-4: Note the warning aboutnode:streamusage in certain environments.Since
node:streamis built into Node.js and might require a specific compatibility flag in certain runtimes (for instance, "nodejs_compat"), ensure that the execution environment permits usage of built-in modules without errors.Please confirm if the runtime environment (like a worker or edge environment) enables Node.js compatibility for
node:stream.
48-48: Optional abort signal is a good improvement for concurrency.Introducing the optional
signal?: AbortSignal;helps handle request abortion gracefully, preventing potential memory leaks or hanging streams when operations are canceled.
60-63: Validatesignalpropagation.The call to
processBodyInitwithoptions?.signalis appropriate. Just ensure that the rest of the code path checks thesignal?.abortedstate and handles it cleanly.Please confirm any upstream usage verifying that any
abortevent onsignalproperly terminates the request streams.
176-176: Ensure chunk re-initialization logic is correct.Re-initializing
_chunksto an empty array before collecting avoids data carryover from previous reads. This looks correct, but be mindful if_collectChunksFromReadablecan be called multiple times concurrently.
380-383: Updated function signature forprocessBodyInitimproves clarity.Adding
signal?: AbortSignalclarifies intent and allows for more robust abort handling.
411-423: Buffer handling and readable creation.The logic to convert
Bufferto a readable stream usingReadable.from(buffer)is straightforward. This helps unify the code path for subsequent operations. Ensure large buffers don’t cause performance or memory issues when read repeatedly.
440-447: Reusing an existingPonyfillReadableStreamis efficient.Returning the same
PonyfillReadableStreamifbodyInitis already aPonyfillReadableStreamavoids unnecessary overhead. This is a clean approach.
449-458: Blob handling looks consistent.Calling
blob.stream()to generate the readable stream seamlessly unifies the flow with other body types. This is an idiomatic approach for Blob data.packages/server/src/utils.ts (1)
4-4: Same note aboutnode:streamcompatibility.As mentioned in the previous file, ensure the target environment supports built-in modules like
node:stream. The pipeline warnings suggest enabling compatibility if needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/node-fetch/src/Body.ts (1)
1-586: 💡 Verification agent🧩 Analysis chain
Consider adding tests for AbortSignal integration.
The implementation now supports
AbortSignalthroughout the body processing logic, but it would be valuable to ensure there are corresponding tests that verify the behavior when a signal is aborted during body processing.
🏁 Script executed:
#!/bin/bash # Check if there are any tests for the AbortSignal functionality # Search for test files echo "Searching for tests related to AbortSignal..." rg -l "AbortSignal|signal.*abort|aborted.*signal" --type ts --type js --glob "*/tests/*" # Look for test cases that specifically test the abort functionality with body processing echo "Searching for test cases for abort functionality in body processing..." rg -A 10 -B 2 "body.*signal.*abort|abort.*body.*signal" --type ts --type js --glob "*/tests/*"Length of output: 737
Action Required: Add Tests for AbortSignal Integration
The implementation correctly propagates the AbortSignal through body processing; however, our investigation did not reveal any tests validating abort behavior. Please add tests to verify that aborting the signal during various stages (e.g., during stream generation and chunk collection) results in the expected cancellation or error handling.
- Location:
packages/node-fetch/src/Body.ts- Recommendation: Create unit/integration tests to simulate abort scenarios (for example, with string, buffer, and stream bodies) to ensure AbortSignal triggers abort behavior as intended.
🧹 Nitpick comments (2)
.changeset/heavy-worms-battle.md (1)
6-6: Grammar correction needed.The sentence structure can be improved for better clarity.
-This PR prevents it to hang and throw an error if the readable stream is destroyed earlier. +This PR prevents it from hanging and throws an error if the readable stream is destroyed earlier.🧰 Tools
🪛 LanguageTool
[uncategorized] ~6-~6: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...it used to hang. This PR prevents it to hang and throw an error if the readable stre...(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
packages/node-fetch/src/Body.ts (1)
485-509: Consider adding error message for destroyed streams.When a stream is destroyed prematurely, a more descriptive error message could be useful for debugging.
pipeline(bodyInit, passthrough, { end: true, signal, }) .then(() => { if (!bodyInit.destroyed) { bodyInit.resume(); } }) .catch(e => { + // If no specific error and the stream is destroyed, provide a clearer message + if (!e && bodyInit.destroyed) { + e = new Error('The readable stream was destroyed before it could be processed'); + } passthrough.destroy(e); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.changeset/heavy-worms-battle.md(1 hunks)packages/node-fetch/src/Body.ts(10 hunks)packages/node-fetch/src/fetchNodeHttp.ts(3 hunks)packages/node-fetch/tests/fetch.spec.ts(2 hunks)packages/server/src/utils.ts(0 hunks)packages/server/test/reproductions.spec.ts(2 hunks)packages/server/test/test-fetch.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/server/src/utils.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/server/test/test-fetch.ts
- packages/node-fetch/tests/fetch.spec.ts
- packages/server/test/reproductions.spec.ts
- packages/node-fetch/src/fetchNodeHttp.ts
🧰 Additional context used
🧬 Code Definitions (1)
packages/node-fetch/src/Body.ts (3)
packages/promise-helpers/src/index.ts (2)
MaybePromise(1-1)handleMaybePromise(29-42)packages/node-fetch/src/Blob.ts (2)
PonyfillBlob(72-321)PonyfillBlob(323-326)packages/node-fetch/src/ReadableStream.ts (1)
PonyfillReadableStream(58-246)
🪛 LanguageTool
.changeset/heavy-worms-battle.md
[uncategorized] ~6-~6: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...it used to hang. This PR prevents it to hang and throw an error if the readable stre...
(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: unit / bun
- GitHub Check: unit / node 20
- GitHub Check: unit / node 18
🔇 Additional comments (10)
.changeset/heavy-worms-battle.md (1)
5-16: Good fix and clear explanation of the issue.The changeset provides a clear explanation of the issue and the solution. The example is helpful in demonstrating the behavior change from hanging to throwing an error when a
Requestmethod is called outside the server adapter scope.🧰 Tools
🪛 LanguageTool
[uncategorized] ~6-~6: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...it used to hang. This PR prevents it to hang and throw an error if the readable stre...(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
packages/node-fetch/src/Body.ts (9)
3-5: LGTM - Required imports for stream handling.The added imports are necessary for handling IncomingMessage objects and using the stream pipeline for proper error propagation.
49-49: LGTM - Good addition of AbortSignal support.Adding the optional
signalproperty to PonyfillBodyOptions allows for cancellation of long-running operations, which is crucial for preventing hanging when streams are destroyed.
61-64: LGTM - Properly propagating the signal to processBodyInit.The constructor now correctly passes the signal from options to the processBodyInit function, ensuring abort signals are properly used throughout the body processing logic.
143-143: LGTM - Improved type definition for chunks.Using
MaybePromise<Uint8Array[]>type allows for better handling of both synchronous and asynchronous chunk collections.
145-190: LGTM - Well-structured chunk collection implementation.The implementation of
_doCollectChunksFromReadableJobproperly handles different stream types and ensures chunks are correctly accumulated. The error handling has been improved to properly propagate errors when streams are destroyed.
192-198: LGTM - Effective memoization pattern.The refactored
_collectChunksFromReadablemethod implements a solid memoization pattern that prevents redundant chunk collection operations. The check for existing chunks before initiating a new collection job is an important optimization.
391-394: LGTM - Updated function signature with AbortSignal support.The updated signature of
processBodyInitnow includes the optional AbortSignal parameter, allowing for proper cancellation of stream processing.
423-423: LGTM - Improved type annotations for better code clarity.The added type annotations across various sections enhance code readability and type safety, making the code easier to understand and maintain.
Also applies to: 430-430, 451-452, 454-454, 460-461, 466-466
485-509: Well-implemented handling of IncomingMessage.This is the core fix for the hanging issue. The implementation properly handles IncomingMessage instances by:
- Creating a PassThrough stream that respects the abort signal
- Using pipeline to safely pipe data with proper error propagation
- Cleaning up resources by resuming the original stream when needed
- Propagating errors to the PassThrough stream
The implementation ensures that if the readable stream is destroyed prematurely, an error will be thrown instead of hanging indefinitely.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/server/test/test-server.ts(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/server/test/test-server.ts (2)
packages/server/src/createServerAdapter.ts (1)
DisposableSymbols(473-478)packages/server/src/types.ts (2)
ServerAdapter(127-132)ServerAdapterBaseObject(12-22)
🪛 Biome (1.9.4)
packages/server/test/test-server.ts
[error] 188-188: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: unit / deno
- GitHub Check: unit / bun
- GitHub Check: unit / node 20
- GitHub Check: unit / node 18
- GitHub Check: type check
- GitHub Check: server (ponyfill)
- GitHub Check: e2e / azure-function
🔇 Additional comments (6)
packages/server/test/test-server.ts (6)
127-127: Looks good: Unconditional Node.js HTTP server registration.Moving the Node.js HTTP server implementation assignment outside of conditional blocks ensures this server type is always available for testing, which aligns with the PR objective to address request handling issues.
129-180: Good implementation of Express test server.The Express test server implementation follows the established pattern for test servers and properly handles resource cleanup with socket management and server shutdown.
190-227: Good implementation of Fastify route handler and server setup.The implementation correctly handles all HTTP methods and properly manages the adapter pattern for request processing. The URL host replacement from '127.0.0.1' to 'localhost' ensures consistent behavior across test environments.
228-263: Proper resource cleanup in Fastify server implementation.The implementation correctly handles async disposal, socket cleanup, and server shutdown. The adapter pattern is properly implemented for context handling.
265-295: Good runtime-specific server implementation organization.The conditional logic for including Bun and Deno server implementations based on the runtime environment is well structured and maintainable.
516-516: Simplified server name handling.Directly returning the server implementation name instead of accessing it through the global server map is a good simplification that eliminates potential issues with property access.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/server/test/test-server.ts (1)
150-150: Consider more robust server port access patternThe current pattern using optional chaining followed by type assertion could cause runtime errors if
serveris unexpectedly undefined. Since this is used in a property that might be accessed multiple times, consider using a more defensive approach.- url: `http://localhost:${(server?.address() as AddressInfo).port}`, + url: `http://localhost:${server && (server.address() as AddressInfo)?.port || 0}`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/server/test/node.spec.ts(4 hunks)packages/server/test/test-server.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/server/test/node.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: unit / deno
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: unit / bun
- GitHub Check: unit / node 20
- GitHub Check: unit / node 18
- GitHub Check: e2e / cloudflare-workers
- GitHub Check: e2e / azure-function
- GitHub Check: server (native)
🔇 Additional comments (5)
packages/server/test/test-server.ts (5)
129-180: Good addition of Express server implementationThe Express server adapter implementation is well-structured and follows the same pattern as other server adapters in the file. It correctly handles resource cleanup, socket tracking, and implements the required TestServer interface methods.
189-189: Replace {} with a more precise type for stricter object typingIt's recommended not to use
{}since it matches any non-nullish value. Based on TypeScript best practices, please update the adapter declaration by replacing{}withRecord<string, unknown>to enforce that only objects are passed.- ServerAdapterBaseObject<{}> + ServerAdapterBaseObject<Record<string, unknown>>🧰 Tools
🪛 Biome (1.9.4)
[error] 189-189: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
299-299: Replace {} with a more precise type for stricter object typingIt's recommended not to use
{}since it matches any non-nullish value. Based on TypeScript best practices, please update the adapter declaration by replacing{}withRecord<string, unknown>to enforce that only objects are passed.- let adapter: ServerAdapter<Context, ServerAdapterBaseObject<{}>>; + let adapter: ServerAdapter<Context, ServerAdapterBaseObject<Record<string, unknown>>>;🧰 Tools
🪛 Biome (1.9.4)
[error] 299-299: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
392-392: Replace {} with a more precise type for stricter object typingIt's recommended not to use
{}since it matches any non-nullish value. Based on TypeScript best practices, please update the adapter declaration by replacing{}withRecord<string, unknown>to enforce that only objects are passed.- let adapter: ServerAdapter<HapiContext, ServerAdapterBaseObject<{}>>; + let adapter: ServerAdapter<HapiContext, ServerAdapterBaseObject<Record<string, unknown>>>;🧰 Tools
🪛 Biome (1.9.4)
[error] 392-392: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
127-127: Server implementation conditionals are now more organizedThe reorganization of server implementation assignments with clear conditionals (Bun/Deno/others) improves maintainability and readability. Moving the
node:httpassignment outside the conditionals ensures it's always available for testing.Also applies to: 267-297
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/node-fetch/src/Body.ts (2)
142-142: Initialization of_chunkswithMaybePromise<Uint8Array[]> | null.
Deferring chunk collection is helpful for stream-based bodies. Just be cautious about concurrent calls to_collectChunksFromReadable()which might trigger multiple reads.Consider adding a short comment clarifying concurrency assumptions if multiple consumers attempt to collect chunks simultaneously.
178-183: Chunk collection from classic readable stream.
The approach of storing all chunks in an array is straightforward. If you expect very large streams, consider a streamed approach or partial read to avoid high memory usage.packages/node-fetch/src/ReadableStream.ts (1)
95-110: Use ofhandleMaybePromiseinreadImpl.
This ensures consistent error handling and asynchronous operation chaining. Looks good, but confirm that partial reads won’t be lost ifcontroller._flush()is called duringpull.Include a comment or test block verifying correct partial read behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/node-fetch/src/Body.ts(10 hunks)packages/node-fetch/src/ReadableStream.ts(3 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
packages/node-fetch/src/Body.ts (4)
packages/promise-helpers/src/index.ts (3)
MaybePromise(1-1)handleMaybePromise(29-42)fakePromise(47-94)packages/node-fetch/src/utils.ts (1)
fakePromise(28-28)packages/node-fetch/src/Blob.ts (2)
PonyfillBlob(72-321)PonyfillBlob(323-326)packages/node-fetch/src/ReadableStream.ts (1)
PonyfillReadableStream(59-269)
packages/node-fetch/src/ReadableStream.ts (1)
packages/promise-helpers/src/index.ts (1)
handleMaybePromise(29-42)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: unit / node 20
- GitHub Check: unit / deno
- GitHub Check: unit / bun
- GitHub Check: unit / node 18
🔇 Additional comments (12)
packages/node-fetch/src/Body.ts (9)
3-4: Imports look fine.
The addition ofIncomingMessage,PassThrough, andReadablesupports the new logic for piping incoming message streams.
48-48: Ensure the abort signal is fully honored.
Withsignal?: AbortSignalnow available inPonyfillBodyOptions, confirm that downstream code properly checks forsignal.abortedand aborts reading if triggered.Do you have tests confirming an abort event is correctly handled?
60-63: Signal forwarding toprocessBodyInit.
Passing the abortsignalintoprocessBodyInitis good for consistent cancellation support. Ensure external user code also has the same signature update to avoid breakage.
144-172: Async iterable chunk collection logic.
This new_doCollectChunksFromReadableJob()method correctly collects data fromAsyncIterable<Uint8Array>. The usage ofhandleMaybePromiseensures consistent handling of both synchronous and asynchronous contexts.
191-197: Refined_collectChunksFromReadablelogic.
This method properly avoids re-collecting if_chunksalready exists, reducing duplication. It’s a welcome improvement in readability.
390-393: ExtendedprocessBodyInitsignature.
Including thesignalparameter here is aligned with the rest of the code. Keep an eye on any external calls that might miss the updated signature.
422-422: Minor typed buffer assignment.
This typed declaration is harmless and clarifies thatbodyInitis indeed aBuffer.
485-492: IncomingMessage piping with support forAbortSignal.
The use ofPassThroughwithsignalis appropriate. Ensure that events like 'error' and 'close' onbodyInitare also handled or bubbled up if needed.
494-502: Returning aPonyfillReadableStreamfrom the piped PassThrough.
This correctly integrates node’sIncomingMessagewith the ponyfill’s stream model.packages/node-fetch/src/ReadableStream.ts (3)
3-3: New import forhandleMaybePromise.
Great addition to unify promise-like and synchronous flows. This improves the code’s consistency.
75-94: Refactor to introducehandleStartwithhandleMaybePromise.
Splitting out initialization logic intohandleStartclarifies the startup sequence. Consider verifying that exceptions thrown insideunderlyingSource.startget properly handled.
268-269: Added[Symbol.toStringTag]property.
This property improves debugging by showing'ReadableStream'in logs and inspections. Nicely done.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/node-fetch/src/Body.ts(10 hunks)packages/server/src/utils.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/server/src/utils.ts
🧰 Additional context used
🧬 Code Definitions (1)
packages/node-fetch/src/Body.ts (3)
packages/promise-helpers/src/index.ts (3)
MaybePromise(1-1)handleMaybePromise(29-42)fakePromise(47-94)packages/node-fetch/src/Blob.ts (2)
PonyfillBlob(72-321)PonyfillBlob(323-326)packages/node-fetch/src/ReadableStream.ts (1)
PonyfillReadableStream(59-269)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: unit / node 23
- GitHub Check: unit / deno
- GitHub Check: unit / node 22
- GitHub Check: unit / bun
- GitHub Check: unit / node 20
- GitHub Check: unit / node 18
🔇 Additional comments (9)
packages/node-fetch/src/Body.ts (9)
3-4: New imports added for handling HTTP messages and stream transformation.The addition of
IncomingMessageandPassThroughimports supports the new functionality for handling HTTP request bodies and enabling proper stream cancellation.
48-48: Good addition of AbortSignal support.Adding the optional
signalproperty toPonyfillBodyOptionsis an excellent enhancement that will help prevent hanging when streams are destroyed prematurely, which directly addresses the PR objective.
60-63: Successfully passing AbortSignal to the body processor.The changes correctly pass the optional AbortSignal to the
processBodyInitfunction, enabling stream abortion capabilities throughout the body processing logic.
142-142: Type improvement for chunks collection.Changing
_chunksto useMaybePromise<Uint8Array[]>allows for more flexible handling of both synchronous and asynchronous chunk collection scenarios.
144-182: Refactored chunk collection with better error handling.This refactoring improves how chunks are collected from readable streams and now properly handles stream abortions via the signal parameter on line 178.
184-190: Improved chunk collection with early return.The new implementation of
_collectChunksFromReadablenow checks if chunks already exist before attempting to collect them, which prevents redundant operations on potentially destroyed streams and addresses the hanging issue mentioned in the PR.
383-386: Function signature updated to support AbortSignal.The update to
processBodyInitfunction signature to include the optionalsignalparameter is crucial for enabling abort capabilities during body processing.
477-495: Excellent addition for handling IncomingMessage with abort capabilities.The new case for handling
IncomingMessageinstances is well-implemented. It correctly pipes the incoming message through a PassThrough stream configured with the abort signal, enabling proper termination of stream operations when requested.This is the key implementation that addresses the PR objective of not hanging when a request is leaked and attempted to be parsed outside the server adapter.
415-416: Type annotations and simplified implementations.The addition of explicit type annotations for variables (
buffer,readableStream,blob) and simplified implementation of factory functions improve code readability and maintainability.Also applies to: 443-447, 452-459
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.changeset/heavy-worms-battle.md (1)
6-8: Refine Language for Clarity
The phrasing "prevents it to hang" is grammatically off. Consider rephrasing to "prevents it from hanging" and change "destroyed earlier" to "destroyed early" for improved clarity and readability.Apply the following diff improvement:
-When any `Request` method is called outside server adapter scope, it used to hang. -This PR prevents it to hang and throw an error if the readable stream is destroyed earlier. +When any `Request` method is called outside the server adapter scope, it used to hang. +This PR prevents it from hanging and throws an error if the readable stream is destroyed early.🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...it used to hang. This PR prevents it to hang and throw an error if the readable stre...(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
packages/node-fetch/src/Body.ts (2)
184-186: Remove unnecessary type castingNow that the signal property is explicitly declared, the cast to
(this as any).signalis unnecessary.- return _body.readable.toArray({ signal: (this as any).signal }).then(chunks => { + return _body.readable.toArray({ signal: this.signal }).then(chunks => {
150-188: Consider adding error handling for stream operationsThe chunk collection operation doesn't explicitly handle errors that might occur when the stream is prematurely destroyed. Adding error handling would make the error messages more clear and help with debugging.
_doCollectChunksFromReadableJob() { if (this.bodyType === BodyInitType.AsyncIterable) { if (Array.fromAsync) { return handleMaybePromise( () => Array.fromAsync(this.bodyInit as AsyncIterable<Uint8Array>), chunks => { this._chunks = chunks; return this._chunks; }, + error => { + if (error.name === 'AbortError') { + throw new TypeError('The operation was aborted', { cause: error }); + } + throw error; + } ); } // ...rest of the code
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.changeset/heavy-worms-battle.md(1 hunks)packages/node-fetch/src/Body.ts(10 hunks)packages/node-fetch/src/ReadableStream.ts(3 hunks)packages/node-fetch/src/Request.ts(1 hunks)packages/node-fetch/src/Response.ts(2 hunks)packages/node-fetch/src/fetchNodeHttp.ts(2 hunks)packages/node-fetch/tests/fetch.spec.ts(2 hunks)packages/server/README.md(1 hunks)packages/server/src/utils.ts(5 hunks)packages/server/test/abort.spec.ts(1 hunks)packages/server/test/compression.spec.ts(3 hunks)packages/server/test/node.spec.ts(5 hunks)packages/server/test/reproductions.spec.ts(2 hunks)packages/server/test/test-fetch.ts(2 hunks)packages/server/test/test-server.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/node-fetch/tests/fetch.spec.ts
- packages/server/test/reproductions.spec.ts
- packages/server/src/utils.ts
- packages/node-fetch/src/Response.ts
- packages/server/README.md
- packages/server/test/test-fetch.ts
- packages/node-fetch/src/fetchNodeHttp.ts
- packages/node-fetch/src/Request.ts
- packages/node-fetch/src/ReadableStream.ts
- packages/server/test/test-server.ts
🧰 Additional context used
🧬 Code Definitions (1)
packages/server/test/node.spec.ts (4)
packages/server/src/utils.ts (1)
createDeferredPromise(13-13)packages/server/src/createServerAdapter.ts (1)
createServerAdapter(535-535)packages/fetch/dist/esm-ponyfill.js (4)
Response(4-4)ReadableStream(6-6)fetch(1-1)URL(20-20)packages/server/test/test-server.ts (1)
fetch(48-50)
🪛 LanguageTool
.changeset/heavy-worms-battle.md
[uncategorized] ~7-~7: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...it used to hang. This PR prevents it to hang and throw an error if the readable stre...
(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
🪛 Biome (1.9.4)
packages/server/test/node.spec.ts
[error] 24-24: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: unit / deno
- GitHub Check: unit / node 20
- GitHub Check: unit / bun
- GitHub Check: unit / node 18
- GitHub Check: type check
- GitHub Check: prettier
- GitHub Check: server (undici)
- GitHub Check: e2e / azure-function
🔇 Additional comments (19)
.changeset/heavy-worms-battle.md (2)
1-4: Patch Metadata is Correctly Specified
The changeset correctly indicates that both@whatwg-node/node-fetchand@whatwg-node/serverare patched. No action needed here.
9-17: Effective Demonstration in Code Snippet
The provided TypeScript code snippet successfully illustrates the issue and its resolution. The snippet shows that callingrequest.text()now throws an error as expected.packages/server/test/abort.spec.ts (3)
5-5: Good utility function addition for conditional test skipping.Adding a
skipIfutility is a clean way to handle conditional test execution based on runtime environments.
8-8: Improved parameter signature enhances test flexibility.Updating the
runTestsForEachServerImplfunction to receiveserverImplNameallows for more precise control over which tests run in specific server environments.
10-13: Well-implemented conditional test skipping logic.This condition correctly prevents test execution in mismatched environments (Bun tests on non-Bun environments and Deno tests on non-Deno environments), which helps avoid the hanging issue when request methods are used outside their proper adapter scope.
packages/server/test/node.spec.ts (4)
97-97: Good utility function addition for conditional test skipping.The
skipIffunction provides a clean way to handle conditional test execution based on runtime environments.
98-150: Well-implemented test for handling cancellation of streaming responses.This test properly verifies that the application correctly handles cancellation of incremental responses without hanging, which directly addresses the PR objective. The test creates a readable stream, reads a few chunks, and then aborts the request to simulate the scenario described in the PR.
236-273: Good test enhancement for verifying Request.signal behavior inside adapters.This test ensures that Request.signal is properly handled within the adapter, which is important for ensuring proper error propagation instead of hanging when a request is aborted.
405-418: Well-implemented conditional skipping for IPv6 test.This change ensures that IPv6 tests only run in environments where they are supported, preventing potential hanging issues in incompatible environments.
packages/server/test/compression.spec.ts (2)
150-150: Improved parameter signature enhances test flexibility.Updating the
runTestsForEachServerImplfunction to receiveserverImplNameallows for more precise control over which compression tests run in specific server environments.
227-295: Enhanced error handling and conditional test execution for compression tests.The changes provide better error handling with explicit JSON parsing error messages and conditionally skip tests on incompatible server implementations. The code also improves the test structure by handling the 'none' encoding case separately and implementing proper stream handling for compression tests.
packages/node-fetch/src/Body.ts (8)
48-48: Good addition of signal property to PonyfillBodyOptionsAdding the optional AbortSignal to the options interface allows for proper cancellation of ongoing operations, which aligns with the PR's goal of preventing hanging when requests are leaked.
55-55: Nice explicit declaration of the signal propertyExplicitly declaring and initializing the signal property addresses the previous review comment about clarifying where this property comes from, making the code more maintainable.
Also applies to: 61-61
89-91: Good signal handling for readable streamsThis change ensures that the abort signal is properly connected to the readable stream, allowing operations to be cancelled when the signal is triggered.
148-148: Good refactoring for chunk collectionChanging the
_chunkstype toMaybePromise<Uint8Array[]> | nulland restructuring the collection logic improves code clarity and prevents redundant collection operations.Also applies to: 190-196
483-505: Excellent handling of IncomingMessage with abort signalThis new case properly handles
IncomingMessageinstances by piping them through aPassThroughstream with signal support. This prevents hanging when the stream is prematurely destroyed and ensures proper error propagation.
389-391: Good function signature updateUpdating the
processBodyInitfunction to accept an abort signal parameter allows for proper signal propagation throughout the body processing pipeline.
421-422: Good cleanup of variable assignmentsThese small variable assignments improve code readability by clearly identifying the types of the values being used, making the code more maintainable.
Also applies to: 428-429, 449-450, 452-453, 458-459, 464-465
3-4: Comprehensive abort signal implementationThe changes to support abort signals throughout the codebase are thorough and well-implemented. From importing the necessary utilities, to initializing the signal in the constructor, to properly connecting it to streams, these changes ensure that operations can be cancelled properly instead of hanging.
Also applies to: 62-65, 493-495
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/node-fetch/src/Body.ts (1)
184-186: Directly reference the signal property instead of using type assertion.The code uses
(this as any).signalwhen we have a properly declaredsignalproperty that could be accessed directly.- return _body.readable.toArray({ signal: (this as any).signal }).then(chunks => { + return _body.readable.toArray({ signal: this.signal }).then(chunks => { this._chunks = chunks; return this._chunks;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/node-fetch/src/Body.ts(10 hunks)packages/server/test/node.spec.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/server/test/node.spec.ts
🧰 Additional context used
🧬 Code Definitions (1)
packages/node-fetch/src/Body.ts (3)
packages/promise-helpers/src/index.ts (2)
MaybePromise(1-1)handleMaybePromise(29-42)packages/node-fetch/src/Blob.ts (2)
PonyfillBlob(72-321)PonyfillBlob(323-326)packages/node-fetch/src/ReadableStream.ts (1)
PonyfillReadableStream(59-269)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: unit / node 23
- GitHub Check: unit / deno
- GitHub Check: unit / node 22
- GitHub Check: unit / node 20
- GitHub Check: unit / bun
- GitHub Check: unit / node 18
- GitHub Check: type check
- GitHub Check: prettier
🔇 Additional comments (11)
packages/node-fetch/src/Body.ts (11)
3-4: New imports support HTTP message handling and abort signals.These imports enable the new functionality for handling IncomingMessage instances and supporting abort signals to cancel operations.
48-48: Good addition of AbortSignal to options interface.Adding the optional
signalproperty to thePonyfillBodyOptionsinterface allows consumers to control the lifecycle of body processing operations, which is crucial for preventing hanging when streams are destroyed.
55-55: AbortSignal property added to track cancellation.The addition of this property enables the class to properly handle cancellation of ongoing operations.
61-65: Proper initialization and propagation of AbortSignal.The constructor now correctly initializes the signal property from options and passes it to the body processing function.
89-91: Stream and signal integration enhances cancellation support.This change properly connects the AbortSignal to the Readable stream, ensuring that if the signal is aborted, the stream operations will also be canceled, preventing potential hangs.
148-148: Type update enables more flexible chunk handling.Changing the
_chunkstype to includeMaybePromiseallows for both synchronous and asynchronous chunk collection, making the code more flexible.
150-188: Extract chunk collection logic for better organization.The new method encapsulates the logic for collecting chunks from various body sources, making the code more maintainable.
190-196: Improved check for existing chunks avoids duplicate work.The refactored method now properly checks if chunks are already collected before initiating a new collection job, which prevents unnecessary work and potential race conditions.
389-392: Function signature update allows for abort signal propagation.The updated signature enables the propagation of the abort signal to lower-level stream operations.
483-501: Critical fix for IncomingMessage handling prevents hanging.This is a key addition that directly addresses the PR's main goal. By properly piping the IncomingMessage through a PassThrough stream with an abort signal, the code can now handle cases where the stream is destroyed prematurely without hanging.
421-422: Type refinements improve code clarity and safety.The explicit type annotations and simplified expressions make the code more readable and type-safe, reducing the risk of runtime errors.
Also applies to: 428-429, 450-450, 453-453, 459-459, 465-465
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/node-fetch/src/Body.ts (1)
147-185: Improved chunk collection logic.The refactored chunk collection logic is more robust, with proper separation of concerns between checking for existing chunks and performing the actual collection job. The code now more efficiently handles AsyncIterable body types and simplifies collection from ReadableStream bodies.
However, there's potential for a minor optimization in error handling.
Consider adding error handling to handle stream errors, particularly for the case where the stream might be aborted:
return _body.readable.toArray().then(chunks => { this._chunks = chunks; return this._chunks; +}).catch(error => { + if (error.name === 'AbortError' || (this.signal && this.signal.aborted)) { + throw new Error('The operation was aborted', { cause: error }); + } + throw error; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/node-fetch/src/Body.ts(10 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/node-fetch/src/Body.ts (3)
packages/promise-helpers/src/index.ts (3)
MaybePromise(1-1)handleMaybePromise(29-42)fakePromise(47-94)packages/node-fetch/src/Blob.ts (3)
buffer(90-149)PonyfillBlob(72-321)PonyfillBlob(323-326)packages/node-fetch/src/ReadableStream.ts (1)
PonyfillReadableStream(59-269)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: unit / deno
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: unit / bun
- GitHub Check: unit / node 20
- GitHub Check: unit / node 18
- GitHub Check: e2e / azure-function
🔇 Additional comments (11)
packages/node-fetch/src/Body.ts (11)
3-4: Good addition of required imports for the new functionality.The new imports for
IncomingMessageandPassThroughsupport the handling of HTTP message bodies and stream processing, which directly addresses the issue described in the PR objectives.
48-48: Great enhancement to support AbortSignal.Adding the
signaloption toPonyfillBodyOptionsallows proper cancellation of operations, which is essential for preventing the hanging behavior mentioned in the PR objectives.
55-55: Well-typed property for the abort signal.The
signalproperty is properly typed with appropriate nullable and optional modifiers, maintaining type safety.
61-65: Properly initializing and passing the abort signal.The signal is correctly initialized from options and passed to the
processBodyInitfunction, ensuring that abort signals are propagated throughout the body processing pipeline.
145-145: Good type enhancement for _chunks property.Using
MaybePromise<Uint8Array[]>instead of justUint8Array[]makes the property capable of handling both synchronous arrays and promises for arrays, which improves flexibility when dealing with asynchronous operations.
187-193: Effective caching mechanism for collected chunks.The new implementation intelligently checks if chunks are already collected before initiating a new collection job, which prevents redundant work and potential race conditions.
386-389: Good function signature update to support AbortSignal.The
processBodyInitfunction now accepts an optional AbortSignal parameter, allowing for proper propagation of cancellation signals throughout the body processing pipeline.
418-428: Improved Buffer handling with clearer type annotations.The explicit type annotation for the buffer improves code readability and type safety, and the use of
Readable.from(buffer)ensures consistent stream creation.
446-450: Better type handling for ReadableStream instances.The explicit type annotation improves code clarity and the direct return of the readableStream instance in the bodyFactory simplifies the code.
455-462: Improved Blob handling with clearer type annotations.The explicit type annotation and direct call to blob.stream() simplifies the code and improves readability.
480-498: Critical fix for the hanging issue when request is leaked.This is the core change that addresses the PR objective. By adding specialized handling for
IncomingMessageinstances and piping them through aPassThroughstream that respects the abort signal, the code now properly handles the case where a request might be leaked and parsed outside the server adapter scope.Instead of hanging indefinitely, the operation will now throw an error if the stream is destroyed prematurely, which is a much better user experience and easier to debug.
2 participants
When any
Requestmethod is called outside server adapter scope, it used to hang.This PR prevents it to hang and throw an error if the readable stream is destroyed earlier.
Closes #1229