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

Conversation

@LPardue
Copy link

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

doc/api/dgram.md Outdated Show resolved Hide resolved
lib/dgram.js Outdated Show resolved Hide resolved
@jasnell jasnell requested a review from mcollina Oct 2, 2017
@jasnell

This comment has been minimized.

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 left a comment

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

This comment has been minimized.

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

This comment has been minimized.

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 left a comment

LGTM

@LPardue

This comment has been minimized.

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 left a comment

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

This comment has been minimized.

Copy link
Author

LPardue commented Oct 10, 2017

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

@LPardue

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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 left a comment

Thanks, looks good to me!

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

This comment has been minimized.

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.

@LPardue LPardue force-pushed the LPardue:ssm branch from 7f4a711 to 87f327a Dec 1, 2017
@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Oct 15, 2019

Getting a clean CI on this will probably require landing #29979 first.

@Trott Trott referenced this pull request Oct 15, 2019
2 of 2 tasks complete
src/udp_wrap.cc Show resolved Hide resolved
@LPardue

This comment has been minimized.

Copy link
Author

LPardue commented Oct 16, 2019

I'm travelling and don't have much time to dig into this, especially since I lost a lot of the context since opening the PR. So if someone wants to step in and address the remaining issues I'm happy for that.

For example, @n-thumann seems to have spotted an issue and fixed it on LPardue@6e9d7c8. It seems like a real issue but I don't have the cycles to confirm right now.

Lucas Pardue and others added 8 commits Oct 2, 2017
This adds RFC 4607 support for IPv4 and IPv6.
@addaleax addaleax force-pushed the LPardue:ssm branch from 1c51999 to b049b3a Oct 16, 2019
@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Oct 16, 2019

@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.

@nodejs-github-bot

This comment has been minimized.

@LPardue

This comment has been minimized.

Copy link
Author

LPardue commented Oct 16, 2019

@addaleax thank you! You deserve a 🥇

@n-thumann

This comment has been minimized.

Copy link
Contributor

n-thumann commented Oct 16, 2019

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

This comment has been minimized.

Copy link
Member

addaleax commented Oct 16, 2019

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

@nodejs-github-bot

This comment has been minimized.

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Oct 17, 2019

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

@addaleax addaleax closed this Oct 17, 2019
addaleax added 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 added 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 added 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 referenced 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.