quic: ensure callbacks of QuicSocket.connect() get called #198

oyyd · 2019-11-19T12:16:24Z

The callbacks of QuicSocket.connect() won't get called after
QuicSocket binding. This PR calls QuicSession[kReady]() directly
when QuicSocket bound to fix this issue.

This PR also modify SocketAddress::Hash and SocketAddress::Compare
to accept values instead of pointers because SocketAddress might
get freed firstly which would cause that the values aren't safe.
For example, the test added is likely to abort before this PR.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

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. PR-URL: nodejs#141 Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs#141 Reviewed-By: James M Snell <[email protected]>

Co-authored-by: Anna Henningsen <[email protected]> Co-authored-by: Daniel Bevenius <[email protected]> Co-authored-by: gengjiawen <[email protected]> Co-authored-by: James M Snell <[email protected]> Co-authored-by: Lucas Pardue <[email protected]> Co-authored-by: oyyd <[email protected]

The extensive testing done on http2 makes it easier to make sure the implementation is correct (and doesn’t diverge unnecessarily). PR-URL: nodejs#126 Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: James M Snell <[email protected]>

Ported from tatsuhiro-t/openssl@920a331 PR-URL: nodejs#6 Reviewed-By: Daniel Bevenius <[email protected]>

PR-URL: nodejs#145 Reviewed-By: James M Snell <[email protected]>

Fixes: nodejs#59 PR-URL: nodejs#145 Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs#147 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

This fixes a crash in test/sequential/test-performance-eventloopdelay.js. This should be squashed into > src: introduce custom smart pointers for `BaseObject`s PR-URL: nodejs#149 Reviewed-By: James M Snell <[email protected]>

Introduced in aa99a6a. PR-URL: nodejs#149 Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs#149 Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs#154 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>

PR-URL: nodejs#153 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>

PR-URL: nodejs#152 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>

Previously, if the expiry timestamp was in the future by less than 1ms, the retransmission timer would not have been scheduled. (This also removes an unused line from the test that this problem made flaky.) PR-URL: nodejs#157 Reviewed-By: James M Snell <[email protected]>

If we set `preferredAddressPolicy` to `accept` on `QuicClientSession` but receives an invalid preferred address, e.g. we don't define `preferredAddress` on the server, the `QuicClientSession` should still work. Before this PR, the included test will cause segmetation fault. PR-URL: nodejs#155 Reviewed-By: James M Snell <[email protected]>

It would be better to add a test to ensure that the timeout is greater than the triple PTO if we can get the PTO. PR-URL: nodejs#160 Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs#159 Reviewed-By: James M Snell <[email protected]>

Update this code to be more in line with the Windows version. Previously, callers (such as nodejs) might assume it was always safe to de-reference this parameter: https://github.com/nodejs/node/blob/d2634be56258e2b957c1061c5f4d86792975bfa9/src/tcp_wrap.cc#L349 called from https://github.com/nodejs/node/blob/d2634be56258e2b957c1061c5f4d86792975bfa9/src/udp_wrap.cc#L567 I suspect this may have been unreachable, and that it was guaranteed by the kernel to be `>= sizeof(struct sockaddr)`, so this should be a NFC simplification. PR-URL: libuv/libuv#2330 Reviewed-By: Saúl Ibarra Corretgé <[email protected]>

FreeBSD defines `sin_len` and `sin6_len` inside `sockaddr_in` and `sockaddr_in6`. `sockaddr`s come from `getsockname` and `uv_ip4_addr` will differ in the first byte if libuv doesn't set `sin_len` correctly. PR-URL: libuv/libuv#2492 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Saúl Ibarra Corretgé <[email protected]>

This reverts commit 170c5d0. PR-URL: nodejs#138 Reviewed-By: Anna Henningsen <[email protected]>

This reverts commit ecda77c. PR-URL: nodejs#138 Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: nodejs#138 Reviewed-By: Anna Henningsen <[email protected]>

This commit will need to be submitted upstream then backed out once it lands and we can update PR-URL: nodejs#138 Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: nodejs#138 Reviewed-By: Anna Henningsen <[email protected]>

The recent commits have broken the build, tests, and linting. One cctest is not fixed but rather disabled here. PR-URL: nodejs#162

After ngtcp2 being updated, `ngtcp2_conn_get_idle_timeout` is renamed as `ngtcp2_conn_get_idle_expiry` which returns `ngtcp2_tstamp` instead of `ngtcp2_duration`. Refs: https://github.com/ngtcp2/ngtcp2/blob/6f40668cdce7db7c043d3a80c07f379841d8c51e/lib/ngtcp2_conn.c#L8604 PR-URL: nodejs#166 Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: nodejs#213 Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: nodejs#216 Reviewed-By: Anna Henningsen <[email protected]>

Generate stateless reset token cryptographically Fixes: nodejs#62 PR-URL: nodejs#215 Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: nodejs#217 Reviewed-By: Anna Henningsen <[email protected]>

Currently the doc target fails and this commit attempts for fix these errors. PR-URL: nodejs#221 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

1. The callbacks of QuicSocket.connect() won't get called after QuicSocket binding. To fix this issue, This PR calls QuicSession[kReady]() directly when QuicSocket bound. 2. This PR also modify SocketAddress::Hash and SocketAddress::Compare to accept struct values instead of pointers because SocketAddress might get freed firstly which would cause the values aren't safe to use. For example, the test added in this PR is likely to abort before this PR.

oyyd · 2019-12-09T08:04:22Z

Also cc @jasnell @danbev for more reviews.

jasnell · 2019-12-13T18:42:26Z

Heads up, this will need to be rebased

addaleax and others added 30 commits October 2, 2019 16:18

http2: use custom BaseObject smart pointers

ba1c9a3

PR-URL: nodejs#141 Reviewed-By: James M Snell <[email protected]>

deps: Add interface required to implement QUIC draft-17

170c5d0

Ported from tatsuhiro-t/openssl@920a331 PR-URL: nodejs#6 Reviewed-By: Daniel Bevenius <[email protected]>

deps: Remove EOED when SSL_MODE_QUIC_HACK is enabled

ecda77c

Ported from tatsuhiro-t/openssl@920a331 PR-URL: nodejs#6 Reviewed-By: Daniel Bevenius <[email protected]>

src: allow unique_ptrs with custom deleter in memory tracker

963d54c

PR-URL: nodejs#145 Reviewed-By: James M Snell <[email protected]>

quic: support memory tracking

4835a89

Fixes: nodejs#59 PR-URL: nodejs#145 Reviewed-By: James M Snell <[email protected]>

doc: fix lint issues in quic.md

c9388c9

PR-URL: nodejs#147 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

src: fix BaseObject Detach for GC’ed objects

07bdfdf

This fixes a crash in test/sequential/test-performance-eventloopdelay.js. This should be squashed into > src: introduce custom smart pointers for `BaseObject`s PR-URL: nodejs#149 Reviewed-By: James M Snell <[email protected]>

lib: fix typo in stream_base_commons.js

b587abb

Introduced in aa99a6a. PR-URL: nodejs#149 Reviewed-By: James M Snell <[email protected]>

test: fix test-process-versions for QUIC

40e5be0

PR-URL: nodejs#149 Reviewed-By: James M Snell <[email protected]>

quic: test client connect invalid options

1cae853

PR-URL: nodejs#154 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>

quic: test invalid createSocket

746a888

PR-URL: nodejs#153 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>

quic: default options parameter in constructor

92c25a6

PR-URL: nodejs#152 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>

test: add tests for quic idleTimeout

81e9492

It would be better to add a test to ensure that the timeout is greater than the triple PTO if we can get the PTO. PR-URL: nodejs#160 Reviewed-By: James M Snell <[email protected]>

test: make test-quic-process-cleanup work with --worker

777d93f

PR-URL: nodejs#159 Reviewed-By: James M Snell <[email protected]>

Revert "deps: Add interface required to implement QUIC draft-17"

08479f4

This reverts commit 170c5d0. PR-URL: nodejs#138 Reviewed-By: Anna Henningsen <[email protected]>

Revert "deps: Remove EOED when SSL_MODE_QUIC_HACK is enabled"

c37c251

This reverts commit ecda77c. PR-URL: nodejs#138 Reviewed-By: Anna Henningsen <[email protected]>

deps: Port BoringSSL QUIC APIs

fe3b34c

PR-URL: nodejs#138 Reviewed-By: Anna Henningsen <[email protected]>

deps: update OpenSSL config

53f668e

PR-URL: nodejs#138 Reviewed-By: Anna Henningsen <[email protected]>

deps: update ngtcp2

59d96d1

PR-URL: nodejs#138 Reviewed-By: Anna Henningsen <[email protected]>

[Upstream] deps: add handling for x509 and client hello cb

abd456b

This commit will need to be submitted upstream then backed out once it lands and we can update PR-URL: nodejs#138 Reviewed-By: Anna Henningsen <[email protected]>

quic: move to ngtcp2_crypto

1734bd0

PR-URL: nodejs#138 Reviewed-By: Anna Henningsen <[email protected]>

quic: fix build and tests

968a23c

The recent commits have broken the build, tests, and linting. One cctest is not fixed but rather disabled here. PR-URL: nodejs#162

jasnell and others added 8 commits December 4, 2019 11:05

quic: additional doc updates

4e9a96e

PR-URL: nodejs#213 Reviewed-By: Anna Henningsen <[email protected]>

quic: use const refs

02e5389

PR-URL: nodejs#216 Reviewed-By: Anna Henningsen <[email protected]>

quic: more idiomatic operators

08bb8c9

PR-URL: nodejs#216 Reviewed-By: Anna Henningsen <[email protected]>

quic: stateless reset generate strategy

f5ae4f7

Generate stateless reset token cryptographically Fixes: nodejs#62 PR-URL: nodejs#215 Reviewed-By: Anna Henningsen <[email protected]>

quic: simplify and condense

23675f2

PR-URL: nodejs#217 Reviewed-By: Anna Henningsen <[email protected]>

quic: minor cleanups

700ab33

PR-URL: nodejs#217 Reviewed-By: Anna Henningsen <[email protected]>

quic: get name and value as std::string option

5500d55

PR-URL: nodejs#217 Reviewed-By: Anna Henningsen <[email protected]>

doc,tools: fix make doc target for quic.md

1c316ac

Currently the doc target fails and this commit attempts for fix these errors. PR-URL: nodejs#221 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

oyyd force-pushed the connect-ready branch 2 times, most recently from 21377c1 to be8cf91 Compare December 9, 2019 07:45

oyyd force-pushed the connect-ready branch from be8cf91 to 431bce9 Compare December 9, 2019 07:46

oyyd changed the title ~~WIP quic: ensure callbacks of QuicSocket.connect() get called~~ quic: ensure callbacks of QuicSocket.connect() get called Dec 9, 2019

addaleax force-pushed the master branch from b3b9c0e to 3bd75b5 Compare December 11, 2019 19:03

jasnell force-pushed the master branch from cf320b5 to 04c4af9 Compare December 13, 2019 18:23

jasnell force-pushed the master branch from 4686b28 to 7ba53ab Compare February 3, 2020 19:28

jasnell force-pushed the master branch 3 times, most recently from 76d2afc to b69367d Compare February 13, 2020 19:39

jasnell force-pushed the master branch 4 times, most recently from 928c50e to 08796cc Compare March 17, 2020 16:23

jasnell force-pushed the master branch 2 times, most recently from 78e9941 to cee2e5d Compare March 24, 2020 01:14

jasnell force-pushed the master branch from cee2e5d to da5f169 Compare April 7, 2020 20:37

jasnell force-pushed the master branch from e782ca4 to bc1b2c0 Compare May 7, 2020 23:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

quic: ensure callbacks of QuicSocket.connect() get called #198

quic: ensure callbacks of QuicSocket.connect() get called #198

oyyd commented Nov 19, 2019

oyyd commented Dec 9, 2019

jasnell commented Dec 13, 2019

quic: ensure callbacks of QuicSocket.connect() get called #198

Are you sure you want to change the base?

quic: ensure callbacks of QuicSocket.connect() get called #198

Conversation

oyyd commented Nov 19, 2019

Checklist

oyyd commented Dec 9, 2019

jasnell commented Dec 13, 2019