Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
GaryGSC wants to merge 1 commit into from
Closed

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

GaryGSC wants to merge 1 commit into from

Conversation

@GaryGSC
Copy link
Contributor

@GaryGSCGaryGSC commented Nov 14, 2019

Added the dgram.connect() and dgram.disconnect() methods that
associate/disassociate a udp socket to/from a remote address.
It optimizes for cases where lots of packets are sent to the same
address.
Also added the dgram.remoteAddress() method to retrieve the associated
remote address.

PR-URL: #26871
Reviewed-By: Colin Ihrig [email protected]
Reviewed-By: Matteo Collina [email protected]

(cherry picked from commit 9e96017)

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

As an aside, this is only a backport of #26871. That PR had a backport-requested-v10.x label. Does this backport even make sense without the semver-major #21745? I don't know enough about UDP. @nodejs/dgram

@nodejs-github-botnodejs-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. v10.x labels Nov 14, 2019
@GaryGSCGaryGSCforce-pushed the backport-26871-to-v10.x branch from e07df6f to 3cd4ea5CompareNovember 14, 2019 07:18
@GaryGSCGaryGSCforce-pushed the backport-26871-to-v10.x branch from 3cd4ea5 to f0f3a11CompareNovember 14, 2019 07:24
@GaryGSCGaryGSCforce-pushed the backport-26871-to-v10.x branch from f0f3a11 to aad1844CompareNovember 14, 2019 07:26
@GaryGSC
Copy link
ContributorAuthor

GaryGSC commented Nov 14, 2019

Sorry for the churn there. I realized I made some tiny mistakes.

Could someone please start the node-test-pull-request CI job?

@santigimeno@GaryGSC
dgram: add support for UDP connected sockets
458f017
Added the `dgram.connect()` and `dgram.disconnect()` methods that associate/disassociate a udp socket to/from a remote address. It optimizes for cases where lots of packets are sent to the same address. Also added the `dgram.remoteAddress()` method to retrieve the associated remote address. Backport-PR-URL: nodejs#30480 PR-URL: nodejs#26871 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Matteo Collina <[email protected]> (cherry picked from commit 9e96017)
@GaryGSCGaryGSCforce-pushed the backport-26871-to-v10.x branch from aad1844 to 458f017CompareNovember 14, 2019 19:02
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 2, 2019

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

@BethGriggsBethGriggs added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 3, 2019
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 3, 2019

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

@GaryGSC
Copy link
ContributorAuthor

GaryGSC commented Dec 5, 2019

This error looks related. It's only on osx1010. (30391/30398)

03:38:04 not ok 745 parallel/test-dgram-connect-send-empty-packet 03:38:04 --- 03:38:04 duration_ms: 120.31 03:38:04 severity: fail 03:38:04 exitcode: -15 03:38:04 stack: |- 03:38:04 timeout 03:38:04 ...

@addaleax
Copy link
Member

addaleax commented Dec 5, 2019

Is that maybe #30030 rather than this backport?

@richardlau
Copy link
Member

richardlau commented Dec 5, 2019

Is that maybe #30030 rather than this backport?

AFAIK #30030 is a regression on macOS Catalina (10.15) which we don't have in our CI.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 25, 2020•
edited by BethGriggs
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/29339/
Only OS X failed on parallel/test-dgram-connect-send-empty-packet, potentially related.

@BethGriggsBethGriggs added the needs-ci PRs that need a full CI run. label Mar 9, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 12, 2020

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

@addaleaxaddaleax removed the needs-ci PRs that need a full CI run. label Jun 20, 2020
This was referenced Jun 21, 2020
Open
Open
Open
Open
@github-actionsgithub-actionsbot mentioned this pull request Jun 25, 2020
Open
14 tasks
@jasnell
Copy link
Member

jasnell commented Jun 25, 2020

ping @nodejs/releasers ... should this land in v10x at this point?

@richardlau
Copy link
Member

richardlau commented Jun 25, 2020

ping @nodejs/releasers ... should this land in v10x at this point?

cc @nodejs/lts Given that 10.x is now in maintenance and there's the uncertainty from the OP about whether it makes sense without a semver-major change (which obviously will not be backported) I'm leaning towards "no".

As an aside, this is only a backport of #26871. That PR had a backport-requested-v10.x label. Does this backport even make sense without the semver-major #21745? I don't know enough about UDP. @nodejs/dgram

@mcollina
Copy link
Member

mcollina commented Jun 25, 2020

I would say no.

@GaryGSC
Copy link
ContributorAuthor

GaryGSC commented Jun 25, 2020

I'm also leaning towards no. Feel free to close. 🙂

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.lib / srcIssues and PRs related to general changes in the lib or src directory.semver-minorPRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@GaryGSC@nodejs-github-bot@addaleax@richardlau@jasnell@mcollina@BethGriggs@santigimeno