dns: enable managing independent channels in JS by addaleax · Pull Request #14518 · nodejs/node · GitHub

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/

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

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.

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

Right, that’s an oversight. Updated!

I think we can add the timeout option here.

Refs: c-ares/c-ares#135

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!

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

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

channel?

@XadillaX Thanks for catching this, done!

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

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

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

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.

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 XadillaX and bnoordhuis 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

dns: enable managing independent channels in JS by addaleax · Pull Request #14518 · nodejs/node · GitHub

Uh oh!

Uh oh!

dns: enable managing independent channels in JS - #14518

dns: enable managing independent channels in JS#14518addaleax wants to merge 4 commits intonodejs:masternodejs/node:masterfrom addaleax:channellingaddaleax/node:channellingCopy head branch name to clipboard

Conversation

addaleax commented Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Jul 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Aug 1, 2017

Uh oh!

addaleax commented Aug 1, 2017

Uh oh!

gibfahn commented Dec 13, 2017

Uh oh!

gibfahn commented Jan 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dns: enable managing independent channels in JS#14518
addaleax wants to merge 4 commits into
nodejs:masterfrom
addaleax:channelling

addaleax commented Jul 27, 2017 •
edited
Loading