-
Notifications
You must be signed in to change notification settings - Fork 29.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dns: add promisified dns module #21264
Conversation
ideally we shouldn't be using those promise utils, they can't be optimised by v8 also should be looped into the new module naming thing as this can't be used first-class by esm |
OK, got rid of those. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once tests and documentation are added :)
26d49ec
to
07345fe
Compare
The full API has been ported, and docs+tests added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a sibling of the Resolver
class, with Resolver.prototype.promises
as its prototype?
CI run: https://ci.nodejs.org/job/node-test-pull-request/15424/
Do you mean like |
@cjihrig Yes, basically – I’d call it |
@addaleax I added a |
req.hostname = hostname; | ||
req.oncomplete = all ? onlookupall : onlookup; | ||
req.resolve = resolve; | ||
req.reject = reject; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now we seem to have two implicit "subclasses" of GetNameInfoReqWrap
: one having a .callback
property and is used for dns
, while the other has .resolve
and .reject
properties. I would rather make this distinction explicit by having a GetAddrInfoPromiseReqWrap
subclass that extends GetAddrInfoReqWrap
.
Same for the other wrapper classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TimothyGu I was aiming to avoid a lot of unnecessary duplicate code. The wraps behave exactly the same in the C++ layer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cjihrig Yeah I understand. What I'm trying to say is that you can have a JavaScript class
that extends GetAddrInfoReqWrap
. As long as its constructor calls super()
properly it should work just fine. No duplication in C++ needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TimothyGu It still seems a bit complex if we hang all the data (host
, port
, bindingName
, etc.) onto a req wrap with C++ binding even though the C++ binding may not necessarily read those properties from the req wrap. The only property that matters to C++ should be wrap.oncomplete
, so it may be cleaner if we create two JS classes, one for the callback and the other one for promises, each with a .handle
(or a symbol property) pointing to those req wraps and add the data properties to the JS instances of those classes. Then the main difference between those two classes would just be how we call resolve
/reject
/callback(null, result)
/callback(dnsException(...))
in handle.oncomplete
. (That's basically what the two types of wraps do in the fs promises implementation although those are implemented in C++)
(It's a bit unfortunate that the resolves have a ChannelWrap
that's not req wrap as _handle
though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a second thought, it may not be feasible to separate the data properties to an outer object since we have been allowing users to retrieve them via the this
bound to the callback...
EDIT: but we do not always bind the callback to the req wrap e.g. dns.lookup('127.0.0.1', function() { console.log(this) }
gives you the global.
Green CI run: https://ci.nodejs.org/job/node-test-pull-request/15481/ |
doc/api/dns.md
Outdated
--> | ||
|
||
Cancel all outstanding DNS queries made by this resolver. The corresponding | ||
callbacks will be called with an error with code `ECANCELLED`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it say something like "the corresponding promises will be rejected with..."?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some doc nits.
doc/api/dns.md
Outdated
that return `Promise` objects rather than using callbacks. The API is accessible | ||
via `require('dns').promises`. | ||
|
||
## Class: dnsPromises.Resolver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this should be ###
level, not ##
, to reflect the hierarchy of not-promisified API.
doc/api/dns.md
Outdated
* [`resolver.resolveTxt()`][`dnsPromises.resolveTxt()`] | ||
* [`resolver.reverse()`][`dnsPromises.reverse()`] | ||
|
||
### resolver.cancel() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this should be ####
level, not ###
, to reflect the hierarchy of not-promisified API.
|
||
The following methods from the `dnsPromises` API are available: | ||
|
||
* [`resolver.getServers()`][`dnsPromises.getServers()`] |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The following methods from the `dnsPromises` API are available: | ||
|
||
* [`resolver.getServers()`][`dnsPromises.getServers()`] | ||
* [`resolver.setServers()`][`dnsPromises.setServers()`] |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
doc/api/dns.md
Outdated
--> | ||
|
||
Cancel all outstanding DNS queries made by this resolver. The corresponding | ||
`Promises` will be rejected with an error with code `ECANCELLED`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`Promises`
-> `Promise`s
?
doc/api/dns.md
Outdated
`{ address: '1.2.3.4', ttl: 60 }` objects rather than an array of strings, | ||
with the TTL expressed in seconds. | ||
|
||
Uses the DNS protocol to resolve a IPv4 addresses (`A` records) for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a IPv4 addresses -> IPv4 addresses
doc/api/dns.md
Outdated
`{ address: '0:1:2:3:4:5:6:7', ttl: 60 }` objects rather than an array of | ||
strings, with the TTL expressed in seconds. | ||
|
||
Uses the DNS protocol to resolve a IPv6 addresses (`AAAA` records) for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a IPv6 addresses -> IPv6 addresses
doc/api/dns.md
Outdated
treated separately. | ||
|
||
### dnsPromises.resolveAny(hostname) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing YAML.
doc/api/dns.md
Outdated
| `'A'` | `address`/`ttl` | | ||
| `'AAAA'` | `address`/`ttl` | | ||
| `'CNAME'` | `value` | | ||
| `'MX'` | Refer to [`dns.resolveMx()`][] | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[`dns.
-> [`dnsPromises.
in all table (with new bottom references)?
doc/api/dns.md
Outdated
Performs a reverse DNS query that resolves an IPv4 or IPv6 address to an | ||
array of hostnames. | ||
|
||
On error, `err` is an [`Error`][] object, where `err.code` is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This note needs to be promisified)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code wise LGTM, regarding the internal structure we can refactor on top of this PR later.
lib/internal/dns/promises.js
Outdated
'use strict'; | ||
const { | ||
bindDefaultResolver, | ||
Resolver: Resolver_, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be renamed to something like CallbackResolver
or LegacyResolver
to be more clear?
doc/api/dns.md
Outdated
|
||
Note that creating a new resolver uses the default server settings. Setting | ||
the servers used for a resolver using | ||
[`resolver.setServers()`][`dns.setServers()`] does not affect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be [`resolver.setServers()`][`dnsPromises.setServers()`]
?
doc/api/dns.md
Outdated
@@ -659,6 +1104,22 @@ uses. For instance, _they do not use the configuration from `/etc/hosts`_. | |||
[`dns.resolveTxt()`]: #dns_dns_resolvetxt_hostname_callback | |||
[`dns.reverse()`]: #dns_dns_reverse_ip_callback | |||
[`dns.setServers()`]: #dns_dns_setservers_servers | |||
[`dnsPromises.getServers()`]: #dns_dnspromises_getservers | |||
[`dnsPromises.lookup()`]: #dns_dnspromises_lookup_hostname_options_callback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In all these hashes, _callback
part needs to be deleted.
resolver.setServers(['4.4.4.4']); | ||
|
||
// This request will use the server at 4.4.4.4, independent of global settings. | ||
resolver.resolve4('example.org').then((addresses) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe show an example with async/await since according to polls that's what most users are doing?
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
doc/api/dns.md
Outdated
added: REPLACEME | ||
--> | ||
|
||
Cancel all outstanding DNS queries made by this resolver. The corresponding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This defacto introduces promise cancellation to Node.js - can we add a note to this that the API for cancellation may change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, we can remove it and revisit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cancel()
, getServers()
, and setServers()
don't technically use promises (or callbacks for that matter). They are just synchronous methods provided on the Resolver
class that are included here too for completeness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cjihrig this is a promise cancellation API though since you are rejecting all promises with an error "from a distance".
I'd really prefer it if we took extra caution around it since we'll want one API for cancellation and ideally users can use the same one for say... promisified timers, fs and DNS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can avoid exposing this in the promise API, or rename it to something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm willing to remove it because it's a simple change. I don't agree with the change though, and I'm very -1 to renaming it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing it until we have a more general strategy about cancellation with async functions SGTM. We can bring this up with TC39 at the next meeting if you'd like.
this.resolve(addresses); | ||
} | ||
|
||
function createLookupPromise(family, hostname, all, hints, verbatim) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would really prefer it if this method dind't do so much explicit construction:
async function createLookupPromise(family, hostname, all, hints, verbatim) {
if (!hostname) {
if (all) return [];
else
return { address: null, family: family === 6 ? 6 : 4 };
}
const matchedFamily = isIP(hostname);
if (matchedFamily !== 0) {
const result = { address: hostname, family: matchedFamily }
if(all) return [result];
return result;
}
return await new Promise((resolve, reject) => {
const req = new GetAddrInfoReqWrap();
// ...
req.resolve = resolve;
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a huge fan of how the code is written and I think it can be simpler - but it looks correct and the effort and progress is really nice.
Left some comment and LGTMd the current code as experimental to iterate on :)
PR-URL: nodejs#21264 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Because reasons. PR-URL: nodejs#21264 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Another CI run: https://ci.nodejs.org/job/node-test-pull-request/15536/ (AIX failures are unrelated, and I saw them on other CI runs today). Landed in 7486c4d, with the |
PR-URL: #21264 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Because reasons. PR-URL: #21264 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Awesome, going to follow up with the nits from above :) |
Notable changes: * build: * Node.js should now be about 60% faster to startup than the previous version, thanks to the use V8's code cache feature for core modules. [#21405](#21405) * dns: * An experimental promisified version of the dns module is now available. Give it a try with `require('dns').promises`. [#21264](#21264) * fs: * `fs.lchown` has been undeprecated now that libuv supports it. [#21498](#21498) * lib: * `Atomics.wake` is being renamed to `Atomics.notify` in the ECMAScript specification ([reference](tc39/ecma262#1220)). Since Node.js now has experimental support for worker threads, we are being proactive and added a `notify` alias, while emitting a warning if `wake` is used. [#21413](#21413) [#21518](#21518) * n-api: * Add API for asynchronous functions. [#17887](#17887) * util: * `util.inspect` is now able to return a result instead of throwing when the maximum call stack size is exceeded during inspection. [#20725](#20725) * vm: * Add `script.createCachedData()`. This API replaces the `produceCachedData` option of the `Script` constructor that is now deprecated. [#20300](#20300) * worker: * Support for relative paths has been added to the `Worker` constructor. Paths are interpreted relative to the current working directory. [#21407](#21407) PR-URL: #21629
Notable changes: * dns: * An experimental promisified version of the dns module is now available. Give it a try with `require('dns').promises`. [#21264](#21264) * fs: * `fs.lchown` has been undeprecated now that libuv supports it. [#21498](#21498) * lib: * `Atomics.wake` is being renamed to `Atomics.notify` in the ECMAScript specification ([reference](tc39/ecma262#1220)). Since Node.js now has experimental support for worker threads, we are being proactive and added a `notify` alias, while emitting a warning if `wake` is used. [#21413](#21413) [#21518](#21518) * n-api: * Add API for asynchronous functions. [#17887](#17887) * util: * `util.inspect` is now able to return a result instead of throwing when the maximum call stack size is exceeded during inspection. [#20725](#20725) * vm: * Add `script.createCachedData()`. This API replaces the `produceCachedData` option of the `Script` constructor that is now deprecated. [#20300](#20300) * worker: * Support for relative paths has been added to the `Worker` constructor. Paths are interpreted relative to the current working directory. [#21407](#21407) PR-URL: #21629
Notable changes: * dns: * An experimental promisified version of the dns module is now available. Give it a try with `require('dns').promises`. [#21264](#21264) * fs: * `fs.lchown` has been undeprecated now that libuv supports it. [#21498](#21498) * lib: * `Atomics.wake` is being renamed to `Atomics.notify` in the ECMAScript specification ([reference](tc39/ecma262#1220)). Since Node.js now has experimental support for worker threads, we are being proactive and added a `notify` alias, while emitting a warning if `wake` is used. [#21413](#21413) [#21518](#21518) * n-api: * Add API for asynchronous functions. [#17887](#17887) * util: * `util.inspect` is now able to return a result instead of throwing when the maximum call stack size is exceeded during inspection. [#20725](#20725) * vm: * Add `script.createCachedData()`. This API replaces the `produceCachedData` option of the `Script` constructor that is now deprecated. [#20300](#20300) * worker: * Support for relative paths has been added to the `Worker` constructor. Paths are interpreted relative to the current working directory. [#21407](#21407) PR-URL: #21629
This is a work in progress attempt at a promisified DNS core module. I'm opening this early to see if people are generally interested in this approach.The entire API is not ported yet - mostly just missinglookup()
andlookupService()
.Some notes:
require('dns').promises
, as similarly done in thefs
API. Like infs
, they are lazy loaded and output an experimental warning.This currently relies onprocess.binding('util').createPromise
and friends, which are currently facing potential removal in another PR.make test
passed for me locally, so no regressions should exist in the callback based API.Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes