dgram: implement socket.bind({ fd }) by oyyd · Pull Request #21745 · nodejs/node

oyyd · 2018-07-11T07:00:55Z

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

mscdex · 2018-07-11T10:03:17Z

lib/dgram.js

&& isFinite(fd) ?

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

vsemozhetbyt · 2018-07-11T16:41:18Z

doc/api/dgram.md

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

BridgeAR · 2018-07-12T23:37:34Z

lib/dgram.js

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

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 · 2018-07-12T23:38:04Z

@nodejs/dgram PTAL

vsemozhetbyt · 2018-07-17T15:39:37Z

Ping @nodejs/dgram

mcollina · 2018-07-17T17:00:31Z

lib/dgram.js

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.

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.

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.

mcollina · 2018-07-17T17:01:34Z

lib/dgram.js

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

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

cjihrig · 2018-07-18T03:45:03Z

lib/dgram.js

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

oyyd · 2018-07-24T07:05:29Z

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

mscdex · 2018-07-29T15:05:20Z

lib/dgram.js

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

mscdex · 2018-07-29T15:05:59Z

lib/dgram.js

The indentation is off here

mscdex · 2018-07-29T15:09:10Z

test/parallel/test-dgram-bind-fd.js

Why this timeout and the one below for the receiver?

Thanks. It's unnecessary here.

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 · 2018-07-30T13:57:00Z

@mscdex @cjihrig @mcollina PTAL

mcollina

LGTN

mscdex · 2018-07-30T14:22:03Z

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 · 2018-07-30T14:25:43Z

@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 · 2018-08-02T07:51:40Z

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

mcollina · 2018-08-03T12:32:17Z

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

mcollina · 2018-08-03T16:00:52Z

@nodejs/tsc PTAL, this need another LGTM.

jasnell · 2018-08-03T22:39:11Z

test/parallel/test-cluster-dgram-bind-fd.js

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

Trott · 2018-08-04T03:36:58Z

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

cjihrig

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

mcollina · 2018-08-06T07:23:28Z

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

(last before landing)

mcollina · 2018-08-06T09:37:54Z

Landed in 2bea9ce

mcollina · 2018-08-06T09:38:19Z

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

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 · 2018-08-06T09:55:18Z

@mcollina Thanks!

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 force-pushed the udp-fd branch 2 times, most recently from db24841 to 6d026ad Compare July 11, 2018 07:29

ChALkeR added the dgram Issues and PRs related to the dgram subsystem / UDP. label Jul 11, 2018

oyyd force-pushed the udp-fd branch from 6d026ad to 4d177b2 Compare July 11, 2018 07:46

mscdex reviewed Jul 11, 2018

View reviewed changes

Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 11, 2018

vsemozhetbyt reviewed Jul 11, 2018

View reviewed changes

BridgeAR reviewed Jul 12, 2018

View reviewed changes

oyyd force-pushed the udp-fd branch from 35f177b to bd13b2c Compare July 13, 2018 05:53

mcollina reviewed Jul 17, 2018

View reviewed changes

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

cjihrig reviewed Jul 18, 2018

View reviewed changes

oyyd force-pushed the udp-fd branch from c12c42d to 75b56e4 Compare July 29, 2018 14:44

mscdex reviewed Jul 29, 2018

View reviewed changes

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

mcollina approved these changes Jul 30, 2018

View reviewed changes

oyyd force-pushed the udp-fd branch from 21f3515 to 0705244 Compare August 2, 2018 06:19

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

jasnell reviewed Aug 3, 2018

View reviewed changes

jasnell approved these changes Aug 3, 2018

View reviewed changes

dgram: implement socket.bind({ fd })

ff8e9a3

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

oyyd force-pushed the udp-fd branch from 0705244 to ff8e9a3 Compare August 4, 2018 06:43

cjihrig approved these changes Aug 4, 2018

View reviewed changes

mcollina closed this Aug 6, 2018

GaryGSC mentioned this pull request Nov 14, 2019

[v10.x backport] dgram: add support for UDP connected sockets #30480

Closed

4 tasks

Uh oh!

Conversation

oyyd commented Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Jul 12, 2018

Uh oh!

vsemozhetbyt commented Jul 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oyyd commented Jul 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oyyd commented Jul 30, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mscdex commented Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Jul 30, 2018

Uh oh!

oyyd commented Aug 2, 2018

Uh oh!

mcollina commented Aug 3, 2018

Uh oh!

mcollina commented Aug 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Aug 4, 2018

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Aug 6, 2018

Uh oh!

mcollina commented Aug 6, 2018

Uh oh!

mcollina commented Aug 6, 2018

oyyd commented Jul 11, 2018 •

edited

Loading

mscdex commented Jul 30, 2018 •

edited

Loading