dgram: add support for UDP connected sockets by santigimeno · Pull Request #26871 · nodejs/node (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation43 Commits2 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes- tests and/or benchmarks are included
- documentation is changed or added
- commit message follows commit guidelines
Would like some feedback as I'm not sure about some decisions I made:
- Should a
disconnectevent be added? - I'm throwing on the
ENOTCONNandEISCONNerrors. Should I be emitting an event instead? - The
send()is now more overloaded than ever.
Thanks
Issues and PRs that require attention from people who are familiar with C++.
Issues and PRs related to general changes in the lib or src directory.
labels
Issues and PRs related to the dgram subsystem / UDP.
label
I think this API shouldn't be a part of nodejs core. There may be some misunderstandings. Some developers may expect messages only from a connected socket, like in tcp, but it's not. You may receive messages from anyone but may send only to an associated address. It's important.
Almost a year ago i solved this problem by creating unicast. This module provided real 1:1 association (for incoming and outgoing messages) and streaming interface.
@reklatsmasters I think that's actually how a connected UDP socket behaves (and tested locally in linux). From the linux connect(2) manual:
If the socket sockfd is of type SOCK_DGRAM, then addr is the address to which datagrams are sent by default, and the only address from which datagrams are received.
@santigimeno Thanks for the explanation! I researched only the libuv sources.
It would be great to implement a connected UDP socket like a duplex stream.
Updated to add the review comments. Thanks!
PRs that have at least one approval, no pending requests for changes, and a CI started.
label
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some doc nits.
Updated addressing docs comments. Thanks
this needs to be rebased.
/cc @nodejs/dgram
PRs that have at least one approval, no pending requests for changes, and a CI started.
label
Rebased to current master.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
PRs that have at least one approval, no pending requests for changes, and a CI started.
label
test/parallel/test-dgram-connect.js seems to be consistently failing on ubuntu.
test/osx failure looks unrelated
08:22:28 not ok 386 parallel/test-dgram-connect
08:22:28 ---
08:22:28 duration_ms: 0.76
08:22:28 severity: fail
08:22:28 exitcode: 1
08:22:28 stack: |-
08:22:28 assert.js:87
08:22:28 throw new AssertionError(obj);
08:22:28 ^
08:22:28
08:22:28 AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
08:22:28 + actual - expected
08:22:28
08:22:28 + 'EAI_AGAIN'
08:22:28 - 'ENOTFOUND'
08:22:28 at /home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1804-64/test/parallel/test-dgram-connect.js:40:12
08:22:28 at /home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1804-64/test/common/index.js:369:15
08:22:28 at dgram.js:408:9
08:22:28 at processTicksAndRejections (internal/process/task_queues.js:79:9)
08:22:28 ...
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.
Updated with a test fixup. Thanks!
This was referenced
Apr 2, 2019
danbev pushed a commit that referenced this pull request
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
BethGriggs pushed a commit that referenced this pull request
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
This was referenced
Apr 23, 2019
PRs that contain new features and should be released in the next minor version.
label
GaryGSC pushed a commit to GaryGSC/node that referenced this pull request
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)
Labels
PRs that have at least one approval, no pending requests for changes, and a CI started.
Issues and PRs that require attention from people who are familiar with C++.
Issues and PRs related to the dgram subsystem / UDP.
Issues and PRs related to general changes in the lib or src directory.
PRs that contain new features and should be released in the next minor version.