dgram: setMulticastInterface() outgoing interface selection by lostnet · Pull Request #7855 · nodejs/node

lostnet · 2016-07-23T20:38:28Z

Checklist

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

Affected core subsystem(s)

dgram

Description of change

Add wrapper for uv's uv_udp_set_multicast_interface which provides the
sender side mechanism to explicitly select an interface. (The
equivalent receiver side mechanism is the optional 2nd argument of
addMembership().)

santigimeno · 2016-07-24T06:39:58Z

test/parallel/test-dgram-multicast-setIf.js

wrapping this with assert.throws will allow to remove the thrown variable

santigimeno · 2016-07-24T07:17:09Z

I don't know how difficult it would be to test, but would it be possible to test that the sender is indeed sending the multicast messages using the chosen interface?

lostnet · 2016-07-24T12:14:48Z

I don't know how difficult it would be to test, but would it be possible to test that the sender is indeed sending the multicast messages using the chosen interface?

It is very easy to test on a system where you can copy output from an ifconfig -a into your script and at least 2 real interfaces have been setup. For automated tests, its rather hard to get anyone to do the prerequisite manual system configuration.

My current test script (requires ifaces[] to be updated to match the system):
multitest.js.txt
(source of original)

lostnet · 2016-07-24T13:31:31Z

Thanks santigimeno, I've made improvements corresponding to each of your comments and verified "make -j4 test" against the new version. Please let me know how it looks.

cjihrig · 2016-08-02T17:51:56Z

test/parallel/test-dgram-multicast-setIf.js

Space after //. Also, Interface doesn't need to be capitalized.

cjihrig · 2016-08-02T18:22:33Z

Perhaps you could add a test similar to test/internet/test-dgram-multicast-multi-process.js.

lostnet · 2016-08-02T22:02:21Z

Thank you @cjihrig and @yorkie, I agree with, and believe I have addressed all the comments in this second set of review changes.

On test-dgram-multicast-multi-process.js:
I've added default interface selection and explicitly to the membership in that test to show that they do no harm. But the only ways I've seen to test that this kind of network setting isn't always doing a NO-OP is with test frameworks that have either a privileged system configuration stage or a configuration verification stage where they decide if they can test opportunistically. (The test framework has to know what a real non-default interface's IP is and that it isn't physically on the same physical network as the default network or all packets get received simply since there isn't an alternative.)

Please let me know what you think, and if I've missed or misunderstood anything on the other changes,
Will

yorkie · 2016-08-03T03:30:59Z

test/parallel/test-dgram-multicast-setIf.js

filename in test/parallel should be in lower-case?

I like the order with regards to dgram-multicast for tab completion and relatedness, so maybe:
test/parallel/test-dgram-multicast-set-interface.js ?
(set_if is the only lowercase I would parse as an IF ioctl and IMO mixing '-'&'' is worse than mixed case.)

jasnell · 2016-08-03T16:26:08Z

doc/api/dgram.md

This should likely include some additional detail about when this method should be called and what the side effects are.

I've added a significant amount of detail (and hopefully used subsections correctly?) But I've intentionally left the question of exactly when a socket is first ready unanswered since it would be redundant to repeat libuv's unusual need to bind before socket operations for every operation.

@sebdeckers

* **crypto** * Support for multiple ECDH curves. [#15206](#15206) * **dgram** * Added `setMulticastInterface()` API. [#7855](#7855) * **n-api** * The command-line flag is no longer required to use N-API. [#14902](#14902) * **tls** * Docs-only deprecation of `parseCertString()`. [#14245](#14245) * **New Contributors** * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)

@sebdeckers

* **crypto** * Support for multiple ECDH curves. [#15206](#15206) * **dgram** * Added `setMulticastInterface()` API. [#7855](#7855) * Custom lookup functions are now supported. [#14560](#14560) * **n-api** * The command-line flag is no longer required to use N-API. [#14902](#14902) * **tls** * Docs-only deprecation of `parseCertString()`. [#14245](#14245) * **New Contributors** * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)

@sebdeckers

* **crypto** * Support for multiple ECDH curves. [#15206](#15206) * **dgram** * Added `setMulticastInterface()` API. [#7855](#7855) * Custom lookup functions are now supported. [#14560](#14560) * **n-api** * The command-line flag is no longer required to use N-API. [#14902](#14902) * **tls** * Docs-only deprecation of `parseCertString()`. [#14245](#14245) * **New Contributors** * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)

@sebdeckers

* **crypto** * Support for multiple ECDH curves. [#15206](#15206) * **dgram** * Added `setMulticastInterface()` API. [#7855](#7855) * Custom lookup functions are now supported. [#14560](#14560) * **n-api** * The command-line flag is no longer required to use N-API. [#14902](#14902) * **tls** * Docs-only deprecation of `parseCertString()`. [#14245](#14245) * **New Contributors** * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)

@sebdeckers

* **crypto** * Support for multiple ECDH curves. [#15206](nodejs/node#15206) * **dgram** * Added `setMulticastInterface()` API. [#7855](nodejs/node#7855) * Custom lookup functions are now supported. [#14560](nodejs/node#14560) * **n-api** * The command-line flag is no longer required to use N-API. [#14902](nodejs/node#14902) * **tls** * Docs-only deprecation of `parseCertString()`. [#14245](nodejs/node#14245) * **New Contributors** * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](nodejs/node#15354)

Add wrapper for uv's uv_udp_set_multicast_interface which provides the sender side mechanism to explicitly select an interface. The equivalent receiver side mechanism is the optional 2nd argument of addMembership(). PR-URL: #7855 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

This LTS release comes with 109 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 29 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) #12678 * crypto: - expose ECDH class (Bryan English) #8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) #10209 - warn on invalid authentication tag length (Tobias Nießen) #17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) #16835 * dgram: - added socket.setMulticastInterface() (Will Young) #7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) #13005 * lib: - return this from net.Socket.end() (Sam Roberts) #13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) #16386 * net: - return this from getConnections() (Sam Roberts) #13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) #13784 * repl: - improve require() autocompletion (Alexey Orlenko) #14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) #16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) #12087 - add process.ppid (cjihrig) #16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) #12839 * tools, build: - a new macOS installer! (JP Wesselink) #15179 * url: - WHATWG URL api support (James M Snell) #7448 * util: - add %i and %f formatting specifiers (Roman Reiss) #10308 PR-URL: #18342

Add wrapper for uv's uv_udp_set_multicast_interface which provides the sender side mechanism to explicitly select an interface. The equivalent receiver side mechanism is the optional 2nd argument of addMembership(). PR-URL: #7855 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

This LTS release comes with 109 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 29 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) #12678 * crypto: - expose ECDH class (Bryan English) #8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) #10209 - warn on invalid authentication tag length (Tobias Nießen) #17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) #16835 * dgram: - added socket.setMulticastInterface() (Will Young) #7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) #13005 * lib: - return this from net.Socket.end() (Sam Roberts) #13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) #16386 * net: - return this from getConnections() (Sam Roberts) #13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) #13784 * repl: - improve require() autocompletion (Alexey Orlenko) #14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) #16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) #12087 - add process.ppid (cjihrig) #16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) #12839 * tools, build: - a new macOS installer! (JP Wesselink) #15179 * url: - WHATWG URL api support (James M Snell) #7448 * util: - add %i and %f formatting specifiers (Roman Reiss) #10308 PR-URL: #18342

Add wrapper for uv's uv_udp_set_multicast_interface which provides the sender side mechanism to explicitly select an interface. The equivalent receiver side mechanism is the optional 2nd argument of addMembership(). PR-URL: #7855 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

This LTS release comes with 109 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 29 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) #12678 * crypto: - expose ECDH class (Bryan English) #8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) #10209 - warn on invalid authentication tag length (Tobias Nießen) #17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) #16835 * dgram: - added socket.setMulticastInterface() (Will Young) #7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) #13005 * lib: - return this from net.Socket.end() (Sam Roberts) #13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) #16386 * net: - return this from getConnections() (Sam Roberts) #13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) #13784 * repl: - improve require() autocompletion (Alexey Orlenko) #14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) #16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) #12087 - add process.ppid (cjihrig) #16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) #12839 * tools, build: - a new macOS installer! (JP Wesselink) #15179 * url: - WHATWG URL api support (James M Snell) #7448 * util: - add %i and %f formatting specifiers (Roman Reiss) #10308 PR-URL: #18342

This LTS release comes with 112 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 30 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) #12678 * crypto: - expose ECDH class (Bryan English) #8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) #10209 - warn on invalid authentication tag length (Tobias Nießen) #17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) #16835 * dgram: - added socket.setMulticastInterface() (Will Young) #7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) #13005 * lib: - return this from net.Socket.end() (Sam Roberts) #13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) #16386 * net: - return this from getConnections() (Sam Roberts) #13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) #13784 * repl: - improve require() autocompletion (Alexey Orlenko) #14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) #16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) #12087 - add process.ppid (cjihrig) #16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) #12839 * tools, build: - a new macOS installer! (JP Wesselink) #15179 * url: - WHATWG URL api support (James M Snell) #7448 * util: - add %i and %f formatting specifiers (Roman Reiss) #10308 PR-URL: #18342

This LTS release comes with 112 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 30 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) nodejs#12678 * crypto: - expose ECDH class (Bryan English) nodejs#8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) nodejs#10209 - warn on invalid authentication tag length (Tobias Nießen) nodejs#17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) nodejs#16835 * dgram: - added socket.setMulticastInterface() (Will Young) nodejs#7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) nodejs#13005 * lib: - return this from net.Socket.end() (Sam Roberts) nodejs#13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) nodejs#16386 * net: - return this from getConnections() (Sam Roberts) nodejs#13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) nodejs#13784 * repl: - improve require() autocompletion (Alexey Orlenko) nodejs#14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) nodejs#16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) nodejs#12087 - add process.ppid (cjihrig) nodejs#16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) nodejs#12839 * tools, build: - a new macOS installer! (JP Wesselink) nodejs#15179 * url: - WHATWG URL api support (James M Snell) nodejs#7448 * util: - add %i and %f formatting specifiers (Roman Reiss) nodejs#10308 PR-URL: nodejs#18342

nodejs-github-bot added dgram Issues and PRs related to the dgram subsystem / UDP. c++ Issues and PRs that require attention from people who are familiar with C++. labels Jul 23, 2016

mscdex removed the c++ Issues and PRs that require attention from people who are familiar with C++. label Jul 23, 2016

santigimeno reviewed Jul 24, 2016
View reviewed changes

test/parallel/test-dgram-multicast-setIf.js Outdated

Copy link

Member

santigimeno Jul 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrapping this with assert.throws will allow to remove the thrown variable

cjihrig reviewed Aug 2, 2016
View reviewed changes

test/parallel/test-dgram-multicast-setIf.js Outdated

Copy link

Contributor

cjihrig Aug 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space after //. Also, Interface doesn't need to be capitalized.

yorkie reviewed Aug 3, 2016
View reviewed changes

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 3, 2016

jasnell reviewed Aug 3, 2016
View reviewed changes

MylesBorins mentioned this pull request Jan 24, 2018

v6.13.0 proposal #18342

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

dgram: setMulticastInterface() outgoing interface selection#7855

dgram: setMulticastInterface() outgoing interface selection#7855
lostnet wants to merge 11 commits intonodejs:masterfrom
lostnet:master

lostnet commented Jul 23, 2016 •

edited by yorkie

Loading

Uh oh!

santigimeno Jul 24, 2016

Uh oh!

santigimeno commented Jul 24, 2016

Uh oh!

lostnet commented Jul 24, 2016 •

edited

Loading

Uh oh!

lostnet commented Jul 24, 2016

Uh oh!

cjihrig Aug 2, 2016

Uh oh!

cjihrig commented Aug 2, 2016

Uh oh!

lostnet commented Aug 2, 2016

Uh oh!

yorkie Aug 3, 2016

Uh oh!

lostnet Aug 3, 2016

Uh oh!

jasnell Aug 3, 2016

Uh oh!

lostnet Aug 5, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

Uh oh!

Conversation

lostnet commented Jul 23, 2016 • edited by yorkie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

santigimeno Jul 24, 2016

Choose a reason for hiding this comment

Uh oh!

santigimeno commented Jul 24, 2016

Uh oh!

lostnet commented Jul 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lostnet commented Jul 24, 2016

Uh oh!

cjihrig Aug 2, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Aug 2, 2016

Uh oh!

lostnet commented Aug 2, 2016

Uh oh!

yorkie Aug 3, 2016

Choose a reason for hiding this comment

Uh oh!

lostnet Aug 3, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell Aug 3, 2016

Choose a reason for hiding this comment

Uh oh!

lostnet Aug 5, 2016

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

lostnet commented Jul 23, 2016 •

edited by yorkie

Loading

lostnet commented Jul 24, 2016 •

edited

Loading