quic: share UDP code with dgram #165

addaleax · 2019-10-09T23:47:25Z

Make UDPWrap useable by other C++ classes, and use them for QUIC. This simplifies the code a bit, and will additionally enable us to test QUIC without actual networking usage (e.g. when reproducibility for a test is particularly important).

One commit is coming from upstream Node.js. Once everything is working, I’d expect this to increase performance a bit, too.

TODO:

Make all tests pass (currently a number of them time out)
Internal documentation for the UDPWrapBase / UDPListener interfaces
Test QUIC without actual network socket

addaleax · 2019-10-09T23:50:42Z

One of the testing possibilities I’d have in mind is #80 – if that’s something that depends on the order of packets arriving, then let’s control the order in which they arrive to test it :)

jasnell · 2019-10-10T00:36:40Z

Looking good so far!

addaleax · 2019-10-11T23:40:28Z

This is ready for review now, the testing parts should be able to follow later.

src/udp_wrap.h

src/js_udp_wrap.cc

This improves dgram performance by avoiding unnecessary async operations. One issue with this commit is that it seems hard to actually create conditions under which the fallback path to the async case is actually taken, for all supported OS, so an internal CLI option is used for testing that path. Another caveat is that the lack of an async operation means that there are slight timing differences (essentially `nextTick()` rather than `setImmediate()` for the send callback). PR-URL: nodejs/node#29832 Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

This allows keeping `BaseObjectPtr`s to `HandleWrap` instances. Previously, the pointer kept the `HandleWrap` object alive, leaving the Environment cleanup code that waits for the handle list to drain in a busy loop, because only the `HandleWrap` destructor removed the item from the list.

Allow using the handle more directly for I/O in other parts of the codebase.

This simplifies the code quite a bit.

This means that we always need to use `DeleteFnPtr` to make sure that the timer is actually cleaned up, but I guess that’s an acceptable restriction for now.

Keeping the JS object alive is not enough, because the per-Environment cleanup may destroy C++ objects even if their JS counterparts are still reachable.

Provide facilities to test `QUIC` sockets without an actual underlying UDP handle for more resilient tests.

addaleax · 2019-10-16T18:11:51Z

Sorry it took so long, but – Finally, it works 🎉

@danbev @jasnell Could you take another look? Here’s what’s changed since your reviews:

9cdf8d9 quic: allow .connect() when socket is already bound
- This is necessary for the test to work, because the socket can be considered bound immediately.
  Afaict, this should also allow calling .connect() on the client side multiple times, with multiple peers, and getting different sessions for them – that would be allowed and good, right?
5453076 squash! quic: allow testing QUIC without real UDP handle
- This adds an actual in-memory-QUIC test without any real I/O that can be used for more robust tests when the need arises.
33951a6 fixup! dgram: make UDPWrap more reusable
- [This is just addressing a review comment].
db35d5e fixup! quic: use UDP code from dgram
- Adjusts @oyyd’s test from test: add tests for quic ipv6Only #171 to account for the better error message that now includes the syscall name.

jasnell · 2019-10-16T18:22:00Z

Afaict, this should also allow calling .connect() on the client side multiple times, with multiple peers, and getting different sessions for them – that would be allowed and good, right?

I think that's definitely fine.

This adds an actual in-memory-QUIC test without any real I/O that can be used for more robust tests when the need arises.

🎉 💯 🥇

This allows keeping `BaseObjectPtr`s to `HandleWrap` instances. Previously, the pointer kept the `HandleWrap` object alive, leaving the Environment cleanup code that waits for the handle list to drain in a busy loop, because only the `HandleWrap` destructor removed the item from the list. PR-URL: #165 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>

Allow using the handle more directly for I/O in other parts of the codebase. PR-URL: #165 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>

Referring to `BaseObject` instances using standard C++ smart pointers can interfere with BaseObject’s own cleanup mechanisms (explicit delete, delete-on-GC and delete-on-cleanup). Introducing custom smart pointers allows referring to `BaseObject`s safely while keeping those mechanisms intact. Refs: nodejs/quic#141 Refs: nodejs/quic#149 Reviewed-By: James M Snell <[email protected]> PR-URL: nodejs#30374 Refs: nodejs/quic#165 Reviewed-By: David Carlier <[email protected]>

Refs: nodejs/quic#141 Reviewed-By: James M Snell <[email protected]> PR-URL: nodejs#30374 Refs: nodejs/quic#149 Refs: nodejs/quic#165 Reviewed-By: David Carlier <[email protected]>

PR-URL: nodejs#30374 Refs: nodejs/quic#141 Refs: nodejs/quic#149 Refs: nodejs/quic#141 Refs: nodejs/quic#165 Reviewed-By: James M Snell <[email protected]> Reviewed-By: David Carlier <[email protected]>

This is no longer necessary now that the copyable `BaseObjectPtr` is available (as opposed to the only-movable `v8::Global`). PR-URL: nodejs#30374 Refs: nodejs/quic#141 Refs: nodejs/quic#149 Refs: nodejs/quic#141 Refs: nodejs/quic#165 Reviewed-By: James M Snell <[email protected]> Reviewed-By: David Carlier <[email protected]>

This allows keeping `BaseObjectPtr`s to `HandleWrap` instances. Previously, the pointer kept the `HandleWrap` object alive, leaving the Environment cleanup code that waits for the handle list to drain in a busy loop, because only the `HandleWrap` destructor removed the item from the list. Refs: nodejs/quic#165 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> PR-URL: nodejs#30374 Refs: nodejs/quic#141 Refs: nodejs/quic#149 Refs: nodejs/quic#141 Reviewed-By: David Carlier <[email protected]>

Referring to `BaseObject` instances using standard C++ smart pointers can interfere with BaseObject’s own cleanup mechanisms (explicit delete, delete-on-GC and delete-on-cleanup). Introducing custom smart pointers allows referring to `BaseObject`s safely while keeping those mechanisms intact. Refs: nodejs/quic#141 Refs: nodejs/quic#149 Reviewed-By: James M Snell <[email protected]> Backport-PR-URL: #32301 PR-URL: #30374 Refs: nodejs/quic#165 Reviewed-By: David Carlier <[email protected]>

Refs: nodejs/quic#141 Reviewed-By: James M Snell <[email protected]> Backport-PR-URL: #32301 PR-URL: #30374 Refs: nodejs/quic#149 Refs: nodejs/quic#165 Reviewed-By: David Carlier <[email protected]>

Backport-PR-URL: #32301 PR-URL: #30374 Refs: nodejs/quic#141 Refs: nodejs/quic#149 Refs: nodejs/quic#141 Refs: nodejs/quic#165 Reviewed-By: James M Snell <[email protected]> Reviewed-By: David Carlier <[email protected]>

This is no longer necessary now that the copyable `BaseObjectPtr` is available (as opposed to the only-movable `v8::Global`). Backport-PR-URL: #32301 PR-URL: #30374 Refs: nodejs/quic#141 Refs: nodejs/quic#149 Refs: nodejs/quic#141 Refs: nodejs/quic#165 Reviewed-By: James M Snell <[email protected]> Reviewed-By: David Carlier <[email protected]>

This allows keeping `BaseObjectPtr`s to `HandleWrap` instances. Previously, the pointer kept the `HandleWrap` object alive, leaving the Environment cleanup code that waits for the handle list to drain in a busy loop, because only the `HandleWrap` destructor removed the item from the list. Refs: nodejs/quic#165 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Backport-PR-URL: #32301 PR-URL: #30374 Refs: nodejs/quic#141 Refs: nodejs/quic#149 Refs: nodejs/quic#141 Reviewed-By: David Carlier <[email protected]>

Allow using the handle more directly for I/O in other parts of the codebase. Originally landed in the QUIC repo Original review metadata: ``` PR-URL: nodejs/quic#165 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> ``` Signed-off-by: James M Snell <[email protected]> PR-URL: nodejs#31871 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

Allow using the handle more directly for I/O in other parts of the codebase. Originally landed in the QUIC repo Original review metadata: ``` PR-URL: nodejs/quic#165 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> ``` Signed-off-by: James M Snell <[email protected]> PR-URL: #31871 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

addaleax changed the title ~~quic: share UDP code with dgram~~ [WIP] quic: share UDP code with dgram Oct 9, 2019

addaleax force-pushed the udp-reusable branch from a684292 to 4de6721 Compare October 11, 2019 21:08

addaleax marked this pull request as ready for review October 11, 2019 23:40

addaleax force-pushed the udp-reusable branch from 7b1bef2 to 53d166c Compare October 11, 2019 23:40

addaleax mentioned this pull request Oct 12, 2019

test: add tests for quic ipv6Only #171

Closed

4 tasks

addaleax force-pushed the udp-reusable branch 2 times, most recently from 9377a99 to 70aa77d Compare October 14, 2019 16:24

jasnell changed the title ~~[WIP] quic: share UDP code with dgram~~ quic: share UDP code with dgram Oct 14, 2019

jasnell approved these changes Oct 14, 2019

View reviewed changes

danbev approved these changes Oct 15, 2019

View reviewed changes

src/udp_wrap.h Outdated Show resolved Hide resolved

src/js_udp_wrap.cc Outdated Show resolved Hide resolved

addaleax added 11 commits October 16, 2019 20:01

dgram: make UDPWrap more reusable

14f779a

Allow using the handle more directly for I/O in other parts of the codebase.

quic: use UDP code from dgram

cdfc843

This simplifies the code quite a bit.

quic: remove CleanupHook from Timer

bf291b6

This means that we always need to use `DeleteFnPtr` to make sure that the timer is actually cleaned up, but I guess that’s an acceptable restriction for now.

quic: use BaseObjectPtr to keep WriteWrap alive

bbd5631

Keeping the JS object alive is not enough, because the per-Environment cleanup may destroy C++ objects even if their JS counterparts are still reachable.

quic: allow .connect() when socket is already bound

9cdf8d9

quic: allow testing QUIC without real UDP handle

4e70ee0

Provide facilities to test `QUIC` sockets without an actual underlying UDP handle for more resilient tests.

squash! quic: allow testing QUIC without real UDP handle

5453076

fixup! dgram: make UDPWrap more reusable

33951a6

fixup! quic: use UDP code from dgram

db35d5e

addaleax force-pushed the udp-reusable branch from 70aa77d to db35d5e Compare October 16, 2019 18:07

jasnell approved these changes Oct 16, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

quic: share UDP code with dgram #165

quic: share UDP code with dgram #165

addaleax commented Oct 9, 2019 •

edited

Loading

addaleax commented Oct 9, 2019

jasnell commented Oct 10, 2019

addaleax commented Oct 11, 2019

addaleax commented Oct 16, 2019

jasnell commented Oct 16, 2019

quic: share UDP code with dgram #165

quic: share UDP code with dgram #165

Conversation

addaleax commented Oct 9, 2019 • edited Loading

addaleax commented Oct 9, 2019

jasnell commented Oct 10, 2019

addaleax commented Oct 11, 2019

addaleax commented Oct 16, 2019

jasnell commented Oct 16, 2019

addaleax commented Oct 9, 2019 •

edited

Loading