Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.2k
dgram: setMulticastInterface() outgoing interface selection#7855
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Conversation
lostnet commented Jul 23, 2016 • edited by yorkie
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by yorkie
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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
santigimeno commented Jul 24, 2016
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 commented Jul 24, 2016 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
It is very easy to test on a system where you can copy output from an My current test script (requires ifaces[] to be updated to match the system): |
lostnet commented Jul 24, 2016
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. |
There was a problem hiding this comment.
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.
cjihrig commented Aug 2, 2016
Perhaps you could add a test similar to |
lostnet commented Aug 2, 2016
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: Please let me know what you think, and if I've missed or misunderstood anything on the other changes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filename in test/parallel should be in lower-case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
doc/api/dgram.md Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should likely include some additional detail about when this method should be called and what the side effects are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
* **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)
* **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)
* **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)
* **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)
* **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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
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) #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
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesAffected 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().)