feat: support allowH2 on urllib@4 #5356
Conversation
[skip ci]
跟英文版同步，去掉误被提升到标题的注释。 [skip ci]
|
Warning Rate limit exceeded@fengmk2 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 42 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces enhancements to the HTTP client configuration and functionality within the application. Key changes include the addition of the Changes
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 (
|
|
debug 日志可以看到是开启了 H2 |
fengmk2
force-pushed
the
enableH2-use-undici-v6
branch
from
12a6f95 to
6a67a1d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- config/config.default.js (2 hunks)
- lib/core/httpclient4.js (1 hunks)
- lib/egg.js (3 hunks)
- package.json (1 hunks)
- test/fixtures/apps/httpclient-allowH2/app.js (1 hunks)
- test/fixtures/apps/httpclient-allowH2/config/config.default.js (1 hunks)
- test/fixtures/apps/httpclient-allowH2/package.json (1 hunks)
- test/lib/core/httpclient.test.js (1 hunks)
Files skipped from review due to trivial changes (1)
- test/fixtures/apps/httpclient-allowH2/package.json
Additional context used
Biome
lib/core/httpclient4.js
[error] 18-18: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Additional comments not posted (15)
test/fixtures/apps/httpclient-allowH2/config/config.default.js (1)
1-2: Enabling HTTP/2 support is a good performance optimization.Setting
allowH2totrueenables HTTP/2 support for the HTTP client, which can improve performance when communicating with servers that support HTTP/2.test/fixtures/apps/httpclient-allowH2/app.js (3)
4-17: LGTM!The
CustomHttpClientclass implementation looks good:
- It extends the existing
HttpClient4class to add custom functionality.- The
requestmethod adds URL validation to ensure the URL starts with "http", enhancing the robustness of HTTP requests.- The
curlmethod is a wrapper around therequestmethod, maintaining compatibility with existing usage patterns.
5-12: LGTM!The
requestmethod implementation looks good:
- It uses a Promise to perform the URL validation asynchronously, without blocking the main execution thread.
- The validation checks if the URL starts with "http" using a regular expression, ensuring only valid URLs are allowed.
- If the validation fails, it throws an assertion error with a descriptive message, providing clear feedback.
- If the validation passes, it calls the superclass
requestmethod to continue the HTTP request process, maintaining the original functionality.
14-16: LGTM!The
curlmethod implementation looks good:
- It is a simple wrapper around the
requestmethod, delegating the functionality to the underlying implementation.- It takes the same parameters as the
requestmethod and passes them through, ensuring compatibility with existing code.- It does not add any additional functionality or modify the behavior of the
requestmethod, maintaining the expected behavior.lib/core/httpclient4.js (4)
4-29: LGTM!The
HttpClient4class is well-designed and enhances the functionality of theHttpClientfromurllib4. It properly normalizes the configuration settings in the constructor and adds tracing support in therequestmethod. Thecurlmethod is a nice addition to maintain compatibility with existing code.Tools
Biome
[error] 18-18: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
16-24: LGTM!The
requestmethod correctly checks for the presence of a tracer in the provided context and falls back to the application instance tracer if no context tracer is found. The current implementation is clear and concise.Regarding the static analysis hint:
The suggestion to use an optional chain is not necessary in this case, as the current implementation is clear and concise. The condition is straightforward and easy to understand, so changing it to an optional chain would not provide any significant benefit.Tools
Biome
[error] 18-18: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
26-28: LGTM!The
curlmethod is a simple wrapper around therequestmethod, which maintains compatibility with existing code that may usecurlfor making requests. It does not add any additional functionality or modify the arguments, making it a straightforward and useful addition to the class.
31-36: LGTM!The
normalizeConfigfunction is a useful utility that ensures thetimeoutconfiguration is properly converted from a string to a numeric value. It uses the reliablehumanize-mslibrary to handle the conversion, enhancing the robustness of the configuration handling.package.json (1)
58-58: Verify compatibility withurllib@4.The PR adds a new dependency
urllib4which is an alias for[email protected]. However, the existing codebase also depends on older versions ofurllib(^2.33.0and^3.22.4).Please ensure that the existing code is compatible with
urllib@4and won't break due to any API changes.Run the following script to verify the
urllibimports in the codebase:Verification successful
Gradual migration to urllib4 confirmed. No immediate compatibility issues detected.
The codebase is implementing a gradual migration strategy from urllib2/3 to urllib4. The new
HttpClient4class inlib/core/httpclient4.jsutilizes urllib4 and supports theallowH2feature mentioned in the PR objectives. The existingHttpClientclass is retained for backwards compatibility.Key observations:
- Multiple HttpClient classes coexist (
HttpClient,HttpClientNext,HttpClient4).- No breaking changes in basic API usage (
request,curl) were detected.- The gradual approach allows for introducing new features while maintaining compatibility.
This strategy minimizes the risk of breaking changes and allows for a smoother transition to urllib4.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify urllib imports in the codebase. # Test: Search for urllib imports. Expect: Imports from urllib4 or imports without version. rg --type js -o $'urllib(4)?'Length of output: 761
Script:
#!/bin/bash # Description: Examine urllib usage in key files # Check httpclient4.js echo "=== Content of lib/core/httpclient4.js ===" cat lib/core/httpclient4.js # Check httpclient.js echo -e "\n=== Content of lib/core/httpclient.js ===" cat lib/core/httpclient.js # Check egg.js (first 50 lines) echo -e "\n=== First 50 lines of lib/egg.js ===" head -n 50 lib/egg.js # Check for version-specific API calls echo -e "\n=== Searching for potential version-specific API calls ===" rg --type js 'urllib\.(request|curl|requestThunk|request2|curl2)' -C 2Length of output: 8119
config/config.default.js (2)
306-307: LGTM!The change improves the clarity of the configuration by explicitly setting the default value of
useHttpClientNexttofalse.
330-330: Approve the addition of theallowH2configuration option.The change introduces a new configuration option to enable HTTP/2 support using the urllib@4 HttpClient, which aligns with the PR objective. The default value is set to
false, making it an opt-in feature that requires explicit enabling.Enabling HTTP/2 support can potentially improve performance for certain types of requests and network conditions. However, it's important to ensure that the underlying infrastructure and dependencies support HTTP/2 before enabling this feature in production environments.
The opt-in nature of the feature mitigates the risk of unexpected behavior for existing applications.
lib/egg.js (3)
17-17: LGTM!The import statement for
HttpClient4is necessary and correctly added.
55-55: LGTM!The assignment of
HttpClient4tothis.HttpClient4is correctly added in the constructor of theEggApplicationclass.
298-300: Verify the new condition for instantiatingHttpClient4.The new condition that checks
this.config.httpclient.allowH2and instantiatesHttpClient4if true is correctly added. However, ensure that this change is thoroughly tested to confirm that it behaves as expected and does not introduce any unexpected behavior.Run the following script to verify the usage of the new condition:
Verification successful
Verified: New condition for
HttpClient4is correctly implementedThe new condition
this.config.httpclient.allowH2is correctly implemented inlib/egg.jsand is the only occurrence in the codebase. The order of conditions (allowH2 -> useHttpClientNext -> enableDNSCache) ensures the intended behavior for selecting the appropriate HTTP client.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new condition for instantiating `HttpClient4`. # Test: Search for the usage of `this.config.httpclient.allowH2`. Expect: Only occurrences in the new condition. rg --type js -A 5 $'this\.config\.httpclient\.allowH2'Length of output: 454
test/lib/core/httpclient.test.js (1)
258-304: LGTM!The new test suite for the HTTP client functionality with
allowH2set to true is comprehensive and well-structured. It covers critical aspects such as global timeout settings, successful HTTP/1.1 and HTTP/2 requests, and URL validation.The test cases help ensure the correctness and reliability of the HTTP client when
allowH2is enabled. They enhance the overall testing coverage and provide confidence in the HTTP client's behavior.Great job on adding this test suite!
Comments failed to post (2)
test/fixtures/apps/httpclient-allowH2/config/config.default.js (1)
3-5: Increase the timeout value to avoid premature request failures.
The
timeoutvalue of99milliseconds is extremely short and might lead to premature request failures, especially in high-latency environments or for requests that take longer to process.Consider increasing the timeout to a more reasonable value, such as 5000 milliseconds (5 seconds), to allow sufficient time for requests to complete:
exports.httpclient = { allowH2: true, request: { - timeout: 99, + timeout: 5000, }, };Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.request: { timeout: 5000, },package.json (1)
58-58: Consider removing older versions of
urllib.The project now has 3 different versions of
urllibspecified inpackage.json:
urllib@^2.33.0urllib-nextaliased tourllib@^3.22.4urllib4aliased tourllib@^4.3.0Having multiple versions of the same dependency can lead to inconsistencies and maintenance overhead. If the older versions are no longer needed, consider removing them.
Apply this diff to remove the older versions:
- "urllib": "^2.33.0", - "urllib-next": "npm:urllib@^3.22.4",Committable suggestion was skipped due to low confidence.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5356 +/- ##
==========================================
- Coverage 99.86% 96.74% -3.12%
==========================================
Files 36 36
Lines 3628 3567 -61
Branches 520 502 -18
==========================================
- Hits 3623 3451 -172
- Misses 5 116 +111 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- .github/workflows/nodejs.yml (1 hunks)
- test/doc.test.js (1 hunks)
Files skipped from review due to trivial changes (1)
- test/doc.test.js
#5347
Summary by CodeRabbit
New Features
Bug Fixes
Tests