feat: upgrade to undici v7 #547
Conversation
fix server side close unexpected exit closes #541
WalkthroughThis pull request introduces several new scripts and updates existing files to enhance HTTP/2 support and error handling in the codebase. New examples demonstrate the use of the Changes
Assessment against linked issues
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
commit: |
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/undici@6.21.0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (9)
examples/h2-other-side-closed-exit-0-fetch.cjs (2)
10-10: Consider using a test endpoint instead of production APIMaking repeated requests to a production API (
edgeupdates.microsoft.com) could potentially cause rate limiting or unnecessary load. Consider using a local test server or a dedicated test endpoint.
24-30: Consider enhancing error reportingThe error handling could be more informative:
- The commented-out error stack trace would be useful for debugging
- Consider logging more details about the failure scenario
}).catch(err => { console.error('main error throw: %s', err); - // console.error(err); + console.error('Error details:', { + code: err.code, + message: err.message, + stack: err.stack, + }); process.exit(1); });src/FetchOpaqueInterceptor.ts (1)
37-38: Consider type safety for opaque assignmentThe type assertion
(handler as any)bypasses TypeScript's type checking. Consider defining a proper interface for the handler that includes the opaque property.- (handler as any).opaque = opaque; + interface OpaqueHandler extends Dispatcher.DispatchHandler { + opaque?: FetchOpaque; + } + (handler as OpaqueHandler).opaque = opaque;examples/longruning.cjs (1)
24-30: Ensure graceful cleanup on error exitThe error handling looks good, but we should ensure proper cleanup of HTTP/2 sessions before exit.
}).catch(err => { console.error('main error throw: %s', err); console.error(err); + // Ensure proper cleanup of HTTP/2 sessions + await httpClient.close(); process.exit(1); });src/HttpAgent.ts (3)
Line range hint
73-89: LGTM! Consider enhancing error messages.The dispatch method's type signature update aligns with undici v7's API changes. The IP address validation logic is thorough, but the error message could be more descriptive.
Consider enhancing the error message to include more context:
- throw new IllegalAddressError(hostname, hostname, family); + throw new IllegalAddressError( + hostname, + hostname, + family, + `IP address ${hostname} (IPv${family}) is not allowed` + );
Line range hint
40-52: Consider preserving error stack traces in callbacks.The error handling in the lookup callback could potentially lose the original stack trace. Consider using
Promise.reject()or wrapping the callback in a try-catch block.Here's a suggested improvement:
lookup(hostname, dnsOptions, (err, ...args: any[]) => { // address will be array on Node.js >= 20 const address = args[0]; const family = args[1]; - if (err) return (callback as any)(err, address, family); + if (err) { + // Preserve the stack trace + const wrappedError = new Error('DNS lookup failed'); + wrappedError.cause = err; + return (callback as any)(wrappedError, address, family); + }
Line range hint
31-67: Well-structured implementation with good separation of concerns.The
HttpAgentclass effectively extends undici's Agent with custom address validation while maintaining clean separation of concerns. The implementation:
- Properly handles different Node.js versions
- Provides flexible configuration options
- Implements comprehensive error handling
Consider adding the following improvements:
- Add input validation for configuration options
- Consider implementing a retry mechanism for transient DNS failures
- Add telemetry/logging for address validation failures to help diagnose issues in production
src/fetch.ts (1)
Line range hint
1-262: Implementation looks solid with room for enhancementThe fetch implementation maintains good separation of concerns with comprehensive diagnostics. The upgrade to undici v7 and changes to state handling appear well-integrated.
Some suggestions for future improvements:
- Consider adding memory usage metrics to the diagnostics channel
- Add documentation about HTTP/2 behavior and memory characteristics
- Consider implementing circuit breakers or pooling limits for H2 connections
src/HttpClient.ts (1)
73-75: Consider marking 'UnidiciTimingInfo' as deprecatedTo inform developers of the updated interface name and encourage migration, consider adding a deprecation notice to
'UnidiciTimingInfo'.Apply this diff to add the deprecation notice:
+/** @deprecated Use UndiciTimingInfo instead */ export interface UnidiciTimingInfo extends UndiciTimingInfo {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
examples/h2-other-side-closed-exit-0-fetch.cjs(1 hunks)examples/h2-other-side-closed-exit-0.cjs(1 hunks)examples/longruning.cjs(1 hunks)package.json(1 hunks)src/FetchOpaqueInterceptor.ts(1 hunks)src/HttpAgent.ts(1 hunks)src/HttpClient.ts(4 hunks)src/fetch.ts(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
examples/h2-other-side-closed-exit-0-fetch.cjs
[error] 16-16: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
examples/h2-other-side-closed-exit-0.cjs
[error] 16-16: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
🪛 eslint
examples/h2-other-side-closed-exit-0-fetch.cjs
[error] 1-1: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
examples/h2-other-side-closed-exit-0.cjs
[error] 1-1: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
examples/longruning.cjs
[error] 1-1: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
🔇 Additional comments (13)
examples/h2-other-side-closed-exit-0-fetch.cjs (2)
32-34: LGTM! Good debugging practice
The beforeExit handler is well-placed to help debug unexpected server exits, which aligns with the PR's objective of fixing server-side exit issues.
3-5: Verify memory usage with allowH2 configuration
Given that this example demonstrates the fix for issue #541 (OOM with allowH2), we should verify that the memory usage remains stable over multiple iterations.
✅ Verification successful
The allowH2 configuration is properly tested and monitored
The codebase shows comprehensive implementation and testing of the allowH2 feature:
- Memory usage monitoring is implemented in
examples/longruning.cjswhich tracks process memory usage withallowH2: true - Multiple test files verify the
allowH2functionality in different scenarios - The feature is well-documented in README.md and consistently implemented across HttpClient, HttpAgent, and fetch modules
- The example file under review follows the established pattern for HTTP/2 support
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related test files that verify memory usage with allowH2
fd -e test.js -e spec.js -e test.ts -e spec.ts | xargs rg -l "allowH2.*true.*memory"
# Look for other examples using allowH2
fd -e js -e ts | xargs rg -l "allowH2.*true"
Length of output: 189
Script:
#!/bin/bash
# Check the test files for memory-related tests with allowH2
rg -A 10 "allowH2.*true" test/options.dispatcher.test.ts test/HttpClient.test.ts
# Look for memory leak or OOM-related tests
rg -l "memory|leak|OOM" test/
Length of output: 1697
Script:
#!/bin/bash
# Let's check the PR description, issue, or commit messages for OOM context
rg -l "OOM|out of memory" .github/
# Check for any memory-related test utilities or helpers
fd test-utils test-helper | xargs rg -l "memory"
# Look for memory-related configurations or settings in the example files
fd -e cjs -e js -e ts examples/ | xargs rg "memory|heap|allowH2"
Length of output: 1873
examples/h2-other-side-closed-exit-0.cjs (1)
1-34: Verify similar patterns across the codebase
Let's check for similar patterns that might need the same improvements.
✅ Verification successful
Let me try a different search strategy to gather more information about the usage patterns.
Based on the search results, I can now provide a final response:
HTTP/2 and Agent usage patterns need consistent error handling
The codebase shows several instances of Agent usage with allowH2: true:
src/HttpClient.ts: Main implementation with proper configurationtest/options.dispatcher.test.ts: Test case- Two example files demonstrating error handling patterns
The example files appear to be intentional demonstrations of error handling patterns, rather than production code requiring fixes. The main implementation in HttpClient.ts already has proper configuration and error handling setup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find similar patterns that might need the same improvements
# Find files using undici with allowH2
echo "Files using undici with allowH2:"
rg -l "allowH2.*true" --type js
# Find potential unbounded request loops
echo -e "\nPotential unbounded request loops:"
ast-grep --pattern 'for ($cond) {
$$$
request($$$)
$$$
}'
# Find files missing cleanup
echo -e "\nFiles using Agent without cleanup:"
rg -l "new Agent\(" --type js | xargs rg -L "\.close\(\)"
Length of output: 454
Script:
#!/bin/bash
# Search for undici imports and their usage patterns
echo "Files importing undici:"
rg -l "require\('undici'\)" --type js
echo -e "\nFiles importing undici with destructuring:"
rg "(?:import|require).*\{.*Agent.*\}.*(?:from\s+)?['\"](undici|node:undici)['\"]" -l
echo -e "\nAgent usage patterns:"
rg "new Agent\(" -A 5
echo -e "\nRequest patterns:"
rg "request\(" -A 3 --type js
Length of output: 2123
🧰 Tools
🪛 Biome (1.9.4)
[error] 16-16: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
🪛 eslint
[error] 1-1: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
src/FetchOpaqueInterceptor.ts (2)
Line range hint 1-2: LGTM: ES module import syntax
The change from CommonJS require to ES module import aligns with modern JavaScript practices and the module system used in the codebase.
Line range hint 35-39: Verify the impact of handler type change on HTTP/2 connections
The change from DispatchHandlers to DispatchHandler aligns with undici v7's API, but we should verify this doesn't affect HTTP/2 connection handling since the PR aims to fix OOM issues with allowH2.
Let's verify the handler type usage across the codebase:
Consider implementing connection pooling limits or cleanup mechanisms if they're not already in place to prevent potential memory leaks with HTTP/2 connections.
✅ Verification successful
Let's gather more specific information about the HTTP/2 handling and dispatcher usage:
HTTP/2 handler type change is safe and properly supported
The verification shows that the handler type change is safe because:
- The
Dispatcher.DispatchHandlertype is consistently used across the codebase, particularly inHttpAgent.dispatch()which handles both HTTP/1.1 and HTTP/2 connections - The HTTP/2 support is properly configured through the
allowH2option inHttpClient,HttpAgent, and tested in bothHttpClient.test.tsandoptions.dispatcher.test.ts - The handler type change in
FetchOpaqueInterceptoraligns with the type used in the core dispatching logic
The OOM concerns are handled at the Agent level through the dispatcher configuration, not through the handler type.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of Dispatcher types and HTTP/2 related code
# Look for other HTTP/2 related configurations and handler usages
# Check for HTTP/2 related code
rg -l "allowH2|http2|HTTP/2" --type ts
# Check for Dispatcher handler type usage
ast-grep --pattern 'handler: Dispatcher.$_'
# Look for similar interceptor patterns
ast-grep --pattern 'function $_Interceptor(opts: $_) {
$$$
return function $_(opts: Dispatcher.$_, handler: Dispatcher.$_) {
$$$
}
}'
Length of output: 336
Script:
#!/bin/bash
# Check HTTP/2 configuration in HttpClient and HttpAgent
rg -A 5 "allowH2" src/HttpClient.ts src/HttpAgent.ts
# Check the fetch implementation for HTTP/2 related code
rg -A 5 "allowH2|http2|HTTP/2" src/fetch.ts
# Look for dispatcher handler type declarations and usage
rg -A 3 "Dispatcher\.(Dispatch)?(Handler|Options)" --type ts
# Check tests for HTTP/2 related scenarios
rg -A 5 "allowH2|http2|HTTP/2" test/HttpClient.test.ts test/options.dispatcher.test.ts
Length of output: 5049
examples/longruning.cjs (1)
3-5: Verify memory stability with HTTP/2 enabled
Since this test specifically targets the OOM issue with allowH2: true (issue #541), we should ensure comprehensive memory monitoring.
Consider adding the following memory monitoring enhancements:
- Set a memory threshold alert
- Track memory growth patterns over time
- Monitor both heap and non-heap memory usage
const httpClient = new HttpClient({
allowH2: true,
+ // Add additional configuration for monitoring
+ maxResponseSize: 50 * 1024 * 1024, // Prevent excessive memory usage per response
});src/HttpAgent.ts (1)
Line range hint 64-64: Verify the impact of allowH2 option on memory usage.
Given the PR objectives mention OOM issues with allowH2, we should verify that this implementation properly handles the memory concerns.
Let's check for any related memory handling or configuration in the codebase:
✅ Verification successful
The previous results show that allowH2 is used in multiple places, but we need more context about:
- The OOM issues mentioned in the PR description
- The example files that demonstrate HTTP/2 usage
- The test implementation to understand how it's being validated
This will help us verify if the implementation properly addresses the memory concerns.
Based on the evidence from the shell script results, I can now generate the final response:
The allowH2 implementation appears to be properly handling HTTP/2 connections with memory considerations
The implementation shows proper handling through:
- The example
longruning.cjsactively monitors memory usage withprocess.memoryUsage() - Tests verify both HTTP/1.1 and HTTP/2 scenarios with connection pool management
- The implementation includes proper connection pooling as shown by
getDispatcherPoolStats()which helps prevent memory leaks - Error handling and connection cleanup are implemented in the example files
The code demonstrates proper implementation of HTTP/2 support with built-in memory monitoring and connection management. The test suite verifies both HTTP/1.1 fallback and HTTP/2 scenarios, while the example files show proper error handling and memory usage tracking. The dispatcher pool stats tracking ensures connections are properly managed, which is crucial for preventing memory leaks in HTTP/2 scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for allowH2-related configurations and memory handling
echo "Searching for allowH2 usage and configuration..."
rg -A 5 "allowH2"
echo "Searching for memory-related configurations..."
rg -A 5 "(maxMemory|memory_limit|oom|outOfMemory)"
echo "Checking for related test files..."
fd -g "*test*" -E "node_modules" --exec grep -l "allowH2"
Length of output: 5514
Script:
#!/bin/bash
# Let's check the PR description and commits for OOM context
echo "Checking PR description and commits for OOM context..."
gh pr view --json title,body
# Let's also check the example files that use allowH2
echo -e "\nChecking example files content..."
cat examples/h2-other-side-closed-exit-0.cjs
cat examples/longruning.cjs
# Let's check the test implementation for allowH2
echo -e "\nChecking test implementation..."
cat test/HttpClient.test.ts
Length of output: 15934
package.json (3)
51-51: Verify undici v7.0.0 changelog for OOM fixes
The upgrade to undici v7.0.0 is a major version bump that should address the HTTP/2 OOM issue (#541). Let's verify the changelog and related fixes.
Consider implementing memory monitoring and HTTP/2 connection pooling tests to validate the fix:
- Add memory usage monitoring in your test suite
- Include stress tests for HTTP/2 connections
- Set up CI memory limits to catch potential regressions
✅ Verification successful
Based on the GitHub API results, I can now generate the final response since I have enough information about the undici v7.0.0 release and its changes.
Upgrade to undici v7.0.0 addresses HTTP/2 memory issues
The upgrade to undici v7.0.0 includes several memory-related improvements:
- Fixed memory leaks in WebSocket implementation
- Reduced memory usage in various components
- Fixed increased memory in finalization (issue first appearing in v6.16.0)
- Added memory leak tests to prevent regressions
Additionally, the release includes significant HTTP/2 improvements:
- Onboarded HTTP/2 into Undici queueing system
- Fixed HTTP/2 queueing issues
- Fixed GOAWAY handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check undici's changelog and related commits for HTTP/2 OOM fixes
# and breaking changes between v6.19.2 and v7.0.0
# Search for relevant GitHub releases and issues
gh api graphql -f query='
{
repository(owner: "nodejs", name: "undici") {
releases(first: 10, orderBy: {field: CREATED_AT, direction: DESC}) {
nodes {
tagName
description
}
}
issues(first: 10, labels: ["http2"], states: CLOSED) {
nodes {
title
body
closedAt
}
}
}
}'
# Check if the package-lock.json exists and contains the old version
if [ -f package-lock.json ]; then
echo "Checking package-lock.json for undici version changes..."
rg "undici.*6\.19\.2" package-lock.json
fi
Length of output: 46284
Line range hint 93-112: Verify dual package hazards
The exports field configuration supports both ESM and CommonJS. This setup needs careful verification to avoid dual package hazards.
Consider:
- Adding integration tests that specifically verify both ESM and CommonJS usage
- Documenting any differences in behavior between ESM and CommonJS
✅ Verification successful
Let me analyze the test files to verify the dual package hazard concerns more thoroughly.
This will help us verify:
- How the package handles both ESM and CJS imports
- Whether there are any potential hazards in the implementation
- If the entry points are properly structured for dual-format support
Dual package hazard verification completed - Implementation is safe
The implementation properly handles dual package hazards through:
- Consistent exports in both ESM and CJS formats, verified by test files
- Proper namespace handling with both default and named exports
- Tests explicitly verifying identical behavior across both formats
- Main entry point correctly structured with explicit exports
The test files demonstrate that:
- ESM: Both namespace (
import * as) and named imports work correctly - CJS: Both
require()and destructured requires work as expected - Assertions in tests verify consistent behavior across formats
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential dual package hazards
# Look for conditional exports usage
echo "Checking for conditional imports/requires..."
rg -t ts -t js "require\(['\"]urllib['\"]|import.*from.*urllib"
# Verify package entry points
echo "Verifying package entry points..."
fd -t f "index\.(js|ts|cjs|mjs)$"
Length of output: 1623
Script:
#!/bin/bash
# Check the content of test files to verify proper ESM/CJS handling
echo "=== ESM Test File ==="
cat test/esm/index.js
echo -e "\n=== CJS Test File ==="
cat test/cjs/index.js
echo -e "\n=== Main Entry Point ==="
cat src/index.ts
Length of output: 3866
51-51:
Review breaking changes impact
The upgrade to undici v7.0.0 along with the Node.js engine requirement of >= 18.19.0 might introduce breaking changes.
Please ensure:
- All HTTP/2 related configurations are updated for undici v7
- Test coverage includes HTTP/2 scenarios
- Documentation is updated to reflect any API changes
Also applies to: 89-89
✅ Verification successful
Let me analyze the results and generate one more verification script to check the specific usage patterns that might be affected by breaking changes in undici v7.0.0.
The results will help us verify if any of the breaking changes in undici v7.0.0 directly impact the codebase's usage patterns.
HTTP/2 support is properly configured and tested
Based on the verification results:
- The codebase already has proper HTTP/2 support with
allowH2configuration properly implemented inHttpClient,HttpAgent, and fetch APIs - Test coverage includes HTTP/2 scenarios with specific tests for
allowH2configurations - The usage patterns of undici (Dispatcher, Agent, Request, fetch) are straightforward and don't use deprecated features
The upgrade to undici v7.0.0 appears safe as:
- HTTP/2 support is properly handled through the
allowH2option - The codebase uses standard APIs (fetch, Request, Agent) that are stable across versions
- Test coverage explicitly verifies HTTP/2 functionality
- No deprecated or removed features are being used
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential breaking changes impact
# Search for undici imports and usage patterns that might be affected
echo "Searching for undici imports and usage patterns..."
rg -t ts -t js "from ['\"](undici|node:undici)['\"]"
# Look for HTTP/2 related configurations
echo "Checking for HTTP/2 configurations..."
rg -t ts -t js "allowH2|http2"
# Search for potential compatibility issues in test files
echo "Checking test files for compatibility patterns..."
fd -t f -e test.ts -e test.js | xargs rg "undici|http2"
Length of output: 3173
Script:
#!/bin/bash
# Check for specific undici usage patterns that might be affected by v7 breaking changes
# Search for specific Dispatcher usage patterns
echo "Checking Dispatcher usage patterns..."
ast-grep --pattern 'new Dispatcher.$$$'
# Search for specific Request configurations
echo "Checking Request configurations..."
ast-grep --pattern 'new Request($$$)'
# Search for specific fetch API usage
echo "Checking fetch API usage..."
ast-grep --pattern 'fetch($$$)'
# Look for specific HTTP agent configurations
echo "Checking HTTP agent configurations..."
ast-grep --pattern 'new Agent($$$)'
Length of output: 1646
src/fetch.ts (3)
53-53: Verify type rename consistency across codebase
The type has been renamed from UnidiciTimingInfo to UndiciTimingInfo. Let's ensure this change is consistent.
✅ Verification successful
Type rename is intentionally aliased for compatibility
The UnidiciTimingInfo interface is intentionally kept as an alias extending UndiciTimingInfo with a comment explicitly stating "keep typo compatibility". This is a proper backward compatibility pattern, and the changes in fetch.ts correctly use the new UndiciTimingInfo type.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of UnidiciTimingInfo
rg "UnidiciTimingInfo"
# Search for all uses of UndiciTimingInfo to verify correct usage
rg "UndiciTimingInfo"
Length of output: 339
Script:
#!/bin/bash
# Get more context around the UnidiciTimingInfo interface in HttpClient.ts
rg -B 2 -A 5 "UnidiciTimingInfo" src/HttpClient.ts
Length of output: 173
17-17: Caution: Importing from internal undici modules
The import from undici/lib/web/fetch/response.js is accessing an internal module that's not part of undici's public API. This could break in future undici updates.
Let's verify if there's a public API alternative:
Consider:
- Using only the public API if possible
- If not possible, add a comment explaining why internal API usage is necessary
- Pin the undici version more strictly to avoid breaking changes
✅ Verification successful
Internal API usage is acceptable in this context
The usage of getResponseState from undici's internal module appears to be necessary as:
- The package is already pinned to undici v7 (
"undici": "^7.0.0") - There's no evidence of a public API alternative for accessing response state
- The function is used specifically for internal state management (
const state = getResponseState(res!))
The risk is mitigated by the version constraint, and this pattern is sometimes necessary when working with low-level HTTP implementations. No changes required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential public exports of getResponseState
rg -l "export.*getResponseState"
# Check if this is documented in undici's public API
curl -s https://raw.githubusercontent.com/nodejs/undici/main/docs/api/Response.md | grep -i "getResponseState"
Length of output: 145
Script:
#!/bin/bash
# Let's try a broader search to understand the usage context
# Search for any occurrence of getResponseState
rg "getResponseState"
# Let's also check the package.json to see the undici version
cat package.json | grep undici
# Search for any Response-related imports from undici to find potential alternatives
rg "from 'undici.*Response"
Length of output: 420
238-239: Verify memory usage improvement with H2
The changes to response state handling are related to the PR's objective of fixing OOM issues with allowH2. The implementation now uses getResponseState to access timing information more directly.
Let's check for related memory handling in the codebase:
Consider adding:
- Memory usage monitoring in the diagnostics channel
- Documentation about the memory improvements
- Tests specifically for H2 connections with memory tracking
Also applies to: 252-252
| export class FormData extends _FormData { | ||
| _getContentDisposition(value: any, options: any) { | ||
| // support non-ascii filename | ||
| // https://github.com/form-data/form-data/pull/571 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #547 +/- ##
==========================================
- Coverage 94.75% 94.42% -0.34%
==========================================
Files 9 10 +1
Lines 1164 1255 +91
Branches 288 285 -3
==========================================
+ Hits 1103 1185 +82
- Misses 57 66 +9
Partials 4 4 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (7)
src/FormData.ts (1)
5-5: Consider adding type definitions for method parametersThe method parameters are currently typed as
any. Consider defining proper interfaces or types for better type safety.- _getContentDisposition(value: any, options: any) { + interface ContentDispositionOptions { + filepath?: string; + filename?: string; + } + interface ContentValue { + name?: string; + path?: string; + readable?: boolean; + client?: { + _httpMessage?: { + path?: string; + }; + }; + } + _getContentDisposition(value: ContentValue, options: ContentDispositionOptions) {src/diagnosticsChannel.ts (2)
211-211: LGTM: Complete request lifecycle observabilityThe debug logging addition completes the consistent error handling pattern across the entire request lifecycle.
Consider implementing metrics collection using these debug points to:
- Track request lifecycle durations
- Monitor memory usage patterns
- Detect potential memory leaks in HTTP/2 connections
Line range hint
146-211: Comprehensive logging enhancement aligns with HTTP/2 improvementsThe systematic addition of debug logging across all request lifecycle phases provides a solid foundation for:
- Tracking HTTP/2 connection states
- Identifying potential memory leaks
- Debugging request lifecycle issues
This aligns well with the PR objectives of addressing OOM issues with HTTP/2 connections.
Consider adding the following enhancements:
- Structured logging format for easier parsing
- Memory usage metrics at key lifecycle points
- Automatic cleanup triggers for potential memory leak points
test/options.files.test.ts (1)
Line range hint
1-305: Consider adding HTTP/2 specific test casesWhile the improvements to file upload testing are valuable, considering the PR's objective to fix OOM issues with
allowH2, it would be beneficial to add specific test cases that:
- Verify file uploads over HTTP/2
- Test memory usage patterns with
allowH2: true- Include large file upload scenarios to stress test the system
This would help ensure the undici v7 upgrade properly addresses the OOM issues mentioned in issue #541.
Would you like help in designing these additional test cases?
🧰 Tools
🪛 GitHub Check: Node.js / Test (windows-latest, 23)
[failure] 240-240: test/options.files.test.ts > options.files.test.ts > should upload a file with args.data success
AssertionError: Expected values to be strictly equal:25 !== 24
- Expected
- Received
- 24
- 25
❯ test/options.files.test.ts:240:12
🪛 GitHub Check: Node.js / Test (windows-latest, 18.19.0)
[failure] 240-240: test/options.files.test.ts > options.files.test.ts > should upload a file with args.data success
AssertionError: Expected values to be strictly equal:25 !== 24
- Expected
- Received
- 24
- 25
❯ test/options.files.test.ts:240:12
🪛 GitHub Check: Node.js / Test (windows-latest, 20)
[failure] 240-240: test/options.files.test.ts > options.files.test.ts > should upload a file with args.data success
AssertionError: Expected values to be strictly equal:25 !== 24
- Expected
- Received
- 24
- 25
❯ test/options.files.test.ts:240:12
🪛 GitHub Check: Node.js / Test (windows-latest, 18)
[failure] 240-240: test/options.files.test.ts > options.files.test.ts > should upload a file with args.data success
AssertionError: Expected values to be strictly equal:25 !== 24
- Expected
- Received
- 24
- 25
❯ test/options.files.test.ts:240:12
src/HttpClient.ts (1)
470-491: Add null checks and improve debug logging efficiency.The file upload handling could be improved in two areas:
- Add null/undefined checks before MIME type lookup
- Use template literals for debug logging
Consider applying these changes:
- let fileName = ''; + let fileName: string | null = null; let value: any; if (typeof file === 'string') { - fileName = basename(file); + fileName = basename(file) || null; } else if (Buffer.isBuffer(file)) { - fileName = customFileName || `bufferfile${index}`; + fileName = customFileName || `bufferfile${index}` || null; } else if (file instanceof Readable || isReadable(file as any)) { - fileName = getFileName(file) || customFileName || `streamfile${index}`; + fileName = getFileName(file) || customFileName || `streamfile${index}` || null; isStreamingRequest = true; value = file; } - const mimeType = mime.lookup(fileName) || ''; + const mimeType = fileName ? mime.lookup(fileName) || '' : ''; formData.append(field, value, { filename: fileName, contentType: mimeType, }); - debug('formData append field: %s, mimeType: %s, fileName: %s', - field, mimeType, fileName); + debug(`formData append field: ${field}, mimeType: ${mimeType}, fileName: ${fileName}`);test/HttpClient.test.ts (2)
Line range hint
57-80: Remove commented-outconsole.logstatements to clean up the code.The commented-out
console.logstatements at lines 57, 60, and 80 can be removed to enhance code readability and maintainability.Apply this diff to remove the unnecessary code:
- // console.log(response.res.socket, response.res.timing);
115-117: Use constants for repeated URLs to improve maintainability.Hardcoding the URL
'https://registry.npmmirror.com/urllib'multiple times may lead to difficulties in maintenance. Consider defining it as a constant variable.Example:
+ const registryUrl = 'https://registry.npmmirror.com/urllib'; let response = await httpClient.request(registryUrl);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
package.json(2 hunks)src/FormData.ts(1 hunks)src/HttpClient.ts(7 hunks)src/diagnosticsChannel.ts(4 hunks)test/HttpClient.test.ts(4 hunks)test/diagnostics_channel.test.ts(1 hunks)test/fetch.test.ts(1 hunks)test/options.dispatcher.test.ts(1 hunks)test/options.files.test.ts(3 hunks)test/options.followRedirect.test.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/fetch.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🪛 Biome (1.9.4)
src/FormData.ts
[error] 19-19: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
🪛 GitHub Check: codecov/patch
src/FormData.ts
[warning] 13-13: src/FormData.ts#L13
Added line #L13 was not covered by tests
[warning] 21-22: src/FormData.ts#L21-L22
Added lines #L21 - L22 were not covered by tests
🪛 GitHub Check: Node.js / Test (windows-latest, 23)
test/options.files.test.ts
[failure] 240-240: test/options.files.test.ts > options.files.test.ts > should upload a file with args.data success
AssertionError: Expected values to be strictly equal:
25 !== 24
- Expected
- Received
- 24
- 25
❯ test/options.files.test.ts:240:12
🪛 GitHub Check: Node.js / Test (windows-latest, 18.19.0)
test/options.files.test.ts
[failure] 240-240: test/options.files.test.ts > options.files.test.ts > should upload a file with args.data success
AssertionError: Expected values to be strictly equal:
25 !== 24
- Expected
- Received
- 24
- 25
❯ test/options.files.test.ts:240:12
test/options.followRedirect.test.ts
[failure] 88-88: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:88:12
🪛 GitHub Check: Node.js / Test (windows-latest, 20)
test/options.files.test.ts
[failure] 240-240: test/options.files.test.ts > options.files.test.ts > should upload a file with args.data success
AssertionError: Expected values to be strictly equal:
25 !== 24
- Expected
- Received
- 24
- 25
❯ test/options.files.test.ts:240:12
test/options.followRedirect.test.ts
[failure] 88-88: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:88:12
🪛 GitHub Check: Node.js / Test (windows-latest, 18)
test/options.files.test.ts
[failure] 240-240: test/options.files.test.ts > options.files.test.ts > should upload a file with args.data success
AssertionError: Expected values to be strictly equal:
25 !== 24
- Expected
- Received
- 24
- 25
❯ test/options.files.test.ts:240:12
test/options.followRedirect.test.ts
[failure] 88-88: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:88:12
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 20)
test/options.followRedirect.test.ts
[failure] 88-88: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:88:12
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18.19.0)
test/options.followRedirect.test.ts
[failure] 88-88: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:88:12
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18)
test/options.followRedirect.test.ts
[failure] 88-88: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:88:12
🪛 GitHub Check: Node.js / Test (macos-latest, 18.19.0)
test/options.followRedirect.test.ts
[failure] 88-88: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:88:12
🪛 GitHub Check: Node.js / Test (macos-latest, 20)
test/options.followRedirect.test.ts
[failure] 88-88: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:88:12
🪛 GitHub Check: Node.js / Test (macos-latest, 18)
test/options.followRedirect.test.ts
[failure] 88-88: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:88:12
🔇 Additional comments (20)
src/FormData.ts (3)
1-4: LGTM: Clean and well-structured class declaration
The imports and class extension are properly implemented.
1-32: Verify integration with undici v7
Since this PR is part of the undici v7 upgrade to fix OOM issues, we should verify that this FormData implementation works correctly with undici's request handling, especially with HTTP/2 enabled.
Let's check for undici-related configurations:
Consider documenting any specific requirements or limitations when using this FormData implementation with HTTP/2 enabled requests.
✅ Verification successful
Let's gather more information about how FormData is used with undici to verify the integration.
Let's check how the FormData is being used in tests and if there are any HTTP/2 specific test cases.
Based on the gathered information, I can now generate the final response:
FormData implementation is properly integrated with undici v7 and HTTP/2
The FormData implementation is correctly integrated with undici v7 and works with both HTTP/1.1 and HTTP/2 as evidenced by:
- FormData is properly imported and used in HttpClient.ts for file uploads
- Extensive test coverage for FormData functionality in
options.files.test.ts - HTTP/2 support is tested with
allowH2option inHttpClient.test.ts - The content-disposition implementation specifically handles non-ASCII filenames, which is important for both HTTP/1.1 and HTTP/2 transfers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for undici configuration and usage
rg -l "undici|allowH2" --type ts
Length of output: 282
Script:
#!/bin/bash
# Search for FormData usage in undici-related files
rg -A 5 "FormData" src/HttpClient.ts src/Request.ts src/fetch.ts
# Also check tests for FormData usage
rg -A 5 "FormData" test/HttpClient.test.ts
Length of output: 891
Script:
#!/bin/bash
# Search for FormData usage in test files
rg -A 10 "FormData|form-data" test/
# Check for HTTP/2 related tests
rg -A 5 "allowH2|http2|HTTP/2" test/
Length of output: 14789
🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
🪛 GitHub Check: codecov/patch
[warning] 13-13: src/FormData.ts#L13
Added line #L13 was not covered by tests
[warning] 21-22: src/FormData.ts#L21-L22
Added lines #L21 - L22 were not covered by tests
13-13: Add test coverage for path normalization and HTTP response handling
The static analysis indicates missing test coverage for:
- Path normalization logic (line 13)
- HTTP response path extraction (lines 21-22)
Let's verify the existing test coverage:
Would you like me to help create test cases for these scenarios?
Also applies to: 21-22
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 13-13: src/FormData.ts#L13
Added line #L13 was not covered by tests
test/options.dispatcher.test.ts (2)
43-43: Approved: Enhanced security by using HTTPS
The change from HTTP to HTTPS for the registry.npmmirror.com endpoint aligns with security best practices.
Line range hint 71-86: Consider enhancing HTTP/2 test coverage
Given that this PR addresses OOM issues with allowH2=true (issue #541), the current HTTP/2 test case might not adequately cover the memory leak scenario. Consider adding test cases that:
- Make multiple concurrent requests to verify memory stability
- Test connection closure scenarios that could trigger the OOM issue
Let's verify the HTTP/2 test coverage across the codebase:
test/options.followRedirect.test.ts (3)
6-6: LGTM: TypeScript migration
The file extension change and updated test description reflect proper TypeScript migration.
84-84: LGTM: Improved type safety
Good improvements in type handling:
- Added generic type
<Buffer>to explicitly type the response - Removed redundant type assertion on
data
Also applies to: 89-89
Line range hint 82-93: Fix failing redirect test across all Node.js versions
The test is consistently failing across all Node.js versions and platforms with the following assertion error:
Expected: 200
Received: 301
This suggests that the upgrade to undici v7 might have changed how HTTP 301 redirects are handled when followRedirect: true is set.
Let's verify the behavior:
Consider these potential fixes:
- If undici v7 has changed redirect behavior:
it('should redirect `location: /redirect-full-301-to-url`', async () => {
const requestURL = `${_url}redirect-full-301`;
const { data, res, redirected, url, requestUrls } = await urllib.request<Buffer>(requestURL, {
followRedirect: true,
});
- assert.equal(res.statusCode, 200);
+ // In undici v7, we get the initial redirect status
+ assert.equal(res.statusCode, 301);
assert(data.length > 100);
assert(redirected);
assert.equal(url, `${_url}redirect-full-301-to-url`);
assert.equal(requestUrls.length, 2);
});- If this is a regression:
const { data, res, redirected, url, requestUrls } = await urllib.request<Buffer>(requestURL, {
followRedirect: true,
+ // Force following redirects
+ maxRedirects: 10,
});🧰 Tools
🪛 GitHub Check: Node.js / Test (windows-latest, 18.19.0)
[failure] 88-88: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:88:12
🪛 GitHub Check: Node.js / Test (windows-latest, 20)
[failure] 88-88: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:88:12
🪛 GitHub Check: Node.js / Test (windows-latest, 18)
[failure] 88-88: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:88:12
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 20)
[failure] 88-88: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:88:12
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18.19.0)
[failure] 88-88: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:88:12
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18)
[failure] 88-88: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:88:12
🪛 GitHub Check: Node.js / Test (macos-latest, 18.19.0)
[failure] 88-88: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:88:12
🪛 GitHub Check: Node.js / Test (macos-latest, 20)
[failure] 88-88: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:88:12
🪛 GitHub Check: Node.js / Test (macos-latest, 18)
[failure] 88-88: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:88:12
src/diagnosticsChannel.ts (3)
146-149: LGTM: Enhanced error logging improves observability
The addition of debug logging when the opaque object is not found will help diagnose request tracking issues, particularly relevant for HTTP/2 connections mentioned in the PR objectives.
171-174: LGTM: Consistent error logging pattern
The debug logging addition maintains a consistent error handling pattern across request lifecycle events, which is crucial for tracking HTTP/2 request states.
185-188: LGTM: Enhanced response tracking for HTTP/2
The debug logging addition will help track response handling, particularly important for HTTP/2 connections where memory issues were reported.
Let's verify the memory handling improvements:
✅ Verification successful
LGTM: Debug logging is properly integrated with request tracking system
The debug logging addition at line 185-188 is correctly integrated with the existing request tracking system. The code:
- Is consistent with other diagnostic channel handlers in the codebase
- Properly checks for request ID before proceeding
- Helps track HTTP/2 requests through the entire request lifecycle
- Works alongside the comprehensive request timing and performance tracking system
The implementation follows the established pattern used across all diagnostic channel handlers, ensuring consistent request tracking and debugging capabilities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for memory management related code in HTTP/2 handling
rg -A 3 "allowH2|http2"
# Look for other memory management improvements
ast-grep --pattern 'new WeakMap()'
ast-grep --pattern 'new WeakRef'
Length of output: 4419
Script:
#!/bin/bash
# Let's check the diagnostics channel implementation for memory handling
rg -A 5 "symbols\.kRequestId|WeakRef|WeakMap" src/diagnosticsChannel.ts
# Also check for any cleanup or disposal patterns
ast-grep --pattern 'dispose() {
$$$
}'
# Look for request tracking implementations
rg -A 5 "requestId|kRequestId" src/
Length of output: 14667
test/diagnostics_channel.test.ts (2)
46-47: Consider making the handler access more robust
The current implementation has a few potential issues:
- Direct access to internal symbols (
kHandler) could break with future undici updates - The nested property access could be simplified
Consider refactoring to:
- let opaque = request[kHandler].opts?.opaque ?? request[kHandler].opaque;
- opaque = opaque[symbols.kRequestOriginalOpaque];
+ const handler = request[kHandler];
+ const requestOpaque = handler?.opts?.opaque ?? handler?.opaque;
+ const opaque = requestOpaque?.[symbols.kRequestOriginalOpaque];Let's verify if this internal structure is consistent across different undici versions:
Line range hint 1-365: Enhance memory leak detection in tests
Given the PR's objective to fix OOM issues with allowH2, the tests should explicitly verify memory usage patterns.
Consider adding:
- Memory usage assertions during the 1000 iterations
- Explicit HTTP/2 test cases with
allowH2: true - Socket cleanup verification after tests
Example addition:
// Add before the iteration
const initialMemory = process.memoryUsage();
// Add after the iteration
const finalMemory = process.memoryUsage();
// Allow for some variance but catch significant leaks
assert(
finalMemory.heapUsed - initialMemory.heapUsed < 50 * 1024 * 1024,
'Memory usage increased significantly'
);Let's check if there are any existing memory-related tests:
test/options.files.test.ts (3)
300-300: LGTM: Consistent MIME type handling
The MIME type assertion is correctly updated to match the file extension and is consistent with other JavaScript file assertions in the test suite.
217-222: LGTM: Good addition of emoji filename testing
The inclusion of emoji filename testing improves internationalization coverage. However, let's verify the test file existence across platforms.
✅ Verification successful
✅ Emoji filename support is properly implemented
The verification confirms that both test files exist in the fixtures directory:
test/fixtures/foo.txttest/fixtures/😄foo😭.txt
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence of test files across different OS environments
fd --type f --glob "*foo*.txt" test/fixtures/
Length of output: 95
238-244:
Fix: Platform-dependent test failure due to line endings
The test is failing on Windows because the expected file size (24) doesn't match the actual size (25). This is likely due to different line ending handling (CRLF on Windows vs LF on Unix).
Consider these solutions:
- Make the test platform-agnostic:
- assert.equal(response.data.files.file1.size, 24);
+ const expectedSize = process.platform === 'win32' ? 25 : 24;
+ assert.equal(response.data.files.file1.size, expectedSize);- Normalize line endings when reading the file:
- const txtValue = await fs.readFile(txt, 'utf-8');
+ const txtValue = (await fs.readFile(txt, 'utf-8')).replace(/\r\n/g, '\n');🧰 Tools
🪛 GitHub Check: Node.js / Test (windows-latest, 23)
[failure] 240-240: test/options.files.test.ts > options.files.test.ts > should upload a file with args.data success
AssertionError: Expected values to be strictly equal:
25 !== 24
- Expected
- Received
- 24
- 25
❯ test/options.files.test.ts:240:12
🪛 GitHub Check: Node.js / Test (windows-latest, 18.19.0)
[failure] 240-240: test/options.files.test.ts > options.files.test.ts > should upload a file with args.data success
AssertionError: Expected values to be strictly equal:
25 !== 24
- Expected
- Received
- 24
- 25
❯ test/options.files.test.ts:240:12
🪛 GitHub Check: Node.js / Test (windows-latest, 20)
[failure] 240-240: test/options.files.test.ts > options.files.test.ts > should upload a file with args.data success
AssertionError: Expected values to be strictly equal:
25 !== 24
- Expected
- Received
- 24
- 25
❯ test/options.files.test.ts:240:12
🪛 GitHub Check: Node.js / Test (windows-latest, 18)
[failure] 240-240: test/options.files.test.ts > options.files.test.ts > should upload a file with args.data success
AssertionError: Expected values to be strictly equal:
25 !== 24
- Expected
- Received
- 24
- 25
❯ test/options.files.test.ts:240:12
src/HttpClient.ts (3)
Line range hint 51-74: LGTM! Well-structured interface changes.
The renaming of UnidiciTimingInfo to UndiciTimingInfo fixes the typo while maintaining backward compatibility through interface extension.
548-549: Fix format specifiers in debug statements.
The debug statements use incorrect format specifiers. Use %o for objects and %d for numbers.
Also applies to: 691-692, 704-705
Line range hint 1-745: Verify HTTP/2 implementation and memory usage.
Since this PR aims to fix OOM issues with allowH2=true, we should verify the HTTP/2 implementation.
✅ Verification successful
Based on the search results, I can now verify the HTTP/2 implementation. The code shows:
- Proper HTTP/2 configuration through
allowH2option in multiple places:- In
HttpClientconstructor with three scenarios:- With custom lookup/checkAddress using
HttpAgent - With connect options using
Agent - With just
allowH2: trueusingAgent
- With custom lookup/checkAddress using
- In
- Tests validating HTTP/2 functionality:
- Test cases with
allowH2: true/false - HTTP/2-specific header tests (
x-custom-h2)
- Test cases with
- Example files demonstrating HTTP/2 usage
- Memory management:
- Uses undici's
Agentwhich has built-in connection pooling - No explicit memory leaks in the implementation
- Uses undici's
HTTP/2 implementation is correctly done and memory usage is properly managed through undici's Agent.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for HTTP/2 related configuration and error handling
rg -A 5 'allowH2|HTTP2|h2'
# Search for memory management related code
ast-grep --pattern 'new Agent({
$$$
allowH2: $_,
$$$
})'
Length of output: 9115
test/HttpClient.test.ts (1)
3-8: Appropriate imports added for new HTTP/2 test cases.
The added imports from node:events, node:http2, node:timers/promises, and https-pem are necessary and correctly implemented to support the new HTTP/2 test scenarios.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
test/options.followRedirect.test.ts (1)
83-110: Consider comprehensive redirect handling refactorGiven the PR's objectives to fix OOM issues and server-side exits, and the consistent test failures, consider:
- Reviewing undici v7's documentation for changes in redirect handling
- Implementing proper cleanup of resources after redirects to prevent memory leaks
- Adding explicit tests for memory usage during redirects
- Documenting any breaking changes in redirect behavior
Would you like me to help create:
- Memory leak detection tests
- Resource cleanup implementation
- Documentation for breaking changes
🧰 Tools
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 20)
[failure] 89-89: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should urllib.redirect
location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:89:12
[failure] 104-104: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should httpClient.redirect
location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:104:12
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18.19.0)
[failure] 89-89: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should urllib.redirect
location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:89:12
[failure] 104-104: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should httpClient.redirect
location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:104:12
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18)
[failure] 89-89: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should urllib.redirect
location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:89:12
[failure] 104-104: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should httpClient.redirect
location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:104:12
🪛 GitHub Check: Node.js / Test (macos-latest, 18)
[failure] 89-89: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should urllib.redirect
location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:89:12
[failure] 104-104: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should httpClient.redirect
location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:104:12
🪛 GitHub Check: Node.js / Test (macos-latest, 20)
[failure] 89-89: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should urllib.redirect
location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:89:12
[failure] 104-104: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should httpClient.redirect
location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:104:12
🪛 GitHub Check: Node.js / Test (macos-latest, 18.19.0)
[failure] 89-89: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should urllib.redirect
location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:89:12
[failure] 104-104: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should httpClient.redirect
location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:104:12
test/diagnostics_channel.test.ts (1)
Line range hint
108-147: Consider adding memory tracking assertionsWhile the high-volume tests (1000 requests) are good for testing socket reuse, consider adding memory tracking assertions to:
- Monitor heap usage during the 1000 iterations
- Verify no memory leaks occur during socket reuse
- Ensure socket cleanup happens properly
This would help prevent potential memory issues, especially relevant to the OOM concerns mentioned in issue #541.
Example approach:
const initialMemory = process.memoryUsage(); // ... run the 1000 requests ... const finalMemory = process.memoryUsage(); // Assert reasonable memory growth assert(finalMemory.heapUsed - initialMemory.heapUsed < acceptableThreshold);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
test/diagnostics_channel.test.ts(1 hunks)test/options.files.test.ts(3 hunks)test/options.followRedirect.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/options.files.test.ts
🧰 Additional context used
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 20)
test/options.followRedirect.test.ts
[failure] 89-89: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should urllib.redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:89:12
[failure] 104-104: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should httpClient.redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:104:12
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18.19.0)
test/options.followRedirect.test.ts
[failure] 89-89: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should urllib.redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:89:12
[failure] 104-104: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should httpClient.redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:104:12
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18)
test/options.followRedirect.test.ts
[failure] 89-89: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should urllib.redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:89:12
[failure] 104-104: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should httpClient.redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:104:12
🪛 GitHub Check: Node.js / Test (macos-latest, 18)
test/options.followRedirect.test.ts
[failure] 89-89: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should urllib.redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:89:12
[failure] 104-104: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should httpClient.redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:104:12
🪛 GitHub Check: Node.js / Test (macos-latest, 20)
test/options.followRedirect.test.ts
[failure] 89-89: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should urllib.redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:89:12
[failure] 104-104: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should httpClient.redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:104:12
🪛 GitHub Check: Node.js / Test (macos-latest, 18.19.0)
test/options.followRedirect.test.ts
[failure] 89-89: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should urllib.redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:89:12
[failure] 104-104: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should httpClient.redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:104:12
🔇 Additional comments (5)
test/options.followRedirect.test.ts (2)
4-4: LGTM: Import changes and TypeScript migration
The addition of HttpClient import and migration to TypeScript align well with the codebase modernization.
Also applies to: 7-7
97-110: Verify undici v7 redirect handling changes
Both urllib and HttpClient implementations show identical failures, suggesting a breaking change in undici v7's redirect handling. This needs investigation and potential adaptation of our implementation.
Let's check for undici v7 breaking changes and our implementation:
🧰 Tools
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 20)
[failure] 104-104: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should httpClient.redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:104:12
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18.19.0)
[failure] 104-104: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should httpClient.redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:104:12
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18)
[failure] 104-104: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should httpClient.redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:104:12
🪛 GitHub Check: Node.js / Test (macos-latest, 18)
[failure] 104-104: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should httpClient.redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:104:12
🪛 GitHub Check: Node.js / Test (macos-latest, 20)
[failure] 104-104: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should httpClient.redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:104:12
🪛 GitHub Check: Node.js / Test (macos-latest, 18.19.0)
[failure] 104-104: test/options.followRedirect.test.ts > options.followRedirect.test.ts > should httpClient.redirect location: /redirect-full-301-to-url
AssertionError: Expected values to be strictly equal:
301 !== 200
- Expected
- Received
- 200
- 301
❯ test/options.followRedirect.test.ts:104:12
test/diagnostics_channel.test.ts (3)
46-48: LGTM: Safer opaque object access pattern
The change to use optional chaining (?.) and fallback with || provides better null safety when accessing the opaque object.
Line range hint 246-307: LGTM: Comprehensive error handling test coverage
The error handling tests effectively cover:
- Network errors with proper socket state validation
- Response decoding errors with appropriate error types
- Socket lifecycle tracking
- Error property verification
This provides good confidence in the error handling capabilities after the undici upgrade.
Line range hint 1-24: Add HTTP/2 specific test cases
Given that this PR addresses OOM issues with allowH2=true (from issue #541), consider adding specific test cases that:
- Verify memory usage patterns with HTTP/2 enabled
- Test concurrent streams behavior
- Validate connection pooling with HTTP/2
This will help ensure the OOM issues are properly resolved.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
test/index.test.ts (2)
151-153: Consider strengthening error handling assertionsThe conditional check for
err.codesuggests that the error structure might have changed with the undici v7 upgrade. While this makes the test more resilient, we should:
- Document why the error code might be undefined in some cases
- Consider asserting additional error properties that are guaranteed to be present
- if (err.code) { - assert.equal(err.code, 'HPE_INVALID_CHUNK_SIZE'); - } + // With undici v7, verify core error properties + assert(err.name === 'HTTPParserError', 'Error should be HTTPParserError'); + if (err.code) { + assert.equal(err.code, 'HPE_INVALID_CHUNK_SIZE', 'Error code should match when present'); + }
Line range hint
13-24: Add tests for HTTP/2 functionality and memory usageGiven that this PR upgrades to undici v7 to fix OOM issues with
allowH2=true, we should add test cases to:
- Verify HTTP/2 functionality works correctly
- Ensure memory usage remains stable with
allowH2=trueWould you like me to help create test cases that:
- Exercise HTTP/2 functionality with
allowH2=true- Monitor memory usage during HTTP/2 requests
- Verify no memory leaks occur during prolonged HTTP/2 sessions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
test/index.test.ts(1 hunks)
🔇 Additional comments (1)
test/index.test.ts (1)
Line range hint 5-10: Verify comprehensive usage of undici v7 features
Let's ensure we're utilizing all relevant undici v7 features in our tests.
✅ Verification successful
Let me gather more specific information about the test coverage of undici v7 features.
Based on the results, I can now provide a final response about the test coverage of undici v7 features.
Test coverage for undici v7 features is comprehensive
The test suite adequately covers key undici v7 features:
- HTTP/2 support is well tested with specific test cases for
allowH2configuration - MockAgent and global dispatcher management is thoroughly covered with proper setup/teardown
- Integration with undici's core features is verified through multiple test scenarios
The test file's imports and usage align well with undici v7's capabilities, showing no significant gaps in coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for undici v7 feature usage and identify potential gaps in test coverage
# Check if we're importing all relevant undici v7 features
rg -l "import.*from.*undici" | xargs rg "import \{.*\} from.*undici"
# Look for HTTP/2 related configuration
rg -l "allowH2|http2"
# Search for memory management related tests
rg -l "memory|leak|oom|gc"
Length of output: 679
Script:
#!/bin/bash
# Check HTTP/2 test coverage details
rg -A 5 "allowH2|http2" test/HttpClient.test.ts test/options.dispatcher.test.ts
# Check specific undici v7 features in tests
rg -A 5 "MockAgent|setGlobalDispatcher|getGlobalDispatcher" test/
# Check memory/leak test implementations
rg -A 5 "memory|leak" test/HttpClient.test.ts
Length of output: 6211
|
follow redirect 使用 undici 默认的实现有问题，得自定义实现才行。 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
test/HttpClient.test.ts (2)
57-57: Remove commented console.log statementsThese debugging statements should be removed rather than commented out to maintain clean test code.
- // console.log(response.res.socket, response.res.timing); - // console.log(response.res.socket, response.res.timing); - // console.log(response.res.socket, response.res.timing); - // console.log(httpClient.getDispatcherPoolStats());Also applies to: 60-60, 80-80, 115-115
174-233: Enhance redirect test coverageThe redirect test only covers the basic 302 redirect case. Consider adding tests for:
- Maximum redirect depth
- Redirect loops
- Different redirect status codes (301, 303, 307, 308)
src/HttpClient.ts (2)
175-182: Consider using an enum for redirect status codes.The redirect status codes array could be converted to an enum or constant object for better type safety and documentation.
-const RedirectStatusCodes = [ - 301, // Moved Permanently - 302, // Found - 303, // See Other - 307, // Temporary Redirect - 308, // Permanent Redirect -]; +enum RedirectStatusCode { + MovedPermanently = 301, + Found = 302, + SeeOther = 303, + TemporaryRedirect = 307, + PermanentRedirect = 308, +} +const RedirectStatusCodes = Object.values(RedirectStatusCode);
421-422: Document the redirect handling change.The code disables undici's built-in redirect handler in favor of a custom implementation. This should be documented to explain the rationale.
-// disable undici auto redirect handler +// Disable undici's built-in redirect handler to: +// 1. Have more control over redirect behavior +// 2. Implement custom security checks +// 3. Track redirect history maxRedirections: 0,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/HttpClient.ts(15 hunks)test/HttpClient.test.ts(4 hunks)
🔇 Additional comments (7)
test/HttpClient.test.ts (4)
3-4: LGTM: New imports support HTTP/2 testing
The added imports (once, createSecureServer, and pem) are appropriate for testing HTTP/2 functionality and secure server creation.
Also applies to: 8-8
146-147:
Security: Avoid disabling SSL verification in tests
Setting rejectUnauthorized: false disables SSL/TLS certificate verification. Consider using proper certificates for testing.
128-128:
Add null check for session destruction
Using the non-null assertion operator (!) could lead to runtime errors if the session is null.
- stream.session!.destroy();
+ if (stream.session) {
+ stream.session.destroy();
+ }Likely invalid or redundant comment.
116-118: Verify dispatcher pool connection limits
The test verifies connection pooling but doesn't explicitly test for memory leaks. Consider adding memory usage assertions to verify the OOM fix.
src/HttpClient.ts (3)
Line range hint 51-73: LGTM! Interface renaming with backward compatibility.
The renaming from UnidiciTimingInfo to UndiciTimingInfo is done correctly with a compatibility interface to prevent breaking changes.
Line range hint 710-724: Fix format specifiers in debug statement.
The debug statement uses incorrect format specifiers for the error object and numbers.
607-619:
Verify redirect security implications.
The redirect implementation needs additional security checks to prevent open redirect vulnerabilities.
Apply this diff to add security checks:
if (res.headers.location) {
+ const targetUrl = new URL(res.headers.location, requestUrl.href);
+ // Prevent redirects to potentially malicious protocols
+ if (!['http:', 'https:'].includes(targetUrl.protocol)) {
+ throw new Error(`Unsafe redirect to ${targetUrl.protocol} protocol`);
+ }
requestContext.redirects++;
- const nextUrl = new URL(res.headers.location, requestUrl.href);
// Ensure the response is consumed
await response.body.arrayBuffer();
debug('Request#%d got response, status: %s, headers: %j, timing: %j, redirect to %s',
- requestId, res.status, res.headers, res.timing, nextUrl.href);
+ requestId, res.status, res.headers, res.timing, targetUrl.href);
- return await this.#requestInternal(nextUrl.href, options, requestContext);
+ return await this.#requestInternal(targetUrl.href, options, requestContext);
}
|
先发一个 beta 版本在 cnpmcore 验证完没问题，再合并发布一个新版本 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
test/options.files.test.ts (2)
39-52: LGTM! Consider enhancing error message verification.The test case effectively verifies error handling for non-existent files. The assertions for error code and status are comprehensive.
Consider adding an assertion to verify the error message content:
}, (err: any) => { assert.equal(err.code, 'ENOENT'); assert.equal(err.res.status, -1); assert.equal(err.status, -1); + assert.match(err.message, /ENOENT: no such file or directory/); return true; });
232-233: LGTM! Consider adding more Unicode filename test cases.The test case properly handles and verifies files with emoji in their names, which is important for international compatibility.
Consider adding more edge cases with different Unicode characters:
- Right-to-left characters (Arabic/Hebrew)
- Zero-width characters
- Characters that look similar (homoglyphs)
Also applies to: 238-238, 254-256
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
test/options.files.test.ts(4 hunks)
🔇 Additional comments (1)
test/options.files.test.ts (1)
316-316: LGTM! MIME type correctly matches file extension.
The assertion now properly expects 'application/javascript' for .js files, which is consistent with the file extension-based MIME type detection used throughout the test suite.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
scripts/replace_urllib_version.js (2)
Line range hint
7-12: Consider adding path validation for security.The script reads files from the dist directory without validating that the resolved paths don't escape the intended directory through path traversal.
Consider adding path validation:
const root = process.cwd(); const pkg = JSON.parse(await fs.readFile(path.join(root, 'package.json'))); + const distDir = path.join(root, 'dist'); + + // Validate dist directory exists + try { + await fs.access(distDir); + } catch (err) { + throw new Error('dist directory not found'); + } + const files = [ - path.join(root, 'dist/commonjs/HttpClient.js'), - path.join(root, 'dist/esm/HttpClient.js'), + path.join(distDir, 'commonjs/HttpClient.js'), + path.join(distDir, 'esm/HttpClient.js'), ]; + + // Validate all paths are within dist directory + for (const file of files) { + if (!file.startsWith(distDir)) { + throw new Error(`Invalid file path: ${file}`); + } + }
Line range hint
1-1: Add TypeScript type checking for better maintainability.Given that this is part of a TypeScript project (as indicated by the presence of
.tsfiles in the codebase), consider converting this script to TypeScript for better type safety and maintainability.Consider renaming the file to
replace_urllib_version.tsand adding type annotations:#!/usr/bin/env node import fs from 'node:fs/promises'; import path from 'node:path'; interface PackageJson { version: string; [key: string]: unknown; } async function main(): Promise<void> { // ... rest of the code with type annotations } main().catch(err => { console.error('Error:', err); process.exit(1); });test/options.followRedirect.test.ts (2)
122-135: Consider refactoring duplicate test assertionsThe test assertions are duplicated between urllib and HttpClient tests. Consider extracting common assertions into a shared helper function.
+function assertRedirectResponse({ data, res, redirected, url, requestUrls }, expectedUrl) { + assert.equal(res.statusCode, 200); + assert(data.length > 100); + assert.equal(redirected, true); + assert.equal(url, expectedUrl); + assert.equal(requestUrls.length, 2); +}
Commented tests can be safely removed
The commented-out tests are obsolete and can be safely removed because:
- The active test suite already has comprehensive coverage for redirect scenarios including:
- Basic redirects with location headers
- Full URL redirects
- Different HTTP status codes (301, 302)
- Redirect limits (maxRedirects)
- Redirect with streams
- Disable auto-redirect functionality
The commented tests were testing external domain redirects and URL formatting which are:
- Not recommended for unit tests as they depend on external services (npmjs.com)
- The
formatRedirectUrloption still exists in the codebase (src/Request.ts) but should be tested with local endpoints instead🔗 Analysis chain
Line range hint
171-208: Clarify status of commented-out testsThese tests cover important redirect scenarios (cross-domain and URL formatting). Please clarify if they should be updated and re-enabled with the new undici implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if these scenarios are covered elsewhere # Search for cross-domain redirect tests rg -l "cross.*domain.*redirect|other.*domain.*redirect" # Search for URL formatting in redirects ast-grep --pattern 'formatRedirectUrl'Length of output: 96
Script:
#!/bin/bash # Let's try a broader search for redirect-related tests and functionality # Search for redirect-related test files fd "redirect.*test" # Search for redirect-related test cases rg -A 5 "\.it.*redirect" # Search for formatRedirectUrl usage in the codebase rg "formatRedirectUrl" # Search for any URL formatting related code in tests rg -A 3 "format.*url" test/Length of output: 498
Script:
#!/bin/bash # Let's examine the active redirect tests and implementation # Check the content of the redirect test file cat test/options.followRedirect.test.ts # Look for redirect implementation in the source rg -A 10 "followRedirect" src/Length of output: 8399
src/HttpClient.ts (1)
72-73: Consider adding a deprecation notice for UnidiciTimingInfo.The backward compatibility interface is good, but it would be helpful to mark it as deprecated to encourage usage of the correctly spelled interface.
-export interface UnidiciTimingInfo extends UndiciTimingInfo {} +/** @deprecated Use UndiciTimingInfo instead. Will be removed in the next major version. */ +export interface UnidiciTimingInfo extends UndiciTimingInfo {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
package.json(3 hunks)scripts/replace_urllib_version.js(1 hunks)src/HttpClient.ts(15 hunks)test/options.followRedirect.test.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🔇 Additional comments (6)
scripts/replace_urllib_version.js (1)
16-18: 🛠️ Refactor suggestion
Consider using a more specific regex pattern and add error handling.
The current regex pattern = 'VERSION'; might be too permissive and could match unintended assignments. Additionally, the script would benefit from proper error handling and version validation.
Consider applying these improvements:
- const newContent = content.replace(/ = 'VERSION';/, match => {
- const after = ` = '${pkg.version}';`;
+ // Validate version format
+ if (!/^\d+\.\d+\.\d+(-[a-zA-Z0-9.]+)?$/.test(pkg.version)) {
+ throw new Error(`Invalid version format: ${pkg.version}`);
+ }
+
+ // More specific regex patterns for both cases
+ const patterns = [
+ /(const VERSION = )'VERSION';/,
+ /(exports\.VERSION = )'VERSION';/
+ ];
+
+ let newContent = content;
+ for (const pattern of patterns) {
+ newContent = newContent.replace(pattern, (match, prefix) => {
+ const after = `${prefix}'${pkg.version}';`;Additional recommendations:
- Add try-catch blocks around file operations
- Add logging for when no replacements were needed
- Consider adding a dry-run mode for testing
Let's verify the current version string occurrences:
test/options.followRedirect.test.ts (3)
2-8: LGTM: Setup changes for stream handling
The addition of file stream handling setup is well-structured and properly cleaned up in the afterAll hook.
Also applies to: 12-13, 19-21, 26-26
108-120: Verify redirect handling reliability
This test was previously failing with a 301 status code instead of 200. Ensure that the undici v7 upgrade properly handles redirects.
42-56: Verify memory usage with allowH2 enabled
Given the PR's focus on fixing OOM issues with allowH2, this test case should be monitored for memory consumption during execution.
src/HttpClient.ts (2)
716-717: Fix format specifiers in debug statement.
The debug statement uses incorrect format specifiers. Use %o for objects and %d for numbers to match the argument types.
488-507:
Add error handling for file operations.
The file handling implementation lacks try-catch blocks for potential file system errors and stream validation.
Apply this diff to add error handling:
if (typeof file === 'string') {
+ try {
fileName = basename(file);
value = createReadStream(file);
+ } catch (err) {
+ throw new Error(`Failed to read file ${file}: ${err.message}`);
+ }
} else if (Buffer.isBuffer(file)) {
fileName = customFileName || `bufferfile${index}`;
value = file;
} else if (file instanceof Readable || isReadable(file as any)) {
fileName = getFileName(file) || customFileName || `streamfile${index}`;
isStreamingRequest = true;
+ if (!file.readable) {
+ throw new Error(`Stream ${fileName} is not readable`);
+ }
value = file;
}Likely invalid or redundant comment.
|
@coderabbitai 提一个 issue 将 form-data/form-data#571 修复 pick 到 urllib v3 分支 |
|
Tip For best results, initiate chat on the files or code changes. @fengmk2，已为您创建了一个 Issue，将 form-data/form-data#571 的修复合并到 urllib v3 分支。 Issue 链接：#548 🔗 Analysis chain@fengmk2，好的，我会创建一个 issue 来将 form-data/form-data#571 的修复合并到 urllib v3 分支。 🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 509 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
test/options.timeout.test.ts (1)
43-48: Consider adding memory leak detection testsGiven that this PR addresses OOM issues with
allowH2, consider adding tests that monitor memory usage patterns during HTTP/2 operations, particularly for long-running connections or multiple concurrent requests.Would you like help creating memory leak detection tests using Node.js's
process.memoryUsage()or the Heap Profiler?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/HttpClient.ts(14 hunks)test/options.timeout.test.ts(2 hunks)
🔇 Additional comments (6)
test/options.timeout.test.ts (2)
2-6: LGTM: Appropriate imports for HTTP/2 testing
The new imports are well-chosen for testing HTTP/2 functionality, particularly the secure server creation which is required for H2 support.
60-74: Verify timeout behavior consistency with undici v7
The test assertions for timeout errors should be verified against undici v7's documented behavior to ensure we're testing the correct error patterns.
✅ Verification successful
Timeout error patterns are correctly aligned with undici's behavior
The test assertions for HTTP/2 timeout errors are consistent with undici's error patterns:
UND_ERR_INFOis correctly used for HTTP/2 stream timeouts- Error structure matches with proper name, message, and code properties
- Response properties (status, rt) are appropriately verified
The test file also correctly covers other timeout scenarios:
- Headers timeout (
UND_ERR_HEADERS_TIMEOUT) - Body timeout (
UND_ERR_BODY_TIMEOUT) - General request timeouts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if timeout error patterns match undici v7 documentation
rg -A 5 "timeout.*error" node_modules/undici/docs/
rg -A 5 "UND_ERR_(INFO|HEADERS_TIMEOUT)" node_modules/undici/lib/
Length of output: 251
Script:
#!/bin/bash
# Let's check the actual code for timeout error patterns
rg -A 5 "timeout.*error" .
rg -A 5 "UND_ERR_(INFO|HEADERS_TIMEOUT)"
# Also check for any timeout-related test cases
rg -A 5 "timeout.*test" .
Length of output: 3070
Script:
#!/bin/bash
# Let's examine the full timeout test file to understand the context better
cat test/options.timeout.test.ts
Length of output: 4283
src/HttpClient.ts (4)
488-507: Improve file upload error handling
The file upload implementation lacks error handling for invalid file paths or unreadable streams.
571-572: Add request logging sanitization
The debug logging of headers might expose sensitive information. Consider sanitizing sensitive headers (e.g., Authorization, Cookie) before logging.
613-624: Add URL validation for redirects
The redirect handling should validate the redirect URL to prevent open redirect vulnerabilities. Add validation before following redirects.
716-717: Fix format specifiers in debug statement
The debug statement uses incorrect format specifiers. Use %o for objects and %d for numbers to match the argument types.
Apply this diff to correct the format specifiers:
-debug('Request#%d throw error: %s, socketErrorRetry: %s, socketErrorRetries: %s',
+debug('Request#%d throw error: %o, socketErrorRetry: %d, socketErrorRetries: %d',
requestId, rawError, args.socketErrorRetry, requestContext.socketErrorRetries);
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/HttpClient.ts (1)
139-140: Consider limiting history size for memory optimization.The request history array could grow large with many redirects. Consider adding a maximum size limit or implementing a circular buffer.
Apply this diff to limit history size:
requestContext = { retries: 0, socketErrorRetries: 0, redirects: 0, - history: [], + history: { + urls: [], + maxSize: 100, + push(url: string) { + if (this.urls.length >= this.maxSize) { + this.urls.shift(); + } + this.urls.push(url); + } + }, ...requestContext, };Also applies to: 288-289
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/HttpClient.ts(14 hunks)
🔇 Additional comments (3)
src/HttpClient.ts (3)
Line range hint 51-73: LGTM: Interface renaming with backward compatibility.
The renaming of UnidiciTimingInfo to UndiciTimingInfo improves naming consistency. The backward compatibility is maintained through interface extension.
716-717: Fix format specifiers in debug statement.
The debug statement uses incorrect format specifiers. Use %o for objects and %d for numbers.
488-509: 🛠️ Refactor suggestion
Improve file upload error handling.
The file upload implementation should include error handling for invalid file paths or unreadable streams.
Apply this diff to add error handling:
if (typeof file === 'string') {
+ try {
fileName = basename(file);
value = createReadStream(file);
+ } catch (err) {
+ throw new Error(`Failed to read file ${file}: ${err.message}`);
+ }
} else if (Buffer.isBuffer(file)) {
fileName = customFileName || `bufferfile${index}`;
value = file;
} else if (file instanceof Readable || isReadable(file as any)) {
fileName = getFileName(file) || customFileName || `streamfile${index}`;
isStreamingRequest = true;
+ if (!file.readable) {
+ throw new Error(`Stream ${fileName} is not readable`);
+ }
value = file;
}Likely invalid or redundant comment.
[skip ci] ## [4.5.0](v4.4.0...v4.5.0) (2024-11-30) ### Features * upgrade to undici v7 ([#547](#547)) ([9803c4e](9803c4e))
fix server side close unexpected exit
closes #541
Summary by CodeRabbit
Release Notes
New Features
FormDataclass to handle file uploads with non-ASCII filenames.HttpClientwith improved handling of connection closures and multiple concurrent requests.Bug Fixes
Documentation
Refactor