fix(ext/node): implement dns.lookupService
#31310
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements the dns.lookupService function for Node.js compatibility in Deno, enabling reverse DNS lookups to resolve IP addresses and ports to hostnames and service names.
- Adds
op_node_getnameinfooperation that wraps platform-specificgetnameinfosystem calls - Implements both callback-based and promise-based
lookupServiceAPIs in the dns module - Enables two Node.js compatibility tests for
dns.lookupService
Reviewed Changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ext/node/ops/dns.rs | Implements op_node_getnameinfo with platform-specific getnameinfo calls for Unix and Windows |
| ext/node/ops/constant.rs | Adds UV error code constants for DNS address info errors |
| ext/node/polyfills/internal_binding/cares_wrap.ts | Adds GetNameInfoReqWrap class and getnameinfo function to wrap the native operation |
| ext/node/polyfills/internal/errors.ts | Adds handleDnsError helper function for DNS error processing |
| ext/node/polyfills/internal/dns/promises.ts | Implements promise-based lookupService function |
| ext/node/polyfills/dns.ts | Implements callback-based lookupService function and exports |
| ext/node/polyfills/dns/promises.ts | Re-exports lookupService from promises module |
| ext/node/lib.rs | Registers the new op_node_getnameinfo operation |
| ext/node/Cargo.toml | Adds socket2 dependency for socket address handling |
| Cargo.lock | Updates lock file with socket2 dependency |
| tests/unit_node/dns_test.ts | Adds unit tests for lookupService with callbacks, promises, and error cases |
| tests/integration/node_unit_tests.rs | Registers the new dns_test module |
| tests/node_compat/config.toml | Enables two Node.js compatibility tests for lookupService |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
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. WalkthroughThis PR adds reverse DNS lookup support (lookupService) for Node.js compatibility. It introduces a new core op ( Sequence Diagram(s)sequenceDiagram
participant App as User App
participant API as dns.lookupService / promises.lookupService
participant Poly as cares_wrap / polyfills
participant Core as op_node_getnameinfo
participant OS as Platform getnameinfo
App->>API: call lookupService(address, port, [callback])
API->>API: validate args, permissions, port
API->>Poly: create GetNameInfoReqWrap / promise
Poly->>Core: invoke op_node_getnameinfo(address, port)
Core->>OS: platform-specific getnameinfo (Unix/Windows)
OS-->>Core: (hostname, service) or error
Core-->>Poly: complete with result or DnsError
Poly->>API: resolve/reject promise or call callback
API->>App: return result or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (1)ext/node/ops/dns.rs (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
🔇 Additional comments (3)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ext/node/polyfills/dns.ts (1)
328-361: Consider adding explicit string validation and JSDoc comments.The implementation is functionally correct, but could be improved:
String validation: While
isIP()handles non-strings by returning 0, explicitvalidateString(address, "address")before Line 341 would provide clearer error messages and match the pattern used in thelookup()function.Missing documentation: Unlike other exported functions in this file (e.g.,
lookup,resolve4),lookupServicelacks JSDoc comments. Adding documentation would help users understand the API, especially the callback signature and error handling.Example addition before Line 337:
+/** + * Resolves the given address and port into a hostname and service using the + * operating system's underlying `getnameinfo` implementation. + * + * @param address A string representing an IPv4 or IPv6 address. + * @param port The numeric port number. + * @param callback The callback function. + */ export function lookupService(Example addition after Line 338:
if (arguments.length !== 3) { throw new ERR_MISSING_ARGS("address", "port", "callback"); } + + validateString(address, "address");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
ext/node/Cargo.toml(1 hunks)ext/node/lib.rs(1 hunks)ext/node/ops/constant.rs(1 hunks)ext/node/ops/dns.rs(3 hunks)ext/node/polyfills/dns.ts(5 hunks)ext/node/polyfills/dns/promises.ts(1 hunks)ext/node/polyfills/internal/dns/promises.ts(4 hunks)ext/node/polyfills/internal/errors.ts(1 hunks)ext/node/polyfills/internal_binding/cares_wrap.ts(3 hunks)tests/integration/node_unit_tests.rs(1 hunks)tests/node_compat/config.toml(1 hunks)tests/unit_node/dns_test.ts(1 hunks)
🔇 Additional comments (19)
ext/node/Cargo.toml (1)
91-91: socket2 dependency wiring looks correctAdding
socket2.workspace = trueis consistent with the new DNS/socket plumbing and matches the existing pattern of workspace-scoped dependencies. No issues from this manifest change alone.ext/node/lib.rs (1)
349-351: New DNS getnameinfo op is correctly exposed
ops::dns::op_node_getnameinfo<P>is wired into the extension ops list next toop_node_getaddrinfo<P>and uses the same permission generic, which is the expected pattern for new DNS ops.tests/node_compat/config.toml (1)
336-337: Enabling Node’s lookupService compat tests is appropriateRegistering
parallel/test-dns-lookupService.jsandparallel/test-dns-lookupService-promises.jshere cleanly plugs the new implementation into the Node-compat test matrix and follows the existing config style.tests/integration/node_unit_tests.rs (1)
74-74: dns_test is correctly hooked into the node_unit_test harnessAdding
dns_testto theunit_test_factory!list correctly wires the new DNS unit tests into the existing integration runner with consistent naming.ext/node/polyfills/dns/promises.ts (1)
23-27: lookupService correctly added to dns promises named exportsIncluding
lookupServicein the destructured export frompromisesalignsnode:dns/promiseswith Node’s public API so that both default and named imports are supported.tests/unit_node/dns_test.ts (2)
17-56: Good coverage for callback and promise lookupService success pathsThese tests exercise both named and default imports of
lookupServicefromnode:dnsandnode:dns/promises, and validate that hostname/service come back as strings, which is exactly what we need for basic behavior verification across both APIs.
58-89: ENOTFOUND behavior is well specified for lookupServiceThe not-found test checks that both promise- and callback-based
lookupServicereport an ENOTFOUND error with the expectedmessage,code, andsyscallfields, which is important for Node-compat error semantics. This should catch regressions in DNS error mapping.ext/node/polyfills/internal/dns/promises.ts (1)
26-32: lookupService promise implementation and wiring look Node-compatibleThe new
lookupServiceflow in the promises layer looks solid:
- Imports are extended appropriately (
validatePort,ERR_MISSING_ARGS,handleDnsError,GetNameInfoReqWrap) to support argument validation and DNS-specific error shaping.onlookupserviceuseshandleDnsError(err, "getnameinfo", this.address)for async failures and resolves with{ hostname, service }on success, matching the expected promise result shape.createLookupServicePromisemirrors the existingcreateLookupPromisepattern, withGetNameInfoReqWrapcarryingaddress,port, and the completion handler, and usesdnsExceptionfor synchronouscares.getnameinfoerrors.lookupService(address, port)enforces arity, ensuresaddressis an IP viaisIP, validatesportwithvalidatePort, and returns the promise fromcreateLookupServicePromise, which is in line with Node’s internal implementation.- Adding
lookupServiceto the default export object keeps the publicdnsPromisessurface coherent with the new functionality.Overall, this is a clean, idiomatic extension of the existing DNS promises machinery.
Also applies to: 53-58, 59-65, 237-281, 551-553
ext/node/polyfills/internal_binding/cares_wrap.ts (2)
141-161: LGTM! GetNameInfoReqWrap class structure is consistent.The class follows the same pattern as
GetAddrInfoReqWrapand has appropriate type signatures for DNS reverse lookup operations.
163-177: LGTM! The getnameinfo function implementation is correct.The async operation pattern matches other functions in this file, and error handling is appropriate.
ext/node/polyfills/internal/errors.ts (1)
334-348: LGTM! DNS error handling logic is comprehensive.The three-tier error handling (UV error codes → NotCapable → generic errors) properly routes DNS-related errors to appropriate constructors.
ext/node/ops/constant.rs (1)
121-135: LGTM! UV error constants are correctly defined.The constants match libuv's DNS error codes and include all values referenced in the getnameinfo implementation.
ext/node/polyfills/dns.ts (2)
315-326: LGTM! The onlookupservice callback handler is correct.The error handling via
handleDnsErrorand success path are properly implemented, following the same pattern as other callback handlers in this file.
363-367: LGTM! Promisify configuration is correct.The
customPromisifyArgsproperty is properly configured to shape promisified results as{ hostname, service }, matching the callback signature.ext/node/ops/dns.rs (5)
23-57: LGTM! DnsError enum is well-designed.The expanded error type properly handles various DNS error scenarios with appropriate error inheritance and UV error code exposure via the
uv_errcodeproperty.
94-118: LGTM! The op_node_getnameinfo operation is correctly implemented.The operation properly handles permissions, input validation, and uses
spawn_blockingfor the blockinggetnameinfocall, which prevents blocking the async runtime.
124-155: LGTM! Unix getnameinfo implementation is correct.The implementation properly uses
libc::getnameinfowith appropriate buffer sizes and theNI_NAMEREQDflag for hostname resolution.
160-167: Verify type compatibility when mixing winapi and windows_sys crates.The code uses
MAKEWORDfrom thewinapicrate (Line 160) butWSAStartupfrom thewindows_syscrate (Line 161). This mixing was previously flagged as a potential type incompatibility issue.Consider using
windows_sysconsistently for all Windows socket operations:- use winapi::shared::minwindef::MAKEWORD; use windows_sys::Win32::Networking::WinSock; // SAFETY: winapi call let wsa_startup_code = *WINSOCKET_INIT.get_or_init(|| unsafe { let mut wsa_data: WinSock::WSADATA = std::mem::zeroed(); - WinSock::WSAStartup(MAKEWORD(2, 2), &mut wsa_data) + WinSock::WSAStartup((2u16 | (2u16 << 8)), &mut wsa_data) });Alternatively, if
winapimust be used, ensure all Windows socket types come from the same crate to avoid ABI issues.Based on learnings
206-248: LGTM! Platform-specific error mapping is comprehensive.The
assert_successfunction properly maps platform-specific DNS error codes to UV error codes for both Unix and Windows, with appropriate fallbacks for unmapped errors.
| const errCode = cares.getnameinfo(req, address, port); | ||
|
|
||
| if (errCode) { | ||
| reject(dnsException(errCode, "getnameinfo", address)); | ||
| } |
There was a problem hiding this comment.
Our cares.getnameinfo will always return 0, but I use this approach because a test on parallel/test-dns-lookupService.js stubs the return value of this function: https://github.com/nodejs/node/blob/591ba692bfe30408e6a67397e7d18bfa1b9c3561/test/parallel/test-dns-lookupService.js#L9-L10
| "parallel/test-dns-lookupService.js" = {} | ||
| "parallel/test-dns-lookupService-promises.js" = {} |
There was a problem hiding this comment.
Nice work 👍 I just hope they won't be flaky 💀
2 participants
Closes #26300
Allows the following node compatibility tests to pass: