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: implement socket.bind({ fd }) #21745

Closed
wants to merge 1 commit into from
Closed

dgram: implement socket.bind({ fd }) #21745

wants to merge 1 commit into from

Conversation

oyyd
Copy link
Contributor

@oyyd oyyd commented Jul 11, 2018

dgram: Implement binding an existing fd. Allow passing a fd property to socket.bind() in dgram.

src: Add UDPWrap::Open.

Cluster is also considered. All related tests are ignored on windows.

Fixes: #14961

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

@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 Jul 11, 2018
@oyyd oyyd force-pushed the udp-fd branch 2 times, most recently from db24841 to 6d026ad Compare July 11, 2018 07:29
@ChALkeR ChALkeR added the dgram Issues and PRs related to the dgram subsystem / UDP. label Jul 11, 2018
lib/dgram.js Outdated
@@ -97,19 +98,27 @@ function newHandle(type, lookup) {
throw new ERR_SOCKET_BAD_TYPE();
}

// Follow the net.js module and ignore fd if it's not a valid number.
function isPossibleValidFD(fd) {
return typeof fd === 'number' && fd > 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

&& isFinite(fd) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or follow the net.js and use validateInt32. I will change it.

@Trott Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 11, 2018
doc/api/dgram.md Outdated
@@ -177,6 +178,11 @@ system will attempt to listen on all addresses. Once binding is
complete, a `'listening'` event is emitted and the optional `callback`
function is called.

The `options` object may contain an `fd` property. When a `fd` greater
Copy link
Contributor

Choose a reason for hiding this comment

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

A nit: I am not sure what is correct: an `fd` or a `fd`, but these two fragments need to be unified)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed :)

lib/dgram.js Outdated
function isPossibleValidFD(fd) {
let valid = true;
try {
validateInt32(fd, 'fd', 0);
Copy link
Member

Choose a reason for hiding this comment

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

Please use isInt32 instead. That does exactly what is necessary here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your advice. As we need to make sure that the fd is greater than 0, I use isUint32 directly.

Another test is added that it should return errno when the type of fd is not "UDP".

@BridgeAR
Copy link
Member

@nodejs/dgram PTAL

@vsemozhetbyt
Copy link
Contributor

Ping @nodejs/dgram

lib/dgram.js Outdated
function _createSocketHandle(address, port, addressType, fd, flags) {
// Opening an existing fd is not supported for UDP handles.
assert(typeof fd !== 'number' || fd < 0);
Copy link
Member

Choose a reason for hiding this comment

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

this used to throw, but now the logic returns an error, so it's a change in API. Given that we export this, it would be a semver-major change.

I'm a bit lost on why we need this _createSocketHandle  function at all. Does anyone know? If it's useful, we should remove the _. If it's not and it's useful for tests only, we should move it to internals.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be in internals. It's hard to say if there is any ecosystem usage though. It's used in core by the cluster module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, _createSocketHandle is used in cluster module to create a handle from a master process and handles like these will be shared with worker processes.

Maybe we can remove _ in other PRs as it's also named like _createServerHandle in the net module.

lib/dgram.js Outdated
const { UV_UDP_REUSEADDR } = process.binding('constants').os;
const TTYWrap = process.binding('tty_wrap');
Copy link
Member

Choose a reason for hiding this comment

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

I'm not fond of UDP requiring TTY. Maybe we should move that guessHandleType somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe move the guessHandleType into lib/internal/util.js or lib/internal/net.js?

@mcollina mcollina added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Jul 17, 2018
lib/dgram.js Outdated
if (err) {
handle.close();
return err;
if (isUint32(fd)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Small detail, but I don't think we want a uint32, but rather an int32 that is not negative.

@oyyd
Copy link
Contributor Author

oyyd commented Jul 24, 2018

I would like to make appropriate change after @cjihrig's #21923 landed.

lib/dgram.js Outdated
return;
}

if (!state.handle)
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit: can braces be included with the conditional's multi-line body?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

lib/dgram.js Outdated
@@ -171,6 +194,44 @@ Socket.prototype.bind = function(port_, address_ /* , callback */) {
return this;
}

// Open an existing fd instead.
if (port !== null && typeof port === 'object' &&
isInt32(port.fd) && port.fd > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The indentation is off here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


socket.on('message', common.mustCall((data) => {
assert.strictEqual(data.toString('utf8'), String(fd));
setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this timeout and the one below for the receiver?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. It's unnecessary here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The receiver one is needed because calling close directly would prevent the message from being sent.

I use process.nextTick instead of the setTimeout as I think it's more precise here.

@oyyd oyyd force-pushed the udp-fd branch 2 times, most recently from bf50fde to 21f3515 Compare July 30, 2018 13:04
@oyyd
Copy link
Contributor Author

oyyd commented Jul 30, 2018

@mscdex @cjihrig @mcollina PTAL

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.

LGTN

@mscdex
Copy link
Contributor

mscdex commented Jul 30, 2018

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

@mcollina why is this tagged as semver-major? aren't feature additions semver-minor?
EDIT: I've seen the comment now...

@mcollina
Copy link
Member

@mscdex I'm ok in landing it as a minor if there is no ecosystem usage of this. That might truly be the case.

@oyyd
Copy link
Contributor Author

oyyd commented Aug 2, 2018

The test that failed before on Jenkins seems unrelated (see #22041).

@mcollina
Copy link
Member

mcollina commented Aug 3, 2018

@mcollina mcollina added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 3, 2018
@mcollina
Copy link
Member

mcollina commented Aug 3, 2018

@nodejs/tsc PTAL, this need another LGTM.

const assert = require('assert');
const cluster = require('cluster');
const dgram = require('dgram');
const UDP = process.binding('udp_wrap').UDP;
Copy link
Member

Choose a reason for hiding this comment

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

minor nit: const { UDP } = process.binding('udp_wrap');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@Trott
Copy link
Member

Trott commented Aug 4, 2018

Green CI and two TSC approvals on a semver-major means this can land. Since TSC was only pinged 12 hours ago, it might be a good idea to leave it open for the weekend to give people a chance to weigh in before landing. (But that is not required.)

dgram: Implement binding an existing `fd`. Allow pass a `fd` property
to `socket.bind()` in dgram.
src: Add `UDPWrap::Open`

Fixes: nodejs#14961
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.

LGTM. It might make sense to expose fd now, but don't worry about it in this PR.

@mcollina
Copy link
Member

mcollina commented Aug 6, 2018

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

(last before landing)

@mcollina
Copy link
Member

mcollina commented Aug 6, 2018

Landed in 2bea9ce

@mcollina mcollina closed this Aug 6, 2018
@mcollina
Copy link
Member

mcollina commented Aug 6, 2018

Thanks @oyyd for your first contribution to Node.js Core! 🎉

mcollina pushed a commit that referenced this pull request Aug 6, 2018
dgram: Implement binding an existing `fd`. Allow pass a `fd` property
to `socket.bind()` in dgram.
src: Add `UDPWrap::Open`

PR-URL: #21745
Fixes: #14961
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@oyyd
Copy link
Contributor Author

oyyd commented Aug 6, 2018

@mcollina Thanks!

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++. dgram Issues and PRs related to the dgram subsystem / UDP. lib / src Issues and PRs related to general changes in the lib or src directory. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow file descriptor to be passed to UDP socket - just as one can for TCP/Unix sockets
10 participants