quic: continued refactoring of quic APIs by jasnell · Pull Request #34351 · nodejs/node

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...

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...

conststream=awaitsession.openStream();console.log(stream.unidirectional);

We end up having to do...

conststream=session.openStream();awaitonce(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

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 aa6217eCompare 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 teamJuly 16, 2020 00:21

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

ronag reviewed Jul 16, 2020
View reviewed changes

Comment threadlib/internal/quic/core.js Outdated

ronag reviewed Jul 16, 2020
View reviewed changes

Comment threadlib/internal/quic/core.js Outdated

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

This comment has been minimized.
Sign in to view

github-actionsBot 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 2bf14cdCompare July 21, 2020 22:26

github-actionsBot 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 529a5c1Compare July 22, 2020 16:17

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

ronag reviewed Jul 22, 2020
View reviewed changes

Comment threadlib/internal/quic/core.js Outdated

ronag reviewed Jul 22, 2020
View reviewed changes

Comment threadlib/internal/quic/core.js Outdated

ronag reviewed Jul 22, 2020
View reviewed changes

Comment threadlib/internal/quic/core.js Outdated

ronag approved these changes Jul 22, 2020
View reviewed changes

addaleax approved these changes Jul 22, 2020
View reviewed changes

Comment threadsrc/quic/node_quic_session.cc Outdated
Comment threadlib/internal/quic/core.js Outdated

jasnell force-pushed the quic-cleanups-7 branch from 61743a5 to 6230726Compare 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

github-actionsBot 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: extensive refactoring of QuicStream lifecycle
c398c63
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.

quic: documentation updates
53de1b2

quic: use async _construct for QuicStream
c3a33be

jasnell force-pushed the quic-cleanups-7 branch from 6230726 to c3a33beCompare 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

Uh oh!

Uh oh!

Conversation

jasnell commented Jul 14, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Jul 16, 2020• edited by jasnell Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jul 16, 2020

Uh oh!

jasnell commented Jul 16, 2020

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

nodejs-github-bot commented Jul 17, 2020• edited by jasnell Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Jul 17, 2020

Uh oh!

jasnell commented Jul 21, 2020

Uh oh!

ronag commented Jul 22, 2020

Uh oh!

jasnell commented Jul 22, 2020

Uh oh!

jasnell commented Jul 22, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Jul 22, 2020• edited by jasnell Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Jul 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jasnell commented Jul 14, 2020•
edited
Loading

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

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

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