enhance(server): use native AbortController and AbortSignal #2031
Conversation
WalkthroughThe changes update dependency versions and streamline request cancellation across the project. Customized abort signal implementations have been removed in favor of the native Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ServerAdapter
participant AbortController
participant RequestHandler
Client->>ServerAdapter: Send Request
ServerAdapter->>AbortController: Create/assign controller
AbortController-->>RequestHandler: Provide signal for processing
RequestHandler->>Stream: Process request with signal
Stream-->>AbortController: On error/close, trigger abort
AbortController-->>ServerAdapter: Notify abort event
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.8-alpha-20250205113213-1ddf37bf9cb93e796a62ac800ea0b4100bb2eaa4 |
npm ↗︎ unpkg ↗︎ |
@whatwg-node/server |
0.10.0-alpha-20250205113213-1ddf37bf9cb93e796a62ac800ea0b4100bb2eaa4 |
npm ↗︎ unpkg ↗︎ |
@whatwg-node/server-plugin-cookies |
2.0.0-alpha-20250205113213-1ddf37bf9cb93e796a62ac800ea0b4100bb2eaa4 |
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 (5)
packages/node-fetch/src/WritableStream.ts (1)
54-55: Distinguish between normal stream closure and error-based abort.Calling
abortCtrl.abort()on a normalcloseevent could be unexpected, asabortoften implies an abnormal termination. Consider differentiating the two scenarios if standard compliance or consumer expectations require that a normal close not raise an abort signal.packages/server/src/utils.ts (4)
111-116: Confirm Node request abort property usage.Using
Object.defineProperty(rawRequest, 'aborted', { value: true })effectively enforces an aborted state, but note that Node commonly managesreq.aborteditself. Double-check that this manual override won't conflict with built-in Node request behavior or external libraries that rely on the existing property.
149-149: Repeating identical request construction logic.Multiple request objects with the same
signal: controller.signalusage appear throughout the function. Consider abstracting the shared logic into a helper to reduce duplication.
155-155: Same cancellation signal pattern.This usage is identical to other request blocks. Reiterating that a helper function might improve maintainability.
211-215: Handling rawRequest as body with a TS exception.Passing the raw incoming request stream and adding the
@ts-expect-errorcomment is workable, but consider revisiting if TypeScript adds proper types for async streams. Meanwhile, ensure the stream isn’t already consumed or curtailed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.changeset/metal-moles-care.md(1 hunks)packages/node-fetch/src/Request.ts(2 hunks)packages/node-fetch/src/WritableStream.ts(1 hunks)packages/node-fetch/src/fetchCurl.ts(3 hunks)packages/node-fetch/src/fetchNodeHttp.ts(2 hunks)packages/server/src/createServerAdapter.ts(5 hunks)packages/server/src/utils.ts(6 hunks)packages/server/src/uwebsockets.ts(7 hunks)
🔇 Additional comments (20)
packages/node-fetch/src/fetchNodeHttp.ts (3)
50-50: LGTM! Clean signal property access.The change to use direct property access for the signal is cleaner and aligns well with the move to native AbortController/AbortSignal.
57-57: LGTM! Consistent signal handling.The signal property access is consistently updated in both request creation paths.
109-111: LGTM! Proper signal propagation to stream pipeline.The signal is correctly propagated to the stream pipeline, ensuring proper cancellation handling throughout the request lifecycle.
Let's verify that the abort signal is properly handled in the pipeline:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other pipeline usages to ensure consistent signal handling rg "pipeline.*signal" -A 5Length of output: 28
🏁 Script executed:
#!/bin/bash echo "Displaying snippet from packages/node-fetch/src/fetchNodeHttp.ts (lines 100-120):" sed -n '100,120p' packages/node-fetch/src/fetchNodeHttp.ts echo -e "\nSearching for 'fetchRequest.signal' usage:" rg "fetchRequest\.signal" -n packages/node-fetch/src/fetchNodeHttp.tsLength of output: 1134
LGTM – Signal propagation is consistently implemented.
The verification shows that in
packages/node-fetch/src/fetchNodeHttp.ts, thefetchRequest.signalis correctly used in the pipeline options (line 110) and similarly applied at other relevant locations, ensuring proper cancellation handling.packages/node-fetch/src/WritableStream.ts (1)
47-51: Adoption of native AbortController looks correct.The new approach cleanly injects the
AbortSignalinto the controller and properly destroys the writable stream on error. One minor consideration: if multiple errors occur,abortCtrl.signal.reasonmay be overwritten. Confirm that this behavior is acceptable or consider preserving the first error encountered.packages/server/src/utils.ts (6)
89-89: Ensure all references to removed parameters are updated.The function signature for
normalizeNodeRequesthas changed. Verify that no callers are still passing the removedregisterSignalparameter or relying on the old behavior.
132-132: Consistent usage of controller.signal.Providing the signal for the
fetchAPI.Requestensures correct cancellation logic, aligning with the new architecture. Looks good.
203-203: Unified abort handling is consistent.Passing
controller.signalagain ensures the request can be aborted properly when using a Bun Node-compat or other streaming approach. No issues detected.
535-539: Immediate abort check is valid.Throwing
abortSignal.reasonif the signal is already aborted preempts unnecessary processing. This is a good practice to fail early when the request cannot proceed.
543-546: Registering the abort listener with a once-option.The code rejects the deferred promise immediately on abort. Coupled with a one-time listener, this approach is clean and prevents multiple rejections.
552-555: Cleaning up the event listener in finally.Removing the abort listener ensures no leaking references or stale callbacks. Overall, the promise resolution path is well-handled.
packages/node-fetch/src/Request.ts (2)
76-76: Straightforward fallback to a new AbortController.Using a user-provided signal when available, else creating a fresh one, aligns with typical fetch patterns. Good job simplifying the lazy initialization.
140-140: Public signal property is clearer.Exposing
AbortSignalplainly removes the complexity of a private_signalwith lazy creation. This should improve maintainability and predictability.packages/node-fetch/src/fetchCurl.ts (2)
33-33: LGTM! Improved signal access.The change from private signal access to public signal property enhances code readability and aligns with native AbortController usage.
91-92: LGTM! Consistent signal handling.The changes consistently update signal access across event listeners and pipeline configuration, improving code maintainability.
Also applies to: 100-101, 126-126
packages/server/src/uwebsockets.ts (3)
34-34: LGTM! Updated interface to use AbortController.The change from
signal: AbortSignaltocontroller: AbortControlleraligns with the PR objective.
78-84: LGTM! Improved event listener setup.The event listener is now properly configured with
{ once: true }option, preventing potential memory leaks.
252-260: LGTM! Enhanced abort handling.The abort event listener now properly cancels the body stream when aborted, improving resource cleanup.
packages/server/src/createServerAdapter.ts (2)
321-321: LGTM! Migrated to native AbortController.The changes properly replace the custom abort signal implementation with native AbortController, improving standardization.
Also applies to: 330-330, 333-333
408-413: LGTM! Improved signal handling in fetchFn.The signal handling is now more explicit and properly integrated with the native AbortController.
.changeset/metal-moles-care.md (1)
1-7: LGTM! Clear and concise changeset.The changeset properly documents both the dependency updates and the migration to native AbortController/AbortSignal.
ardatan
force-pushed
the
use-native-abort-ctrl
branch
from
c9a7e9a to
98b4d1e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/server/src/utils.ts (1)
111-126: Consider cleaning up the AbortController.The
AbortControllerinstance should be cleaned up when it's no longer needed to prevent potential memory leaks. Consider removing the event listeners and aborting the controller when the response is finished.serverResponse.once('finish', () => { nodeResponse.removeListener('close', closeEventListener); + nodeResponse.removeListener('error', closeEventListener); + if (!controller.signal.aborted) { + controller.abort(); + } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.changeset/metal-moles-care.md(1 hunks)packages/node-fetch/src/Request.ts(2 hunks)packages/node-fetch/src/WritableStream.ts(1 hunks)packages/node-fetch/src/fetchCurl.ts(3 hunks)packages/node-fetch/src/fetchNodeHttp.ts(2 hunks)packages/server/src/createServerAdapter.ts(5 hunks)packages/server/src/utils.ts(6 hunks)packages/server/src/uwebsockets.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .changeset/metal-moles-care.md
- packages/node-fetch/src/fetchNodeHttp.ts
- packages/node-fetch/src/fetchCurl.ts
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: unit / node 20
- GitHub Check: unit / deno
- GitHub Check: unit / node 18
- GitHub Check: server (undici)
- GitHub Check: unit / bun
- GitHub Check: type check
- GitHub Check: alpha / snapshot
- GitHub Check: e2e / cloudflare-modules
- GitHub Check: server (ponyfill)
- GitHub Check: esm
- GitHub Check: server (native)
- GitHub Check: e2e / cloudflare-workers
- GitHub Check: e2e / azure-function
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: lint
- GitHub Check: e2e / aws-lambda
- GitHub Check: node-fetch (noConsumeBody)
- GitHub Check: prettier
🔇 Additional comments (9)
packages/node-fetch/src/WritableStream.ts (1)
47-55: LGTM! Clean implementation using native AbortController.The changes effectively replace the custom signal implementation with the native AbortController, making the code more maintainable and aligned with web standards. The implementation correctly:
- Creates an AbortController
- Passes its signal to the controller
- Sets up error and close event handlers to trigger abort
packages/node-fetch/src/Request.ts (1)
76-76: LGTM! Simplified signal initialization.The changes effectively:
- Remove the lazy initialization pattern
- Make the signal property public and directly initialized
- Use native AbortController's signal by default
This aligns with the PR objectives stating that signals are no longer considered expensive enough to require lazy initialization.
Also applies to: 140-140
packages/server/src/uwebsockets.ts (3)
34-34: LGTM! Clean interface update.The interface and function signature changes correctly reflect the transition to using AbortController, making the API more consistent with web standards.
Also applies to: 37-42
78-84: LGTM! Improved event listener setup.The event listener setup is enhanced with:
- Direct use of AbortController's signal
- The
once: trueoption to prevent memory leaks
252-260: LGTM! Robust abort handling.The abort handling is improved with:
- Proper cancellation of the response body
- Single-use event listener to prevent memory leaks
- Proper propagation of the abort reason
packages/server/src/createServerAdapter.ts (2)
321-321: LGTM! Clean abort handling in handleUWS.The changes effectively:
- Create a new AbortController for each request
- Set up proper abort handling with single-use event listeners
- Maintain proper cleanup through the
once: trueoptionAlso applies to: 330-334
408-413: LGTM! Simplified signal handling in fetchFn.The signal handling is streamlined by:
- Directly accessing the signal from RequestInit
- Using the native AbortController signal
- Properly propagating the abort signal to the response promise
packages/server/src/utils.ts (2)
89-89: LGTM! Function signature simplified.The removal of the
registerSignalparameter simplifies the function signature while maintaining functionality through nativeAbortController.
533-559: LGTM! Improved abort signal handling.The changes enhance the abort signal handling by:
- Using native
AbortSignaltype- Adding proper event listener cleanup
- Using the standard
reasonproperty for error handling
ardatan
force-pushed
the
use-native-abort-ctrl
branch
from
98b4d1e to
1ddf37b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/server/src/createServerAdapter.ts (1)
351-363: Consider cleanup of abort event listenerWhile the implementation is correct, consider cleaning up the abort event listener when the response ends to prevent potential memory leaks.
res.end = function (data: any) { resEnded = true; + controller.signal.removeEventListener('abort', cb); return originalResEnd(data); };packages/server/src/utils.ts (1)
533-559: Consider enhancing error handling in abort signal managementWhile the implementation is correct, consider adding a type check for the abort reason to handle cases where it might be undefined.
if (abortSignal?.aborted) { - throw abortSignal.reason; + throw abortSignal.reason instanceof Error ? abortSignal.reason : new Error('Request aborted'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.changeset/metal-moles-care.md(1 hunks)packages/node-fetch/src/Request.ts(2 hunks)packages/node-fetch/src/WritableStream.ts(1 hunks)packages/node-fetch/src/fetchCurl.ts(3 hunks)packages/node-fetch/src/fetchNodeHttp.ts(2 hunks)packages/server/src/createServerAdapter.ts(5 hunks)packages/server/src/utils.ts(6 hunks)packages/server/src/uwebsockets.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- .changeset/metal-moles-care.md
- packages/node-fetch/src/fetchCurl.ts
- packages/node-fetch/src/fetchNodeHttp.ts
- packages/node-fetch/src/Request.ts
- packages/server/src/uwebsockets.ts
⏰ Context from checks skipped due to timeout of 90000ms (14)
- 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: e2e / cloudflare-modules
- GitHub Check: server (undici)
- GitHub Check: unit / node 18
- GitHub Check: server (ponyfill)
- GitHub Check: e2e / azure-function
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: server (native)
- GitHub Check: type check
- GitHub Check: node-fetch (noConsumeBody)
🔇 Additional comments (3)
packages/node-fetch/src/WritableStream.ts (1)
47-55: Well-implemented transition to native AbortController!The changes effectively replace custom signal handling with the native AbortController, providing a more standardized and maintainable implementation. The error and close event handling is properly integrated with the abort mechanism.
packages/server/src/createServerAdapter.ts (1)
321-334: Good implementation of native AbortController!The transition to native AbortController is well implemented with proper event handling setup.
packages/server/src/utils.ts (1)
111-126: Well-implemented AbortController in request normalization!The integration of native AbortController in the request normalization process is clean and properly handles all necessary events.
Apparently in the benchmarks, no need to use a custom implementation for AbortSignal in the server-side
Request.signal.Also we don't need to introduce signals lazily as they are not expensive anymore.
Summary by CodeRabbit
Chores
Refactor