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.

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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

GaryGSC
Copy link
Contributor

@GaryGSC GaryGSC 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 cjihrig@gmail.com
Reviewed-By: Matteo Collina matteo.collina@gmail.com

(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-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. v10.x labels Nov 14, 2019
@GaryGSC
Copy link
Contributor Author

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 <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

(cherry picked from commit 9e96017)
@nodejs-github-bot

This comment has been minimized.

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

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

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

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

@GaryGSC
Copy link
Contributor Author

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

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.

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

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

@addaleax addaleax 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-actions github-actions bot 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

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

I would say no.

@GaryGSC
Copy link
Contributor Author

GaryGSC commented Jun 25, 2020

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

@mcollina mcollina closed this Jun 25, 2020
Sign up for free to 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 / 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
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