net: support DNS hints in createConnection() by cjihrig · Pull Request #6000 · nodejs/node

cjihrig · 2016-04-01T15:36:27Z

Pull Request check-list

Please make sure to review and check all of these items:

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to CONTRIBUTING.md?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

Affected core subsystem(s)

net

Description of change

This commit adds support for passing DNS lookup hints to createConnection().

This PR is semver minor, but I'd like to toss out the idea of a semver major change where no DNS hints are implicitly set. Instead, users could specify hints if necessary. My motivation is a number of issues related to varying support of V4MAPPED. Thoughts @bnoordhuis / @indutny?

jasnell · 2016-04-01T16:56:44Z

maybe make these const since you're in here anyway?

jasnell · 2016-04-01T16:57:58Z

Generally that sounds ok to me, but can you describe the issues a bit?

cjihrig · 2016-04-01T17:07:40Z

The issue is that it isn't supported on all platforms and we end up with hacks just to set the flag on connections. In most cases this probably isn't needed at all, and being explicit would probably be better.

Some reference issues:
nodejs/node-v0.x-archive#8540
#3800
#337
#3771
#5436

bnoordhuis · 2016-04-01T17:11:40Z

LGTM. IIRC, the deeper issue is that we set AI_V4MAPPED indiscriminately instead of only when family === 6.

cjihrig · 2016-04-01T17:13:16Z

@bnoordhuis yea, that was one of the issues. It's also not supported on FreeBSD and Android. Would you be OK with making hints explicit in a semver major?

bnoordhuis · 2016-04-01T17:18:32Z

You mean making it a required argument period or required when you want to set AI_V4MAPPED? The latter is okay, I think, although there may be some fallout and it pushes the burden of dealing with platform inconsistencies to user land.

cjihrig · 2016-04-01T17:34:44Z

You mean making it a required argument period or required when you want to set AI_V4MAPPED?

I meant removing this block and making hints strictly optional.

I suppose another alternative is to make the change suggested in #5436 and only set V4MAPPED when the flag is supported by the platform and the family is explicitly set to 6.

This commit adds support for passing DNS lookup hints to createConnection(). PR-URL: nodejs#6000 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

bnoordhuis · 2016-04-01T17:45:17Z

I meant removing this block and making hints strictly optional.

Right. I think it's a good idea in that it means less implicit behavior and more mechanism over policy. However, we've been shipping this code for a while so we should test that it doesn't break to much. @thealphanerd can probably help with that.

cjihrig · 2016-04-01T17:46:30Z

OK, I'll put together a PR that can be run through CITGM.

cjihrig · 2016-04-01T17:46:43Z

CI for this PR: https://ci.nodejs.org/job/node-test-pull-request/2123/

UPDATE: First run had one failure, which may have been flakey. Trying again: https://ci.nodejs.org/job/node-test-pull-request/2125/

jasnell · 2016-04-01T20:11:28Z

@cjihrig .. thanks .. yeah, I think I'm +1 on it also. this change LGTM if CI is green. Will watch for the other PR!

cjihrig · 2016-04-01T21:04:24Z

Landed in 39de601

This commit adds support for passing DNS lookup hints to createConnection(). PR-URL: #6000 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2016-04-05T17:53:30Z

@cjihrig the tests are failing when this commit is cherry-picked to v5.x

assert.js:330
throw actual;
^
TypeError: invalid argument: hints must use valid flags
at lookup (dns.js:129:13)
at lookupAndConnect (net.js:963:3)
at Socket.connect (net.js:903:5)
at Object.exports.connect.exports.createConnection (net.js:69:35)
at /Users/thealphanerd/code/node/master/test/parallel/test-net-create-connection.js:26:11
at _tryBlock (assert.js:296:5)
at _throws (assert.js:315:12)
at Function.assert.throws (assert.js:338:3)
at fail (/Users/thealphanerd/code/node/master/test/parallel/test-net-create-connection.js:25:12)
at Server.<anonymous> (/Users/thealphanerd/code/node/master/test/parallel/test-net-create-connection.js:92:3)

cjihrig · 2016-04-05T18:03:54Z

Looks like the error message in lib/dns.js is different across the branches:
master: https://github.com/nodejs/node/blob/master/lib/dns.js#L131
v5.x: https://github.com/nodejs/node/blob/v5.x/lib/dns.js#L129

Can you give that a shot in the test?

MylesBorins · 2016-04-05T18:05:40Z

@cjihrig I already tried updating the error message. The error as included above was the new test failing

This commit adds support for passing DNS lookup hints to createConnection(). PR-URL: nodejs#6000 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Conflicts: test/parallel/test-net-create-connection.js

cjihrig · 2016-04-05T18:22:25Z

@thealphanerd worked fine for me. See #6061

This commit adds support for passing DNS lookup hints to createConnection(). PR-URL: #6000 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Conflicts: test/parallel/test-net-create-connection.js

Buffer: * Buffer.prototype.compare can now compare sub-ranges of two Buffers (James M Snell) #5880 deps: * update to http-parser 2.7.0 (Fedor Indutny) #6279 * update ESLint to 2.7.0 (silverwind) #6132 net: * adds support for passing DNS lookup hints to createConnection() (Colin Ihrig) #6000 node: * Make the builtin libraries available for the --eval and --print CLI options (Anna Henningsen) #6207 npm: * upgrade npm to 3.8.6 (Kat Marchán) #6153 repl: * Pressing enter in the repl will repeat the last command by default if no input has been received. This behaviour was in node previously and was not removed intentionally. (Rich Trott) #6090 src: * add SIGINFO to supported signals (James Reggio) #6093 streams: * Fix a regression that caused by net streams requesting multiple chunks synchronously when combined with cork/uncork (Matteo Collina) #6164 zlib: * The flushing flag is now configurable allowing for decompression of partial data (Anna Henningsen) #6069

Buffer: * Buffer.prototype.compare can now compare sub-ranges of two Buffers (James M Snell) #5880 deps: * update to http-parser 2.7.0 (Fedor Indutny) #6279 * update ESLint to 2.7.0 (silverwind) #6132 net: * adds support for passing DNS lookup hints to createConnection() (Colin Ihrig) #6000 node: * Make the builtin libraries available for the --eval and --print CLI options (Anna Henningsen) #6207 npm: * upgrade npm to 3.8.6 (Kat Marchán) #6153 repl: * Pressing enter in the repl will repeat the last command by default if no input has been received. This behavior was in node previously and was not removed intentionally. (Rich Trott) #6090 src: * add SIGINFO to supported signals (James Reggio) #6093 streams: * Fix a regression that caused by net streams requesting multiple chunks synchronously when combined with cork/uncork (Matteo Collina) #6164 zlib: * The flushing flag is now configurable allowing for decompression of partial data (Anna Henningsen) #6069

Buffer: * Buffer.prototype.compare can now compare sub-ranges of two Buffers (James M Snell) #5880 deps: * update to http-parser 2.7.0 (Fedor Indutny) #6279 * update ESLint to 2.7.0 (silverwind) #6132 net: * adds support for passing DNS lookup hints to createConnection() (Colin Ihrig) #6000 node: * Make the builtin libraries available for the --eval and --print CLI options (Anna Henningsen) #6207 npm: * upgrade npm to 3.8.6 (Kat Marchán) #6153 repl: * Pressing enter in the repl will repeat the last command by default if no input has been received. This behaviour was in node previously and was not removed intentionally. (Rich Trott) #6090 src: * add SIGINFO to supported signals (James Reggio) #6093 streams: * Fix a regression that caused by net streams requesting multiple chunks synchronously when combined with cork/uncork (Matteo Collina) #6164 zlib: * The flushing flag is now configurable allowing for decompression of partial data (Anna Henningsen) #6069 PR-URL: #6322

Buffer: * Buffer.prototype.compare can now compare sub-ranges of two Buffers (James M Snell) nodejs#5880 deps: * update to http-parser 2.7.0 (Fedor Indutny) nodejs#6279 * update ESLint to 2.7.0 (silverwind) nodejs#6132 net: * adds support for passing DNS lookup hints to createConnection() (Colin Ihrig) nodejs#6000 node: * Make the builtin libraries available for the --eval and --print CLI options (Anna Henningsen) nodejs#6207 npm: * upgrade npm to 3.8.6 (Kat Marchán) nodejs#6153 repl: * Pressing enter in the repl will repeat the last command by default if no input has been received. This behaviour was in node previously and was not removed intentionally. (Rich Trott) nodejs#6090 src: * add SIGINFO to supported signals (James Reggio) nodejs#6093 streams: * Fix a regression that caused by net streams requesting multiple chunks synchronously when combined with cork/uncork (Matteo Collina) nodejs#6164 zlib: * The flushing flag is now configurable allowing for decompression of partial data (Anna Henningsen) nodejs#6069 PR-URL: nodejs#6322

Buffer: * Buffer.prototype.compare can now compare sub-ranges of two Buffers (James M Snell) #5880 deps: * update to http-parser 2.7.0 (Fedor Indutny) #6279 * update ESLint to 2.7.0 (silverwind) #6132 net: * adds support for passing DNS lookup hints to createConnection() (Colin Ihrig) #6000 node: * Make the builtin libraries available for the --eval and --print CLI options (Anna Henningsen) #6207 npm: * upgrade npm to 3.8.6 (Kat Marchán) #6153 repl: * Pressing enter in the repl will repeat the last command by default if no input has been received. This behaviour was in node previously and was not removed intentionally. (Rich Trott) #6090 src: * add SIGINFO to supported signals (James Reggio) #6093 streams: * Fix a regression that caused by net streams requesting multiple chunks synchronously when combined with cork/uncork (Matteo Collina) #6164 zlib: * The flushing flag is now configurable allowing for decompression of partial data (Anna Henningsen) #6069 PR-URL: #6322

cjihrig added net Issues and PRs related to the net subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Apr 1, 2016

jasnell reviewed Apr 1, 2016
View reviewed changes

Comment threadtest/parallel/test-net-create-connection.js Outdated
jasnellApr 1, 2016
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe make these const since you're in here anyway?

net: support DNS hints in createConnection()
f3792c9
This commit adds support for passing DNS lookup hints to createConnection(). PR-URL: nodejs#6000 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

cjihrig force-pushed the hints branch from b1ba8d5 to f3792c9Compare April 1, 2016 17:43

cjihrig mentioned this pull request Apr 1, 2016
New GitHub Merging #6002
Closed

cjihrig merged this pull request into nodejs:masterApr 1, 2016

cjihrig deleted the hints branch April 1, 2016 21:04

cjihrig mentioned this pull request Apr 2, 2016
net: remove implicit setting of DNS hints #6021
Closed
2 tasks

cjihrig mentioned this pull request Apr 5, 2016
net: support DNS hints in createConnection() #6061
Closed

MylesBorins mentioned this pull request Apr 21, 2016
V5.11.0 proposal #6321
Closed

MylesBorins mentioned this pull request Apr 21, 2016
V5.11.0 proposal #6322
Closed

Uh oh!

Uh oh!

Conversation

cjihrig commented Apr 1, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

Uh oh!

jasnellApr 1, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Apr 1, 2016

Uh oh!

cjihrig commented Apr 1, 2016

Uh oh!

bnoordhuis commented Apr 1, 2016

Uh oh!

cjihrig commented Apr 1, 2016

Uh oh!

bnoordhuis commented Apr 1, 2016

Uh oh!

cjihrig commented Apr 1, 2016

Uh oh!

bnoordhuis commented Apr 1, 2016

Uh oh!

cjihrig commented Apr 1, 2016

Uh oh!

cjihrig commented Apr 1, 2016

Uh oh!

jasnell commented Apr 1, 2016

Uh oh!

cjihrig commented Apr 1, 2016

Uh oh!

MylesBorins commented Apr 5, 2016

Uh oh!

cjihrig commented Apr 5, 2016

Uh oh!

MylesBorins commented Apr 5, 2016

Uh oh!

cjihrig commented Apr 5, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants