net,dgram: add ipv6Only option for net and dgram by oyyd · Pull Request #23798 · nodejs/node · GitHub

For our servers, the dual-stack support is enable by default, i.e. binding host "::" will also make "0.0.0.0" bound. This commit add ipv6Only option in net.Server.listen(), net.Socket.connect() and dgram.createSocket() methods which allows to disable dual-stack support.

Support for cluster module is also provided in this commit.

Fixes: #17664

/cc @silverwind

Checklist

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

Nit:

Suggested change

      
                `0.0.0.0` be bound. **Default:** `false`
          
                `0.0.0.0` be bound. **Default:** `false`.

Nit:

Suggested change

      
              **Default:** `false`
          
              **Default:** `false`.

minor nit: perhaps this should be camel case (ipv6Only)?

Though I prefer ipv6only here, it's better to follow camel case :)

Suggested change

      
            // This test ensures that dual-stack suuport is disabled when
          
            // This test ensures that dual-stack support is disabled when

This looks wrong. Boolean(flags | constants.UV_TCP_IPV6ONLY) is always true. Did you mean to use &?

Yes, a terrible mistake. I'm going to update it and add a test of falsy ipv6Only scenario.

Unused default argument.

The default value of ipv6Only would be actually undefined rather than false which is how its document described. This is the only reason I set the default argument.

PTAL. (Travis ci task for this halted..)

Thanks for doing this. Would you mind als contributing the same option to UDP / dgram?

Yes, that's possible. But the last discussion #17664 (comment) shows there is no need to provide the same option to dgram. How do you think about this now?

Just "disable dual-stack support", no "the." (And you'd normally say "binding to host", not "binding host".)

Did you mean to write WORKER_COUNT? The name's a bit confusing to me.

I should have been more careful.

It's unnecessary to wrap the Countdown callback in a mustCall(). That's done automatically.

I think I misunderstood the comment #17664 (comment). We could bind to and use unspecified address and it works as expected:

example

const dgram = require('dgram');

const socket = dgram.createSocket('udp6');
socket.on('message', (msg, rinfo) => {
  console.log('rinfo', rinfo)
});
socket.bind(0, '::', () => {
  const { port } = socket.address();
  const client = dgram.createSocket('udp4');
  // Ensure the `client` won't bind ipv6.
  client.bind(0, () => {
    client.send(Buffer.allocUnsafe(1), port, '0.0.0.0');
  });
});

So I would like to add the option to dgram too. Maybe I should open another PR?

ipv6Only for dgram.createSocket() is added. PTAL.

Yes, that's possible. But the last discussion #17664 (comment) shows there is no need to provide the same option to dgram. How do you think about this now?

I'd say still go for it. Even if the OS might not properly support the socket option, we can still expose it.

nit: don't need the common.mustCall() wrapper here.

/ping @silverwind @vsemozhetbyt @mscdex @richardlau @jasnell It should look good now. PTAL.

Docs LGTM.

CI: https://ci.nodejs.org/job/node-test-pull-request/18353/

parallel/test-net-connect-ipv6only failed on AIX:

events.js:167
      throw er; // Unhandled 'error' event
      ^

Error: connect EHOSTUNREACH :::35876 - Local (undefined:undefined)
    at internalConnect (net.js:883:16)
    at defaultTriggerAsyncIdScope (internal/async_hooks.js:294:19)
    at defaultTriggerAsyncIdScope (net.js:974:9)
    at internalTickCallback (internal/process/next_tick.js:70:11)
    at process._tickCallback (internal/process/next_tick.js:47:5)
    at Function.Module.runMain (internal/modules/cjs/loader.js:778:11)
    at startup (internal/bootstrap/node.js:300:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:833:3)
Emitted 'error' event at:
    at emitErrorNT (internal/streams/destroy.js:82:8)
    at emitErrorAndCloseNT (internal/streams/destroy.js:50:3)
    at internalTickCallback (internal/process/next_tick.js:72:19)
    at process._tickCallback (internal/process/next_tick.js:47:5)
    [... lines matching original stack trace ...]
    at bootstrapNodeJSCore (internal/bootstrap/node.js:833:3)

I have no idea why this happened though.

@nodejs/platform-aix ^^^^^^

I suggest removing the support for net.Socket.connect() in this PR as it fails on AIX and I guess we need to resolve it in libuv. The rest parts still make sense as the support for server is what we mostly care about.

I suggest removing the support for net.Socket.connect() in this PR as it fails on AIX and I guess we need to resolve it in libuv. The rest parts still make sense as the support for server is what we mostly care about.

Will you be doing that and rebasing? If not, should we apply a help wanted label to this so someone else will do it?

I will remove the net.Socket.connect() and rebase again.

For TCP servers, the dual-stack support is enable by default, i.e. binding host "::" will also make "0.0.0.0" bound. This commit add ipv6Only option in `net.Server.listen()` and `dgram.createSocket()` methods which allows to disable dual-stack support. Support for cluster module is also provided in this commit. Fixes: nodejs#17664

Resume: https://ci.nodejs.org/job/node-test-pull-request/18839/

Resume (for known issues): https://ci.nodejs.org/job/node-test-commit-arm-fanned/4499/

I have removed the support for net.Socket.connect() and the CI is green now.

/cc @bnoordhuis @jasnell Does this still LGTY?

Yes, still LGTM

Landed in 33a25b2. Thanks!

For TCP servers, the dual-stack support is enable by default, i.e. binding host "::" will also make "0.0.0.0" bound. This commit add ipv6Only option in `net.Server.listen()` and `dgram.createSocket()` methods which allows to disable dual-stack support. Support for cluster module is also provided in this commit. Fixes: #17664 PR-URL: #23798 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable Changes: * console,util: * `console` functions now handle symbols as defined in the spec. #23708 * The inspection `depth` default is now back at 2. #24326 * dgram,net: * Added ipv6Only option for `net` and `dgram`. #23798 * http: * Chosing between the http parser is now possible per runtime flag. #24739 * readline: * The `readline` module now supports async iterators. #23916 * repl: * The multiline history feature is removed. #24804 * tls: * Added min/max protocol version options. #24405 * The X.509 public key info now includes the RSA bit size and the elliptic curve. #24358 * url: * `pathToFileURL()` now supports LF, CR and TAB. #23720 * Windows: * Tools are not installed using Boxstarter anymore. #24677 * The install-tools scripts or now included in the dist. #24233 * Added new collaborator: * [antsmartian](https://github.com/antsmartian) - Anto Aravinth. #24655 PR-URL: #24854

For TCP servers, the dual-stack support is enable by default, i.e. binding host "::" will also make "0.0.0.0" bound. This commit add ipv6Only option in `net.Server.listen()` and `dgram.createSocket()` methods which allows to disable dual-stack support. Support for cluster module is also provided in this commit. Fixes: nodejs#17664 PR-URL: nodejs#23798 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable Changes: * console,util: * `console` functions now handle symbols as defined in the spec. nodejs#23708 * The inspection `depth` default is now back at 2. nodejs#24326 * dgram,net: * Added ipv6Only option for `net` and `dgram`. nodejs#23798 * http: * Chosing between the http parser is now possible per runtime flag. nodejs#24739 * readline: * The `readline` module now supports async iterators. nodejs#23916 * repl: * The multiline history feature is removed. nodejs#24804 * tls: * Added min/max protocol version options. nodejs#24405 * The X.509 public key info now includes the RSA bit size and the elliptic curve. nodejs#24358 * url: * `pathToFileURL()` now supports LF, CR and TAB. nodejs#23720 * Windows: * Tools are not installed using Boxstarter anymore. nodejs#24677 * The install-tools scripts or now included in the dist. nodejs#24233 * Added new collaborator: * [antsmartian](https://github.com/antsmartian) - Anto Aravinth. nodejs#24655 PR-URL: nodejs#24854

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 Oct 21, 2018

addaleax added cluster Issues and PRs related to the cluster subsystem. 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 Oct 21, 2018

vsemozhetbyt reviewed Oct 21, 2018

View reviewed changes

mscdex reviewed Oct 21, 2018

View reviewed changes

richardlau reviewed Oct 21, 2018

View reviewed changes

oyyd force-pushed the net-ipv6only branch 2 times, most recently from 291bb1d to 91e21b2 Compare October 22, 2018 09:58

bnoordhuis reviewed Oct 22, 2018

View reviewed changes

oyyd changed the title net: add ipv6only option for net module net: add ipv6Only option for net module Oct 22, 2018

bnoordhuis approved these changes Oct 23, 2018

View reviewed changes

jasnell reviewed Oct 23, 2018

View reviewed changes

oyyd force-pushed the net-ipv6only branch from cbafc39 to daa9abc Compare October 24, 2018 06:07

oyyd changed the title net: add ipv6Only option for net module net,dgram: add ipv6Only option for net and dgram Oct 24, 2018

jasnell reviewed Oct 24, 2018

View reviewed changes

oyyd force-pushed the net-ipv6only branch from fe146de to aad0e5a Compare October 25, 2018 02:58

jasnell approved these changes Oct 25, 2018

View reviewed changes

silverwind reviewed Oct 25, 2018

View reviewed changes

Comment thread doc/api/dgram.md Outdated

oyyd force-pushed the net-ipv6only branch from 539ae7c to b047728 Compare November 21, 2018 07:54

oyyd closed this Nov 22, 2018

oyyd added the dgram Issues and PRs related to the dgram subsystem / UDP. label Nov 22, 2018

BridgeAR mentioned this pull request Dec 5, 2018

v11.4.0 proposal #24854

Merged

4 tasks

	`0.0.0.0` be bound. Default: `false`
	`0.0.0.0` be bound. Default: `false`.

	// This test ensures that dual-stack suuport is disabled when
	// This test ensures that dual-stack support is disabled when

net,dgram: add ipv6Only option for net and dgram by oyyd · Pull Request #23798 · nodejs/node · GitHub

Uh oh!

Uh oh!

net,dgram: add ipv6Only option for net and dgram - #23798

net,dgram: add ipv6Only option for net and dgram#23798oyyd wants to merge 1 commit intonodejs:masternodejs/node:masterfrom oyyd:net-ipv6onlyoyyd/node:net-ipv6onlyCopy head branch name to clipboard

Conversation

oyyd commented Oct 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

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!

oyyd Oct 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oyyd commented Oct 22, 2018

Uh oh!

silverwind commented Oct 22, 2018

Uh oh!

oyyd commented Oct 23, 2018

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!

oyyd commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oyyd commented Oct 24, 2018

Uh oh!

silverwind commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oyyd commented Oct 25, 2018

Uh oh!

vsemozhetbyt commented Oct 25, 2018

Uh oh!

Uh oh!

Trott commented Nov 6, 2018

Uh oh!

oyyd commented Nov 12, 2018

Uh oh!

Trott commented Nov 12, 2018

Uh oh!

oyyd commented Nov 15, 2018

Uh oh!

Trott commented Nov 18, 2018

Uh oh!

oyyd commented Nov 18, 2018

Uh oh!

oyyd commented Nov 21, 2018

Uh oh!

oyyd commented Nov 21, 2018

Uh oh!

oyyd commented Nov 21, 2018

Uh oh!

jasnell commented Nov 21, 2018

net,dgram: add ipv6Only option for net and dgram#23798
oyyd wants to merge 1 commit into
nodejs:masterfrom
oyyd:net-ipv6only

oyyd commented Oct 21, 2018 •
edited
Loading

oyyd Oct 22, 2018 •
edited
Loading

oyyd commented Oct 24, 2018 •
edited
Loading

silverwind commented Oct 24, 2018 •
edited
Loading