quic: continued refactoring of quic APIs #34351

jasnell · 2020-07-14T03:29:35Z

@nodejs/quic ... this one includes a fairly extensive reworking of the QuicStream lifecycle, including being able to enable autoDestroy, eliminating the 'abort' event and the aborted property, fixing up a number of bugs and internal timing issues. There's still a lot to do to validate protocol, timing, and error handling, but this is a major step in the right direction.

/cc @ronag

Checklist

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

nodejs-github-bot · 2020-07-16T00:24:37Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32365/
QUIC CI: https://ci.nodejs.org/job/node-test-commit/39670/

nodejs-github-bot · 2020-07-16T04:57:48Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32368/

jasnell · 2020-07-16T04:58:41Z

Moving back to a draft, several failures on POSIX systems...

lib/internal/quic/core.js

nodejs-github-bot · 2020-07-17T15:38:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32388/
QUIC CI: https://ci.nodejs.org/job/node-test-commit/39702/

jasnell · 2020-07-17T23:51:40Z

Moving back to draft to work on the changes @ronag suggested for using an async _construct

jasnell · 2020-07-21T22:19:29Z

@ronag... so just to follow up on this. I've gone ahead and made the change to use async _construct(), however, it really does not improve the ergonomics of the API at all because there are a number of properties and methods that are only meaningful/useful after the internal handle is created. What we end up with is, instead of...

const stream = await session.openStream();
console.log(stream.unidirectional);

We end up having to do...

const stream = session.openStream();
await once(stream, 'ready');
console.log(stream.unidirectional);

The additional methods for sending headers, getting the id and directionality of the stream, etc, all end up having to wait until after the ready event anyway. We could buffer the method calls and make those wait on 'ready' internally but it just adds complexity.

I'm really preferring the const stream = await session.openStream() variation that this PR implemented originally.

ronag · 2020-07-22T08:06:08Z

I'm really preferring the const stream = await session.openStream() variation that this PR implemented originally.

Yea that does seem simpler. Just feels a little different than the rest of our API's (maybe calling it connect would make it more familiar). Being able to start writing to the stream without having to await is kind of nice but not that important I guess.

jasnell · 2020-07-22T14:47:14Z

Yea that does seem simpler. Just feels a little different than the rest of our API's (maybe calling it connect would make it more familiar). Being able to start writing to the stream without having to await is kind of nice but not that important I guess.

connect is already used on the QuicSocket to create the client-side QuicSession instance. In this case, openStream() is exactly descriptive of what it does.

The move to the async _construct() is definitely good to do, but I will make it so that openStream() returns Promise that resolves when the async _construct() resolves.

jasnell · 2020-07-22T16:18:14Z

Ok, @nodejs/quic .. this should finally be ready for review

src/quic/node_quic_session.cc

lib/internal/quic/core.js

nodejs-github-bot · 2020-07-22T22:45:54Z

Resumed due to flaky failures...

CI: https://ci.nodejs.org/job/node-test-pull-request/32479/
QUIC CI: https://ci.nodejs.org/job/node-test-commit/39807/

Although most of the time openStream will be able to create the stream immediately, when a stream is opened before the handshake is complete we have to wait for the handshake to be complete before continuing.

Removing no longer needed code

qlog files are diagnostic files that are being used to verify the quic implementation. Make sure they don't get checked in.

This one was a bit of a rabbit hole... but, with this set of changes, `QuicStream` should now work with autoDestroy, supports a promisified `close()`, and fixes a number of other internal bugs that were spotted trying to get it to work.

Although most of the time openStream will be able to create the stream immediately, when a stream is opened before the handshake is complete we have to wait for the handshake to be complete before continuing. PR-URL: #34351 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

PR-URL: #34351 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Removing no longer needed code PR-URL: #34351 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

PR-URL: #34351 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

qlog files are diagnostic files that are being used to verify the quic implementation. Make sure they don't get checked in. PR-URL: #34351 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

This one was a bit of a rabbit hole... but, with this set of changes, `QuicStream` should now work with autoDestroy, supports a promisified `close()`, and fixes a number of other internal bugs that were spotted trying to get it to work. PR-URL: #34351 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

PR-URL: #34351 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

jasnell · 2020-07-23T13:55:13Z

Landed in 69c78be...b06fe33

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jul 14, 2020

jasnell force-pushed the quic-cleanups-7 branch from 6adf667 to aa6217e Compare July 16, 2020 00:21

jasnell marked this pull request as ready for review July 16, 2020 00:21

jasnell requested a review from a team July 16, 2020 00:21

jasnell marked this pull request as draft July 16, 2020 04:58

ronag reviewed Jul 16, 2020

View reviewed changes

lib/internal/quic/core.js Outdated Show resolved Hide resolved

ronag reviewed Jul 16, 2020

View reviewed changes

lib/internal/quic/core.js Outdated Show resolved Hide resolved

jasnell marked this pull request as ready for review July 16, 2020 23:34

This comment has been minimized.

Sign in to view

github-actions bot mentioned this pull request Jul 17, 2020

CI Relability 2020-07-17 JaneaSystems/nodejs-reliability#40

Open

14 tasks

jasnell marked this pull request as draft July 17, 2020 23:51

This was referenced Jul 18, 2020

CI Relability 2020-07-18 JaneaSystems/nodejs-reliability#41

Open

CI Relability 2020-07-19 JaneaSystems/nodejs-reliability#42

Open

CI Relability 2020-07-20 JaneaSystems/nodejs-reliability#43

Open

CI Relability 2020-07-21 JaneaSystems/nodejs-reliability#44

Open

jasnell force-pushed the quic-cleanups-7 branch from 8796d58 to 2bf14cd Compare July 21, 2020 22:26

github-actions bot mentioned this pull request Jul 22, 2020

CI Relability 2020-07-22 JaneaSystems/nodejs-reliability#45

Open

5 tasks

jasnell force-pushed the quic-cleanups-7 branch 2 times, most recently from fcad493 to 529a5c1 Compare July 22, 2020 16:17

jasnell marked this pull request as ready for review July 22, 2020 16:17

ronag approved these changes Jul 22, 2020

View reviewed changes

addaleax approved these changes Jul 22, 2020

View reviewed changes

src/quic/node_quic_session.cc Outdated Show resolved Hide resolved

lib/internal/quic/core.js Outdated Show resolved Hide resolved

jasnell force-pushed the quic-cleanups-7 branch from 61743a5 to 6230726 Compare July 22, 2020 22:44

jasnell added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. dont-land-on-v14.x quic Issues and PRs related to the QUIC implementation / HTTP/3. labels Jul 22, 2020

jasnell added this to Backlog in QUIC via automation Jul 22, 2020

jasnell moved this from Backlog to In Progress in QUIC Jul 22, 2020

github-actions bot mentioned this pull request Jul 23, 2020

CI Relability 2020-07-23 JaneaSystems/nodejs-reliability#46

Open

6 tasks

jasnell added 8 commits July 23, 2020 06:51

quic: convert openStream to Promise

3aefa5e

Although most of the time openStream will be able to create the stream immediately, when a stream is opened before the handshake is complete we have to wait for the handshake to be complete before continuing.

quic: simplify QuicStream construction logic

9999fa0

quic: remove stream pending code

42ee750

Removing no longer needed code

quic: remove unneeded quicstream.aborted and fixup docs

21a96cb

quic: gitignore qlog files

ec838a6

qlog files are diagnostic files that are being used to verify the quic implementation. Make sure they don't get checked in.

quic: documentation updates

53de1b2

quic: use async _construct for QuicStream

c3a33be

jasnell force-pushed the quic-cleanups-7 branch from 6230726 to c3a33be Compare July 23, 2020 13:51

jasnell added a commit that referenced this pull request Jul 23, 2020

quic: simplify QuicStream construction logic

da20287

PR-URL: #34351 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

jasnell added a commit that referenced this pull request Jul 23, 2020

quic: remove stream pending code

a65296d

Removing no longer needed code PR-URL: #34351 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

jasnell added a commit that referenced this pull request Jul 23, 2020

quic: remove unneeded quicstream.aborted and fixup docs

83bf0d7

PR-URL: #34351 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

jasnell added a commit that referenced this pull request Jul 23, 2020

quic: documentation updates

8bd61d4

PR-URL: #34351 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

jasnell added a commit that referenced this pull request Jul 23, 2020

quic: use async _construct for QuicStream

b06fe33

PR-URL: #34351 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

jasnell closed this Jul 23, 2020

QUIC automation moved this from In Progress to Done Jul 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

quic: continued refactoring of quic APIs #34351

quic: continued refactoring of quic APIs #34351

jasnell commented Jul 14, 2020 •

edited

nodejs-github-bot commented Jul 16, 2020 •

edited by jasnell

nodejs-github-bot commented Jul 16, 2020

jasnell commented Jul 16, 2020

This comment has been minimized.

nodejs-github-bot commented Jul 17, 2020 •

edited by jasnell

jasnell commented Jul 17, 2020

jasnell commented Jul 21, 2020

ronag commented Jul 22, 2020

jasnell commented Jul 22, 2020

jasnell commented Jul 22, 2020

nodejs-github-bot commented Jul 22, 2020 •

edited by jasnell

jasnell commented Jul 23, 2020

quic: continued refactoring of quic APIs #34351

quic: continued refactoring of quic APIs #34351

Conversation

jasnell commented Jul 14, 2020 • edited

Checklist

nodejs-github-bot commented Jul 16, 2020 • edited by jasnell

nodejs-github-bot commented Jul 16, 2020

jasnell commented Jul 16, 2020

This comment has been minimized.

nodejs-github-bot commented Jul 17, 2020 • edited by jasnell

jasnell commented Jul 17, 2020

jasnell commented Jul 21, 2020

ronag commented Jul 22, 2020

jasnell commented Jul 22, 2020

jasnell commented Jul 22, 2020

nodejs-github-bot commented Jul 22, 2020 • edited by jasnell

jasnell commented Jul 23, 2020

jasnell commented Jul 14, 2020 •

edited

nodejs-github-bot commented Jul 16, 2020 •

edited by jasnell

nodejs-github-bot commented Jul 17, 2020 •

edited by jasnell

nodejs-github-bot commented Jul 22, 2020 •

edited by jasnell