dns: enable managing independent channels in JS #14518
Conversation
addaleax
added
cares
nodejs-github-bot
added
c++
doc/api/dns.md
Outdated
There was a problem hiding this comment.
I’d be happy to accept other suggestions for the public name here.
There was a problem hiding this comment.
Resolver is definitely better, I think.
src/cares_wrap.cc
Outdated
There was a problem hiding this comment.
Nit: This line can segfault, see #14519. As I already stated there, you don't really need to worry about this.
There was a problem hiding this comment.
It’s not really new code, but yes, I’m changing this to the non-deprecated version of Get so that it “only” crashes.
src/cares_wrap.cc
Outdated
There was a problem hiding this comment.
This could as well be on one line IMO to be consistent with QueryAWrap::Send etc.
There was a problem hiding this comment.
Right, that’s an oversight. Updated!
src/cares_wrap.cc
Outdated
There was a problem hiding this comment.
I think we can add the timeout option here.
lib/dns.js
Outdated
There was a problem hiding this comment.
Perhaps make the function anonymous by default then set the name to bindingName before returning...
const fn = (name, /* options, */ callback) => {
/* ... */
};
fn.name = bindingName
return fn;
lib/dns.js
Outdated
There was a problem hiding this comment.
this is optional, but it might be worthwhile allowing the servers to be set here as an optional argument.
There was a problem hiding this comment.
Maybe not in this PR. @XadillaX already mentioned above that there might be other options we could consider adding here, so it seems something to give a little more thought to :)
doc/api/dns.md
Outdated
lib/dns.js
Outdated
There was a problem hiding this comment.
how about hide _handle via Object.defineProperty?
There was a problem hiding this comment.
Using a plain _handle property is kind of the standard in Node core. If we were to hide something, I’d suggest thinking of a meaningful custom util.inspect overload for resolvers.
This can be used to implement custom timeouts. Fixes: nodejs#7231
|
One more CI: https://ci.nodejs.org/job/node-test-commit/11484/ I’d like to land if it’s green. |
|
Landed in cee8d6d...0f5dabe |
addaleax
added
the
notable-change
|
Should land with #17014 if it lands on v6.x. |
|
Release team decided not to land on v6.x, if you disagree let us know. |
7 participants
Fixes: #7231
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes (bonus: maketest-internetalso passes)Affected core subsystem(s)
dns
CI: https://ci.nodejs.org/job/node-test-commit/11400/CI: https://ci.nodejs.org/job/node-test-commit/11402/CI: https://ci.nodejs.org/job/node-test-commit/11404/