Skip to content
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

dgram: add source-specific multicast support #15735

Closed
wants to merge 9 commits into from
Closed

Conversation

LPardue
Copy link

@LPardue LPardue commented Oct 2, 2017

This adds RFC 4607 support for IPv4 and IPv6. I'm opening this PR to start a discussion, it is not fully realised yet.

This changeset depends on libuv PR 964 and preempts that change being merged into node deps (i.e. it won't compile against master at the time of writing).

In approaching this change I considered modify the existing addMembership/dropMembership functions with a new sourceAddress parameter vs. adding addSourceSpecificMembership/dropSourceSpecificMembership. Both have merits but I picked the latter, I'm not stuck on it though. It comes does to opinion on which looks worse: socket.addMembership(232.1.1.1, null, 123.45.67.89) or socket.addSourceSpecificMembership(123.45.67.89, 232.1.1.1).

I've tested this manually on a local network but don't currently have any tests defined. I'm not sure what node is looking for in that respect, testing source-specific multicast is hairy.

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
Affected core subsystem(s)

dgram

@nodejs-github-bot 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 2, 2017
@jasnell jasnell requested review from mscdex and cjihrig October 2, 2017 15:31
@jasnell jasnell added wip Issues and PRs that are still a work in progress. semver-minor PRs that contain new features and should be released in the next minor version. labels Oct 2, 2017
doc/api/dgram.md Outdated Show resolved Hide resolved
lib/dgram.js Outdated Show resolved Hide resolved
@jasnell jasnell requested a review from mcollina October 2, 2017 15:34
@jasnell
Copy link
Member

jasnell commented Oct 2, 2017

Good to see this and thank you for the PR! First scan through looks good!

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

Looking mostly good. Left some comments.

doc/api/dgram.md Outdated Show resolved Hide resolved
lib/dgram.js Outdated Show resolved Hide resolved
lib/dgram.js Outdated Show resolved Hide resolved
lib/dgram.js Outdated Show resolved Hide resolved
lib/dgram.js Outdated Show resolved Hide resolved
src/udp_wrap.cc Outdated Show resolved Hide resolved
src/udp_wrap.cc Show resolved Hide resolved
src/udp_wrap.cc Outdated Show resolved Hide resolved
@LPardue
Copy link
Author

LPardue commented Oct 3, 2017

Thanks for the comments. I'll address them soon. One observation is that this is pretty much a copy/paste job based on the existing functions. I.e. the existing code probably also suffers from the issues raised. Would you like me to reflect back changes to the existing functions?

@mcollina
Copy link
Member

mcollina commented Oct 7, 2017

I'm a bit uncomfortable in landing this without tests, I know very well that testing multicast is complicated.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@LPardue
Copy link
Author

LPardue commented Oct 7, 2017

Thanks for the feedback. Are there any resources for designing tests like this in node? How much of libuv tests (or methodology and test infrastructure) could be reused. This area is all a bit new to me but I completely understand the importance of testing before landing. (edited to fix spelling mistakes)

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

Left a comment, but otherwise LGTM. Getting through the rest of the libuv PR is one of my plans for the week.

Are there any resources designing tests like this in node?

I'd look in the test subdirectories. The files that start with test-dgram- are a good place to look. You can also take a look at https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md.

How much of libuv tests (or methodology and test infrastructure) could be reused.

The test machines are the same, and you'll likely be able to take the same approach to both. However, the libuv tests are all C and Node's are JS, so you'll have to port them between languages.

lib/dgram.js Outdated Show resolved Hide resolved
@LPardue
Copy link
Author

LPardue commented Oct 10, 2017

Thanks Colin, I hope to follow this up later in the week.

@LPardue
Copy link
Author

LPardue commented Oct 18, 2017

@cjihrig I've addressed your final review comment about type checking.

@mcollina I've added two tests for source-specific multicast (IPv4 and IPv6). These are near identical to the existing test test-dgram-multicast-multi-process.js test and so provide the same amount of test coverage. The main difference is that they discover a source address to use for the test., localhost doesn't work and will cause the receiver to fail.

@cjihrig
Copy link
Contributor

cjihrig commented Oct 18, 2017

@LPardue I went through the libuv PR. It needs updates from the author. I'd like another libuv collaborator to review it as well.

@LPardue
Copy link
Author

LPardue commented Oct 18, 2017

@LPardue I went through the libuv PR. It needs updates from the author. I'd like another libuv collaborator to review it as well.

Indeed, the PR doesn't work on Windows without fixes I suggested there.

src/udp_wrap.cc Outdated Show resolved Hide resolved
src/udp_wrap.cc Outdated Show resolved Hide resolved
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me!

doc/api/dgram.md Show resolved Hide resolved
doc/api/dgram.md Outdated Show resolved Hide resolved
@LPardue
Copy link
Author

LPardue commented Nov 22, 2017

I think I've addressed review comments. The conflicting file relates to libuv and probably shouldn't be in the PR, I'll try to fix this soon.

This is really stalled on libuv/libuv#964 which is awaiting changes in response to review.

@addaleax
Copy link
Member

@LPardue I’ve pushed a fix for my comment, included the commit you referenced, and a small fixup to make documentation linting pass (due to relatively new changes to the doc markdown format). I think after that this only requires us to get CI to pass.

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 16, 2019
@nodejs-github-bot
Copy link
Collaborator

@LPardue
Copy link
Author

LPardue commented Oct 16, 2019

@addaleax thank you! You deserve a 🥇

@n-thumann
Copy link
Contributor

I noticed two deprecation warnings, caused by this._healthCheck()and this._handle. I fixed both of them in n-thumann@d265295. Would you mind merging this commit too? :)

`this._healthCheck()`& `this._handle` caused deprecation warnings.
@addaleax
Copy link
Member

@n-thumann Done! (with linting issues fixed)

@nodejs-github-bot
Copy link
Collaborator

@addaleax
Copy link
Member

Landed in 1784b7f 🎉 Thanks for the PR, and sorry for all the delays! ✨

@addaleax addaleax closed this Oct 17, 2019
addaleax pushed a commit that referenced this pull request Oct 17, 2019
This adds RFC 4607 support for IPv4 and IPv6.

Co-Authored-By: Nicolas Thumann <46975855+n-thumann@users.noreply.github.com>
Co-Authored-By: Rich Trott <rtrott@gmail.com>
PR-URL: #15735
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 23, 2019
This adds RFC 4607 support for IPv4 and IPv6.

Co-Authored-By: Nicolas Thumann <46975855+n-thumann@users.noreply.github.com>
Co-Authored-By: Rich Trott <rtrott@gmail.com>
PR-URL: #15735
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 23, 2019
This adds RFC 4607 support for IPv4 and IPv6.

Co-Authored-By: Nicolas Thumann <46975855+n-thumann@users.noreply.github.com>
Co-Authored-By: Rich Trott <rtrott@gmail.com>
PR-URL: #15735
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit that referenced this pull request Nov 5, 2019
Notable changes:

* cli:
  * Added a new flag (`--trace-uncaught`) that makes Node.js print the
    stack trace at the time of throwing uncaught exceptions, rather than
    at the creation of the `Error` object, if there is any. This is
    disabled by default because it affects GC behavior.
    #30025
* crypto
  * Added `Hash.prototype.copy()` method. It returns a new `Hash` object
    with its internal state cloned from the original one.
    #29910
* dgram
  * Added source-specific multicast support. This adds methods to
    Datagram sockets to support RFC 4607
    (https://tools.ietf.org/html/rfc4607) for IPv4 and IPv6.
    #15735
* fs
  * Added a `bufferSize` option to `fs.opendir()`. It allows to control
    the number of entries that are buffered internally when reading from
    the directory. #30114
* meta
  * Added Chengzhong Wu (https://github.com/legendecas) to
    collaborators. #30115

PR-URL: #30262
@targos targos mentioned this pull request Nov 5, 2019
targos added a commit that referenced this pull request Nov 5, 2019
Notable changes:

* cli:
  * Added a new flag (`--trace-uncaught`) that makes Node.js print the
    stack trace at the time of throwing uncaught exceptions, rather than
    at the creation of the `Error` object, if there is any. This is
    disabled by default because it affects GC behavior.
    #30025
* crypto
  * Added `Hash.prototype.copy()` method. It returns a new `Hash` object
    with its internal state cloned from the original one.
    #29910
* dgram
  * Added source-specific multicast support. This adds methods to
    Datagram sockets to support RFC 4607
    (https://tools.ietf.org/html/rfc4607) for IPv4 and IPv6.
    #15735
* fs
  * Added a `bufferSize` option to `fs.opendir()`. It allows to control
    the number of entries that are buffered internally when reading from
    the directory. #30114
* meta
  * Added Chengzhong Wu (https://github.com/legendecas) to
    collaborators. #30115

PR-URL: #30262
targos pushed a commit that referenced this pull request Jan 8, 2020
This adds RFC 4607 support for IPv4 and IPv6.

Co-Authored-By: Nicolas Thumann <46975855+n-thumann@users.noreply.github.com>
Co-Authored-By: Rich Trott <rtrott@gmail.com>
PR-URL: #15735
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jan 8, 2020
This adds RFC 4607 support for IPv4 and IPv6.

Co-Authored-By: Nicolas Thumann <46975855+n-thumann@users.noreply.github.com>
Co-Authored-By: Rich Trott <rtrott@gmail.com>
PR-URL: #15735
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
This adds RFC 4607 support for IPv4 and IPv6.

Co-Authored-By: Nicolas Thumann <46975855+n-thumann@users.noreply.github.com>
Co-Authored-By: Rich Trott <rtrott@gmail.com>
PR-URL: #15735
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet