Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.4k
dgram: add support for UDP connected sockets#26871
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot commented Mar 22, 2019
santigimeno commented Mar 22, 2019
/cc @cjihrig |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
reklatsmasters commented Mar 24, 2019
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 |
santigimeno commented Mar 24, 2019
@reklatsmasters I think that's actually how a connected UDP socket behaves (and tested locally in linux). From the linux connect(2) manual:
|
reklatsmasters commented Mar 24, 2019
@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. |
santigimeno commented Mar 25, 2019
Updated to add the review comments. Thanks! |
nodejs-github-bot commented Mar 27, 2019
vsemozhetbyt left a comment
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
santigimeno commented Mar 29, 2019 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Updated addressing docs comments. Thanks |
targos commented Mar 30, 2019
this needs to be rebased. /cc @nodejs/dgram |
santigimeno commented Mar 31, 2019
Rebased to current master. |
nodejs-github-bot commented Apr 1, 2019
danbev commented Apr 1, 2019
Re-run of failing node-test-commit-linux |
mcollina left a comment
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
danbev commented Apr 2, 2019
test/osx failure looks unrelated08:22:28 not ok 386 parallel/test-dgram-connect08:22:28 ---08:22:28 duration_ms: 0.7608:22:28 severity: fail08:22:28 exitcode: 108:22:28 stack: |-08:22:28 assert.js:8708: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 - expected08: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:1208:22:28 at /home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1804-64/test/common/index.js:369:1508:22:28 at dgram.js:408:908: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.
santigimeno commented Apr 2, 2019
Updated with a test fixup. Thanks! |
nodejs-github-bot commented Apr 2, 2019
danbev commented Apr 3, 2019
Re-build of failing node-test-commit-linux (✔️) |
danbev commented Apr 3, 2019
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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
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]>
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)
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:
disconnectevent be added?ENOTCONNandEISCONNerrors. Should I be emitting an event instead?send()is now more overloaded than ever.Thanks