dns: enable managing independent channels in JS #14518

addaleax · 2017-07-27T17:38:29Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes (bonus: make test-internet also passes)
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

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/

addaleax · 2017-07-27T17:58:49Z

doc/api/dns.md

@@ -54,6 +54,55 @@ dns.resolve4('archive.org', (err, addresses) => {
 There are subtle consequences in choosing one over the other, please consult
 the [Implementation considerations section][] for more information.

+## Class dns.Channel


I’d be happy to accept other suggestions for the public name here.

dns.Resolver?

Resolver is definitely better, I think.

@XadillaX @jasnell I agree, renamed to Resolver. :)

tniessen · 2017-07-27T19:31:31Z

src/cares_wrap.cc

+      : AsyncWrap(channel->env(), req_wrap_obj, AsyncWrap::PROVIDER_QUERYWRAP),
+        channel_(channel) {
+    if (env()->in_domain())
+      req_wrap_obj->Set(env()->domain_string(), env()->domain_array()->Get(0));


Nit: This line can segfault, see #14519. As I already stated there, you don't really need to worry about this.

It’s not really new code, but yes, I’m changing this to the non-deprecated version of Get so that it “only” crashes.

tniessen · 2017-07-27T19:33:25Z

src/cares_wrap.cc

-               GetQueryArg());
+    AresQuery(name,
+              ns_c_in,
+              ns_t_any);


This could as well be on one line IMO to be consistent with QueryAWrap::Send etc.

Right, that’s an oversight. Updated!

XadillaX · 2017-07-28T03:06:57Z

src/cares_wrap.cc

@@ -409,22 +477,53 @@ struct CaresAsyncData {
  uv_async_t async_handle;
 };

-void SetupCaresChannel(Environment* env) {
+void ChannelWrap::Setup() {
  struct ares_options options;
  memset(&options, 0, sizeof(options));
  options.flags = ARES_FLAG_NOCHECKRESP;


I think we can add the timeout option here.

Refs: c-ares/c-ares#135

jasnell · 2017-07-31T19:52:47Z

lib/dns.js

@@ -263,26 +274,27 @@ function resolver(bindingName) {
    req.hostname = name;
    req.oncomplete = onresolve;
    req.ttl = !!(options && options.ttl);
-    var err = binding(req, name);
+    var err = this._handle[bindingName](req, name);
    if (err) throw errnoException(err, bindingName);
    return req;
  };


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;

@jasnell Good idea, done!

jasnell · 2017-07-31T19:54:40Z

lib/dns.js

-function resolver(bindingName) {
-  var binding = cares[bindingName];
+class Channel {
+  constructor() {


this is optional, but it might be worthwhile allowing the servers to be set here as an optional argument.

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 :)

addaleax · 2017-07-31T20:43:31Z

New CI: https://ci.nodejs.org/job/node-test-commit/11468/

XadillaX · 2017-08-01T06:38:52Z

doc/api/dns.md

+* [`resolver.resolveTxt()`][`dns.resolveTxt()`]
+* [`resolver.reverse()`][`dns.reverse()`]
+
+### channel.cancel()


@XadillaX Thanks for catching this, done!

XadillaX · 2017-08-01T06:39:40Z

lib/dns.js

+// Resolver instances correspond 1:1 to c-ares channels.
+class Resolver {
+  constructor() {
+    this._handle = new ChannelWrap();


how about hide _handle via Object.defineProperty?

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.

Ref: nodejs#7231

This can be used to implement custom timeouts. Fixes: nodejs#7231

addaleax · 2017-08-01T16:35:15Z

One more CI: https://ci.nodejs.org/job/node-test-commit/11484/

I’d like to land if it’s green.

addaleax · 2017-08-01T18:08:17Z

Landed in cee8d6d...0f5dabe

PR-URL: #14518 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Ref: #7231 PR-URL: #14518 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

This can be used to implement custom timeouts. Fixes: #7231 PR-URL: #14518 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

PR-URL: #14518 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Ref: #7231 PR-URL: #14518 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

This can be used to implement custom timeouts. Fixes: #7231 PR-URL: #14518 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

PR-URL: #14518 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

V8 6.0: The V8 engine has been upgraded to version 6.0, which has a significantly changed performance profile. [#14574](#14574) More detailed information on performance differences can be found at https://medium.com/the-node-js-collection/get-ready-a-new-v8-is-coming-node-js-performance-is-changing-46a63d6da4de Other notable changes: * **DNS** * Independent DNS resolver instances are supported now, with support for cancelling the corresponding requests. [#14518](#14518) * **REPL** * Autocompletion support for `require()` has been improved. [#14409](#14409) * **Utilities** * The WHATWG Encoding Standard (`TextDecoder` and `TextEncoder`) has been implemented. [#13644](#13644) * **Added new collaborators** * [XadillaX](https://github.com/XadillaX) – Khaidi Chu

V8 6.0: The V8 engine has been upgraded to version 6.0, which has a significantly changed performance profile. [#14574](#14574) More detailed information on performance differences can be found at https://medium.com/the-node-js-collection/get-ready-a-new-v8-is-coming-node-js-performance-is-changing-46a63d6da4de Other notable changes: * **DNS** * Independent DNS resolver instances are supported now, with support for cancelling the corresponding requests. [#14518](#14518) * **N-API** * Multiple N-API functions for error handling have been changed to support assigning error codes. [#13988](#13988) * **REPL** * Autocompletion support for `require()` has been improved. [#14409](#14409) * **Utilities** * The WHATWG Encoding Standard (`TextDecoder` and `TextEncoder`) has been implemented as an experimental feature. [#13644](#13644) * **Added new collaborators** * [XadillaX](https://github.com/XadillaX) – Khaidi Chu

This fixes a bug introduced in 727b291 where code managing the `uv_timer_t` for a `ChannelWrap` instance was left unchanged, when it should have changed the lifetime of the handle to being tied to the `ChannelWrap` instance’s lifetime. Fixes: nodejs#14599 Ref: nodejs#14518

This fixes a bug introduced in 727b291 where code managing the `uv_timer_t` for a `ChannelWrap` instance was left unchanged, when it should have changed the lifetime of the handle to being tied to the `ChannelWrap` instance’s lifetime. Fixes: #14599 Ref: #14518 PR-URL: #14634 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

V8 6.0: The V8 engine has been upgraded to version 6.0, which has a significantly changed performance profile. [#14574](#14574) More detailed information on performance differences can be found at https://medium.com/the-node-js-collection/get-ready-a-new-v8-is-coming-node-js-performance-is-changing-46a63d6da4de Other notable changes: * **DNS** * Independent DNS resolver instances are supported now, with support for cancelling the corresponding requests. [#14518](#14518) * **N-API** * Multiple N-API functions for error handling have been changed to support assigning error codes. [#13988](#13988) * **REPL** * Autocompletion support for `require()` has been improved. [#14409](#14409) * **Utilities** * The WHATWG Encoding Standard (`TextDecoder` and `TextEncoder`) has been implemented as an experimental feature. [#13644](#13644) * **Added new collaborators** * [XadillaX](https://github.com/XadillaX) – Khaidi Chu

V8 6.0: The V8 engine has been upgraded to version 6.0, which has a significantly changed performance profile. [#14574](#14574) More detailed information on performance differences can be found at https://medium.com/the-node-js-collection/get-ready-a-new-v8-is-coming-node-js-performance-is-changing-46a63d6da4de Other notable changes: * **DNS** * Independent DNS resolver instances are supported now, with support for cancelling the corresponding requests. [#14518](#14518) * **N-API** * Multiple N-API functions for error handling have been changed to support assigning error codes. [#13988](#13988) * **REPL** * Autocompletion support for `require()` has been improved. [#14409](#14409) * **Utilities** * The WHATWG Encoding Standard (`TextDecoder` and `TextEncoder`) has been implemented as an experimental feature. [#13644](#13644) * **Added new collaborators** * [XadillaX](https://github.com/XadillaX) – Khaidi Chu * [gabrielschulhof](https://github.com/gabrielschulhof) – Gabriel Schulhof

V8 6.0: The V8 engine has been upgraded to version 6.0, which has a significantly changed performance profile. [#14574](#14574) More detailed information on performance differences can be found at https://medium.com/the-node-js-collection/get-ready-a-new-v8-is-coming-node-js-performance-is-changing-46a63d6da4de Other notable changes: * **DNS** * Independent DNS resolver instances are supported now, with support for cancelling the corresponding requests. [#14518](#14518) * **N-API** * Multiple N-API functions for error handling have been changed to support assigning error codes. [#13988](#13988) * **REPL** * Autocompletion support for `require()` has been improved. [#14409](#14409) * **Utilities** * The WHATWG Encoding Standard (`TextDecoder` and `TextEncoder`) has been implemented as an experimental feature. [#13644](#13644) * **Added new collaborators** * [XadillaX](https://github.com/XadillaX) – Khaidi Chu * [gabrielschulhof](https://github.com/gabrielschulhof) – Gabriel Schulhof Conflicts: src/node_version.h

gibfahn · 2017-12-13T14:59:52Z

Should land with #17014 if it lands on v6.x.

gibfahn · 2018-01-15T22:05:50Z

Release team decided not to land on v6.x, if you disagree let us know.

addaleax added cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. dns Issues and PRs related to the dns subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Jul 27, 2017

addaleax requested review from bnoordhuis and XadillaX July 27, 2017 17:38

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jul 27, 2017

addaleax mentioned this pull request Jul 27, 2017

How to add max timeout on DNS query? #7231

Closed

addaleax force-pushed the channelling branch from 1ff2fe3 to 5ce7982 Compare July 27, 2017 17:48

addaleax commented Jul 27, 2017

View reviewed changes

addaleax force-pushed the channelling branch from 5ce7982 to 2eff98a Compare July 27, 2017 18:01

tniessen approved these changes Jul 27, 2017

View reviewed changes

addaleax force-pushed the channelling branch from 2eff98a to bc57216 Compare July 27, 2017 19:46

XadillaX reviewed Jul 28, 2017

View reviewed changes

tniessen mentioned this pull request Jul 30, 2017

crypto: change segmentation faults to CHECKs #14548

Closed

3 tasks

cjihrig approved these changes Jul 31, 2017

View reviewed changes

jasnell reviewed Jul 31, 2017

View reviewed changes

addaleax force-pushed the channelling branch from bc57216 to 4f11e4a Compare July 31, 2017 20:30

XadillaX reviewed Aug 1, 2017

View reviewed changes

addaleax added 4 commits August 1, 2017 11:42

src,dns: refactor cares_wrap to avoid global state

23245d4

dns: enable usage of independent cares resolvers

ca4e5c9

Ref: nodejs#7231

dns: add channel.cancel()

2fb2a88

This can be used to implement custom timeouts. Fixes: nodejs#7231

dns: name generated functions

b4e426f

addaleax force-pushed the channelling branch from 4f11e4a to b4e426f Compare August 1, 2017 09:42

jasnell approved these changes Aug 1, 2017

View reviewed changes

addaleax closed this Aug 1, 2017

addaleax added a commit that referenced this pull request Aug 1, 2017

src,dns: refactor cares_wrap to avoid global state

727b291

PR-URL: #14518 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

addaleax added a commit that referenced this pull request Aug 1, 2017

dns: enable usage of independent cares resolvers

6e05970

Ref: #7231 PR-URL: #14518 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

addaleax added a commit that referenced this pull request Aug 1, 2017

dns: name generated functions

0f5dabe

PR-URL: #14518 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

addaleax added a commit that referenced this pull request Aug 1, 2017

src,dns: refactor cares_wrap to avoid global state

106a23b

PR-URL: #14518 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

addaleax added a commit that referenced this pull request Aug 1, 2017

dns: enable usage of independent cares resolvers

69e41dc

Ref: #7231 PR-URL: #14518 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

addaleax added a commit that referenced this pull request Aug 1, 2017

dns: name generated functions

32b30d5

PR-URL: #14518 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

addaleax added the notable-change PRs with changes that should be highlighted in changelogs. label Aug 2, 2017

addaleax mentioned this pull request Aug 2, 2017

v8.3.0 proposal #14594

Merged

tniessen mentioned this pull request Aug 2, 2017

test/parallel/test-async-wrap-getasyncid.js crashing #14599

Closed

addaleax mentioned this pull request Aug 4, 2017

src: properly manage timer in cares ChannelWrap #14634

Closed

3 tasks

gibfahn mentioned this pull request Dec 13, 2017

src: fix bad sizeof expression #17014

Merged

gibfahn added the dont-land-on-v6.x label Jan 15, 2018

addaleax mentioned this pull request Jan 23, 2019

doc: clarify what dns.setResolvers() affects #25570

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dns: enable managing independent channels in JS #14518

dns: enable managing independent channels in JS #14518

addaleax commented Jul 27, 2017 •

edited

Loading

addaleax Jul 27, 2017

XadillaX Jul 27, 2017

jasnell Jul 31, 2017

addaleax Jul 31, 2017

tniessen Jul 27, 2017

addaleax Jul 27, 2017

tniessen Jul 27, 2017

addaleax Jul 27, 2017

XadillaX Jul 28, 2017

XadillaX Jul 28, 2017

jasnell Jul 31, 2017

addaleax Jul 31, 2017

jasnell Jul 31, 2017

addaleax Jul 31, 2017

addaleax commented Jul 31, 2017

XadillaX Aug 1, 2017

addaleax Aug 1, 2017

XadillaX Aug 1, 2017

addaleax Aug 1, 2017

addaleax commented Aug 1, 2017

addaleax commented Aug 1, 2017

gibfahn commented Dec 13, 2017

gibfahn commented Jan 15, 2018

dns: enable managing independent channels in JS #14518

dns: enable managing independent channels in JS #14518

Conversation

addaleax commented Jul 27, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Jul 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Aug 1, 2017

addaleax commented Aug 1, 2017

gibfahn commented Dec 13, 2017

gibfahn commented Jan 15, 2018

addaleax commented Jul 27, 2017 •

edited

Loading