Skip to content
This repository has been archived by the owner on Aug 11, 2020. It is now read-only.

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

Open
wants to merge 119 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 119 commits into from

Conversation

oyyd
Copy link
Contributor

@oyyd oyyd commented Nov 19, 2019

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

addaleax and others added 30 commits October 2, 2019 16:18
@addaleax @jasnell
src: introduce custom smart pointers for BaseObjects
2303d02 
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 <jasnell@gmail.com>
@addaleax @jasnell
http2: use custom BaseObject smart pointers
ba1c9a3 
PR-URL: nodejs#141
Reviewed-By: James M Snell <jasnell@gmail.com>
@danbev @addaleax
@gengjiawen @jasnell @LPardue
quic: initial protocol implementation
84d86f3 
Co-authored-by: Anna Henningsen <anna@addaleax.net>
Co-authored-by: Daniel Bevenius <daniel.bevenius@gmail.com>
Co-authored-by: gengjiawen <technicalcute@gmail.com>
Co-authored-by: James M Snell <jasnell@gmail.com>
Co-authored-by: Lucas Pardue <lucaspardue.24.7@gmail.com>
Co-authored-by: oyyd <oyydoibh@gmail.com
@addaleax @jasnell
http2: use shared memory tracking implementation
b2d2ca4 
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 <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
deps: Add interface required to implement QUIC draft-17
170c5d0 
Ported from
tatsuhiro-t/openssl@920a331

PR-URL: nodejs#6
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@jasnell
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 <daniel.bevenius@gmail.com>
@addaleax @jasnell
src: allow unique_ptrs with custom deleter in memory tracker
963d54c 
PR-URL: nodejs#145
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax @jasnell
quic: support memory tracking
4835a89 
Fixes: nodejs#59
PR-URL: nodejs#145
Reviewed-By: James M Snell <jasnell@gmail.com>
@danbev @jasnell
doc: fix lint issues in quic.md
c9388c9 
PR-URL: nodejs#147
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@addaleax @jasnell
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 <jasnell@gmail.com>
@addaleax @jasnell
lib: fix typo in stream_base_commons.js
b587abb 
Introduced in aa99a6a.

PR-URL: nodejs#149
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax @jasnell
test: fix test-process-versions for QUIC
40e5be0 
PR-URL: nodejs#149
Reviewed-By: James M Snell <jasnell@gmail.com>
@trivikr
quic: test client connect invalid options
1cae853 
PR-URL: nodejs#154
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@trivikr
quic: test invalid createSocket
746a888 
PR-URL: nodejs#153
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@trivikr
quic: default options parameter in constructor
92c25a6 
PR-URL: nodejs#152
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@addaleax
quic: compute retransmission timeout correctly
70a65d3 
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 <jasnell@gmail.com>
@oyyd @jasnell
quic: client should work when receives invalid preferred address
d9ee6ab 
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 <jasnell@gmail.com>
@oyyd @jasnell
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 <jasnell@gmail.com>
@addaleax @jasnell
test: make test-quic-process-cleanup work with --worker
777d93f 
PR-URL: nodejs#159
Reviewed-By: James M Snell <jasnell@gmail.com>
@vtjnash @jasnell
deps: cherry-pick "unix,udp: ensure addr is non-null"
da3a546 
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é <s@saghul.net>
@oyyd @jasnell
deps: cherry-pick "unix: set sin_len and sin6_len"
5e76d74 
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 <anna@addaleax.net>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <s@saghul.net>
@jasnell
Revert "deps: Add interface required to implement QUIC draft-17"
08479f4 
This reverts commit 170c5d0.

PR-URL: nodejs#138
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell
Revert "deps: Remove EOED when SSL_MODE_QUIC_HACK is enabled"
c37c251 
This reverts commit ecda77c.

PR-URL: nodejs#138
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell
deps: Port BoringSSL QUIC APIs
fe3b34c 
PR-URL: nodejs#138
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell
deps: update OpenSSL config
53f668e 
PR-URL: nodejs#138
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell
deps: update ngtcp2
59d96d1 
PR-URL: nodejs#138
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell
[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 <anna@addaleax.net>
@jasnell
quic: move to ngtcp2_crypto
1734bd0 
PR-URL: nodejs#138
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@addaleax
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
@oyyd @addaleax
quic: fix idle timeout
6b97000 
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 <anna@addaleax.net>
jasnell and others added 8 commits December 4, 2019 11:05
@jasnell
quic: additional doc updates
4e9a96e 
PR-URL: nodejs#213
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell
quic: use const refs
02e5389 
PR-URL: nodejs#216
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell
quic: more idiomatic operators
08bb8c9 
PR-URL: nodejs#216
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell
quic: stateless reset generate strategy
f5ae4f7 
Generate stateless reset token cryptographically

Fixes: nodejs#62
PR-URL: nodejs#215
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell
quic: simplify and condense
23675f2 
PR-URL: nodejs#217
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell
quic: minor cleanups
700ab33 
PR-URL: nodejs#217
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell
quic: get name and value as std::string option
5500d55 
PR-URL: nodejs#217
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@danbev @jasnell
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 <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@oyyd oyyd force-pushed the connect-ready branch 2 times, most recently from 21377c1 to be8cf91 Compare December 9, 2019 07:45
@oyyd
quic: ensure callbacks of QuicSocket.connect() get called
431bce9 
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 oyyd changed the title WIP quic: ensure callbacks of QuicSocket.connect() get called quic: ensure callbacks of QuicSocket.connect() get called Dec 9, 2019
@oyyd
Copy link
Contributor Author

oyyd commented Dec 9, 2019

Also cc @jasnell @danbev for more reviews.

@jasnell
Copy link
Member

jasnell commented Dec 13, 2019

Heads up, this will need to be rebased

@jasnell jasnell force-pushed the master branch 3 times, most recently from 76d2afc to b69367d Compare February 13, 2020 19:39
@jasnell jasnell force-pushed the master branch 4 times, most recently from 928c50e to 08796cc Compare March 17, 2020 16:23
@jasnell jasnell force-pushed the master branch 2 times, most recently from 78e9941 to cee2e5d Compare March 24, 2020 01:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@oyyd @jasnell @addaleax @danbev @trivikr @vtjnash @Fishrock123 @juanarbol @lundibundi