-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: add support for UDP connected sockets #26871
Conversation
/cc @cjihrig |
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 |
@reklatsmasters I think that's actually how a connected UDP socket behaves (and tested locally in linux). From the linux connect(2) manual:
|
@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. |
075bbc1
to
f9325cf
Compare
Updated to add the review comments. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some doc nits.
f9325cf
to
69c6a8c
Compare
Updated addressing docs comments. Thanks |
this needs to be rebased. /cc @nodejs/dgram |
69c6a8c
to
8dbc438
Compare
Rebased to current master. |
Re-run of failing node-test-commit-linux |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
test/osx failure looks unrelated08: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! |
Re-build of failing node-test-commit-linux (✔️) |
Landed in 9e96017. |
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>
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>
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)
Added the
dgram.connect()
anddgram.disconnect()
methods thatassociate/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 associatedremote address.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesWould like some feedback as I'm not sure about some decisions I made:
disconnect
event be added?ENOTCONN
andEISCONN
errors. Should I be emitting an event instead?send()
is now more overloaded than ever.Thanks