feat: upgrade dnscache_httpclient and enable dnscache_fetch#5731
feat: upgrade dnscache_httpclient and enable dnscache_fetch#5731
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @Dipper30, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades Egg.js's network request capabilities by enhancing its DNS caching mechanism and integrating the modern Fetch API. The core change involves a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements to Egg.js by upgrading the DNS caching capabilities and integrating the fetch API. The new DNSCacheResolver provides a more robust DNS caching mechanism with support for dns.resolve, TTL respect, and IP address rotation, while maintaining backward compatibility. The addition of app.fetch, ctx.fetch, and their safeFetch variants is a great modernization step for the framework's HTTP client capabilities. The implementation is well-structured, and the new features are thoroughly tested. I've identified a potential bug in the new DNS resolver logic that could affect address rotation under specific conditions, and I've also suggested a couple of minor refactorings to improve code clarity and maintainability. Overall, this is a high-quality contribution that significantly improves Egg.js.
style: formatting code fix: dns cache record
Dipper30
force-pushed
the
feat/enable-dnscache-httpclient-and-fetch
branch
from
73c6e14 to
9002b83
Compare
There was a problem hiding this comment.
Pull request overview
This pull request introduces significant enhancements to Egg.js's DNS caching capabilities and adds new fetch API support. The changes enable a modern dns.resolve-based caching mode with TTL support and IP rotation, while maintaining backward compatibility with the existing dns.lookup mode. Additionally, it integrates fetch APIs (app.fetch, ctx.fetch, and their safe variants) that leverage the same DNS caching infrastructure.
Key Changes:
- Replaced the legacy DNSCacheHttpClient with a new unified DNSCacheResolver that supports both dns.lookup and dns.resolve modes with configurable TTL-based refresh and IP address rotation
- Added fetch API integration (app.fetch, app.safeFetch, ctx.fetch, ctx.safeFetch) with DNS caching support via urllib4's FetchFactory
- Comprehensive test coverage for both DNS resolver modes and fetch functionality
Reviewed changes
Copilot reviewed 40 out of 41 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/core/dnscache.js | New unified DNS cache resolver supporting both dns.lookup and dns.resolve modes with TTL and rotation |
| lib/core/dnscache_httpclient.js | Removed legacy DNS cache implementation in favor of the new resolver |
| lib/core/fetch_factory.js | Refactored to create fetch instances with configurable options and SSRF protection |
| lib/core/context_fetch.js | New context wrapper for fetch APIs providing ctx.fetch and ctx.safeFetch |
| lib/core/httpclient.js | Updated to use DNSCacheResolver's lookup function instead of direct DNS calls |
| lib/core/httpclient_next.js | Added DNS resolver integration and dnsCache getter |
| lib/egg.js | Added dnsResolver, fetch, and safeFetch getters with initialization logic |
| lib/core/utils.js | Added detectFetchVersion utility to check Node.js >= 20 |
| app/extend/context.js | Extended context with fetchClient, fetch, and safeFetch methods |
| config/config.default.js | Added new DNS resolver configuration options (useDNSResolver, dnsServers, dnsAddressRotation) |
| index.d.ts | Added TypeScript definitions for EggContextFetch interface |
| site/docs/core/httpclient.md | Updated documentation for new DNS resolver and fetch features |
| site/docs/core/httpclient.zh-CN.md | Updated Chinese documentation for new DNS resolver and fetch features |
| test/lib/core/fetch.test.js | Comprehensive test suite for new fetch APIs with DNS caching |
| test/lib/core/dnscache_resolver_httpclient.test.js | Test suite for dns.resolve mode with legacy httpclient |
| test/lib/core/dnscache_resolver_httpclient_next.test.js | Test suite for dns.resolve mode with httpclient_next |
| test/lib/core/dnscache_httpclient.test.js | Updated tests for dns.lookup mode with address rotation support |
| test/lib/core/dnscache_fetch.test.js | Test suite for fetch API with DNS caching |
| test/lib/core/fetch_factory.test.js | Removed old fetch factory tests |
| test/fixtures/apps/fetch/* | New test fixture app for fetch functionality testing |
| test/fixtures/apps/dnscache_resolver_httpclient/* | New test fixture app for dns.resolve mode testing |
| test/fixtures/apps/dnscache_resolver_httpclient_next/* | New test fixture app for dns.resolve mode with httpclient_next |
| test/fixtures/apps/dnscache_fetch/* | New test fixture app for fetch with DNS caching |
| test/fixtures/apps/dnscache_httpclient/config/config.default.js | Updated with adjusted timing and agent configuration |
| test/fixtures/apps/dnscache_httpclient/app/controller/home.js | Fixed boolean check for disableDNSCache parameter |
| dnsCacheLookupInterval: 10000, // will not work if useDNSResolver is true | ||
|
|
||
| // DNS resolver mode using dns.resolve | ||
| useDNSResolver: false, // will not work if enableDNSCache IS NOT true |
There was a problem hiding this comment.
The comment "will not work if enableDNSCache IS NOT true" uses all caps "IS NOT" which is unconventional. Standard practice is to use lowercase "is not" or rephrase as "only works when enableDNSCache is true" for better readability.
| assert(app.dnsResolver.getDnsCache().size === 0); | ||
|
|
||
| await app.httpRequest() | ||
| .get('/??disableDNSCache=false&url=' + encodeURIComponent(url + '/get_headers')) |
There was a problem hiding this comment.
There's a typo in the URL path - double question mark ('??') instead of single question mark ('?'). This will cause the request to fail as it's an invalid query string format.
| const data = await result.json(); | ||
| assert(/localhost:\d+/.test(data.host)); | ||
|
|
||
| utils.sleep(5000); |
There was a problem hiding this comment.
The await statement is missing here. The sleep function returns a Promise that should be awaited, otherwise the test will continue immediately without waiting, potentially causing timing-related test failures.
| it('disable DNSCache in one request SHOULD NOT work', async () => { | ||
| mockResolve4Promise(app); | ||
| await app.httpRequest() | ||
| .get('/?disableDNSCache=true&url=' + encodeURIComponent(url + '/get_headers')) | ||
| .expect(200) | ||
| .expect(/"host":"localhost:\d+"/); | ||
| assert(app.dnsResolver.getDnsCache().size === 1); | ||
|
|
||
| await app.httpRequest() | ||
| .get('/?disableDNSCache=false&url=' + encodeURIComponent(url + '/get_headers')) | ||
| .expect(200) | ||
| .expect(/"host":"localhost:\d+"/); | ||
| assert(app.dnsResolver.getDnsCache().size === 1); | ||
| }); |
There was a problem hiding this comment.
Inconsistent comparison between the two test files. In 'dnscache_resolver_httpclient_next.test.js' line 262, the test expects that disabling DNS cache "SHOULD NOT work" (cache size should be 1), whereas in 'dnscache_resolver_httpclient.test.js' line 260, the identical test expects that it "should work" (cache size should be 0). This suggests either a test logic error or a behavioral difference that isn't documented.
|
|
||
| /** | ||
| * This test failure can be totally ignored because it depends on how your service provider | ||
| * deals with the domain when you cannot find that：Some providers will batchly switch |
There was a problem hiding this comment.
The comment states "Some providers will batchly switch" but "batchly" is not a standard English word. The correct term should be "batch" (as a verb) or "in batches".
| // DNS 同时缓存的最大域名数量，默认 1000 | ||
| dnsCacheMaxLength: 1000, | ||
| // 旧的 dns.lookup 模式下的 DNS 缓存查询间隔，单位毫秒，默认 10000ms | ||
| dnsCacheLookupInterval: 10000, // 当 useDNSResolver 为 true 时不生效 |
There was a problem hiding this comment.
The comment "当 useDNSResolver 为 true 时不生效" (will not work when useDNSResolver is true) appears on line 283, but this should be clarified with better documentation. Consider adding why it doesn't work (because the new resolver mode uses TTL-based refresh instead of fixed intervals).
| // Verify cyclic pattern: after each request, index advances | ||
| // Starting from 0: after req1->1, req2->2, req3->0, req4->1, req5->2, req6->0 | ||
| assert.strictEqual(indices[0], 1); // after 1st request | ||
| assert.strictEqual(indices[1], 2); // after 2nd request | ||
| assert.strictEqual(indices[2], 0); // after 3rd request (wraps around) | ||
| assert.strictEqual(indices[3], 1); // after 4th request | ||
| assert.strictEqual(indices[4], 2); // after 5th request | ||
| assert.strictEqual(indices[5], 0); // after 6th request (wraps around again) |
There was a problem hiding this comment.
The array indices comparison is off by one. After the initial request that populates the cache, the first index is already at 0. The test expects indices[0] to be 1 (after 1st request), but indices[0] is captured before any request. The array should start from indices[1] = 1 (after 1st curl), indices[2] = 2 (after 2nd curl), etc. The current assertions don't match the actual rotation sequence.
| * @member Config#httpclient | ||
| * @property {Boolean} enableDNSCache - Enable DNS lookup from local cache or not, default is false. | ||
| * @property {Boolean} dnsCacheLookupInterval - minimum interval of DNS query on the same hostname (default 10s). | ||
| * @property {Boolean} enableDNSCache - Enable DNS lookup cache using dns.lookup (old behavior), default is false. |
There was a problem hiding this comment.
The documentation comment has incorrect grammar. "Enable DNS lookup cache using dns.lookup (old behavior)" should clarify what "old behavior" refers to - it's better to say "Enable DNS lookup cache (using dns.lookup, the legacy mode)" or "Enable DNS lookup cache using dns.lookup mode".
| dnsCacheLookupInterval: 10000, // will not work if useDNSResolver is true | ||
|
|
||
| // DNS resolver mode using dns.resolve (new feature) | ||
| useDNSResolver: false, // will not work if enableDNSCache IS NOT true |
There was a problem hiding this comment.
The error message uses "IS NOT" in all caps which is unconventional. Standard practice is to use lowercase "is not" or simply rephrase as "will not work when enableDNSCache is false" for better readability.
| dnsCacheLookupInterval: 10000, // 当 useDNSResolver 为 true 时不生效 | ||
|
|
||
| // 新特性：开启 DNS 解析器模式，使用 dns.resolve | ||
| useDNSResolver: false, // 当 enableDNSCache 不为 true 时不生效 |
There was a problem hiding this comment.
The comment "当 enableDNSCache 不为 true 时不生效" on line 286 uses a double negative ("not not true") which is confusing. It should be rephrased as "当 enableDNSCache 为 false 时不生效" or "仅在 enableDNSCache 为 true 时生效" (only works when enableDNSCache is true).
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
egg/lib/core/httpclient_next.js
Lines 67 to 69 in 9002b83
SafeCurl ignores configured DNS cache/resolver
The SSRF-safe client built in safeCurl only forwards { checkAddress } to the new HttpClientNext, so it never receives the custom lookup injected by createHttpClient when httpclient.enableDNSCache/useDNSResolver are enabled. Calls through app.httpclient.safeCurl therefore bypass the DNS cache and address rotation and fall back to the system resolver, diverging from normal curl behavior. The SSRF client should be created with the same lookup as the main client so DNS cache settings are honored.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (now - timestamp >= ttl) { | ||
| // refresh in background, keep serving cached value | ||
| this._updateDNS(hostname); |
There was a problem hiding this comment.
When a cached DNS entry has expired, _updateDNS(hostname) is fired in the background without awaiting or catching it. If that refresh fails (e.g., DNS server down), the rejected promise is never handled, which will emit unhandledRejection warnings and can terminate the process in environments using strict unhandled-rejection handling even though the cached record is still returned. The refresh promise should be caught/logged to avoid crashing on transient DNS failures.
Useful? React with 👍 / 👎.
|
@Dipper30 Thank you very much for your contribution to the egg open source project. I really like the DNSResolver feature you created. Once this |
killagu
left a comment
killagu
left a comment
There was a problem hiding this comment.
单测里需要补充一下 dnscache 单独的单测。
| ctx: Context; | ||
| app: Application; |
| * @param {Boolean} options.addressRotation - Enable address rotation for both lookup and resolve modes, default is true | ||
| */ | ||
| constructor(options = {}) { | ||
| this.maxCacheSize = options.max || 1000; |
There was a problem hiding this comment.
maxCacheSize/max 字段是否对齐更好。
| this._updateDNS(hostname) | ||
| .then(record => { | ||
| this._callbackWithRecord(record, options, callback); | ||
| }) | ||
| .catch(err => { | ||
| callback(err); | ||
| }); |
There was a problem hiding this comment.
callback 里套一个 promise 看着很奇怪。建议拆开来。
getLookupFunction
async lookup() {
// ...
}
lookupWithCallback(..., callback) {
this.lookup(...)
.then(record => callback(null, ...))
.catch(callback)
}| } | ||
|
|
||
| // Should not reach here, all records should use records structure | ||
| throw new Error('[dns_cache_error]: Invalid cache record structure'); |
There was a problem hiding this comment.
callback 风格不能 throw，要用 callback 丢回去。
| */ | ||
| _callbackWithRecord(record, options, callback) { | ||
| // All records use the unified structure with rotation | ||
| if (record.records && Array.isArray(record.records)) { |
There was a problem hiding this comment.
这里分支的原因是因为有不同的 options？如果上层同一个 host lookup 的参数不一样会不会有问题？
| return this.httpclient; | ||
| } | ||
|
|
||
| get dnsResolver() { |
There was a problem hiding this comment.
感谢review，侵入egg代码的部分会移除，我会重开一个pr仅新增 lookup 函数传参，dnscache逻辑交给插件自己实现
Enhances Egg.js DNS caching (dnscache) with a new dns.resolve-based mode that respects TTL and supports IP rotation, while maintaining full backward compatibility with the existing dns.lookup mode.
Also adds app.fetch, app.safeFetch, ctx.fetch, and ctx.safeFetch—all integrated with the same dnscache logic as httpclient.
Test cases added.