Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

quic: more quic refactorings and cleanups #34283

Closed
wants to merge 18 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jul 9, 2020

Builds on #34262 ... that one needs to land first (the first three commits are from that PR)

  • quic: replace ipv6Only option with 'udp6-only' type

    Since the ipv6Only option was mutually exclusive with
    using 'udp6', making it it's own type simplifies things
    a bit.

(more will be added)

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 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 9, 2020
@jasnell jasnell force-pushed the quic-cleanups-6 branch 5 times, most recently from 534f1e8 to 5b5deb3 Compare July 13, 2020 22:25
@jasnell jasnell marked this pull request as ready for review July 13, 2020 22:49
@jasnell jasnell requested a review from a team July 13, 2020 22:49
@jasnell
Copy link
Member Author

jasnell commented Jul 13, 2020

@nodejs/quic ... there's still a lot more to do on this but this PR is already fairly large and complicated so I wanted to get this one in review. There are several major pieces in here:

  1. Movement to a Promises-based API. The QuicEndpoint, QuicSocket, and QuicSession classes have been moved to a Promises-based API for listen(), connect(), close(), etc. The openStream() API will also be come Promise based as well as the close() API on QuicStream. This enables code like:
const sock = createQuicSocket();
sock.on('session', (session) => {
  session.on('stream', (stream) => stream.end('hi!'));
});
await sock.listen();

and

const sock = createQuicSocket();
const client = await sock.connect();
const stream = await client.openStream();
stream.end('hello');
await client.close();
  1. Improved error handling for event emitter handlers (work in progress). Previously, the error handling was rudimentary and incomplete and did not properly account for async functions used as event handlers. This starts to improve that incrementally but additional changes will come in the next PR round.

  2. Improved QuicSession shutdown timing to match the spec. Previously, shutting down a QuicSession was not properly following the spec in terms of timing. There will be additional refinements made but this is much much closer to what it should be.

  3. Other API cleanups. More will be coming. The next PR will focus largely on refactoring the lifecycle of QuicStream so that we can use the default autoDestroy: true, eliminate things like aborted, and generally just cleanup the implementation.

@nodejs-github-bot

This comment has been minimized.

is ignored if `type` is `'udp4'`. **Default**: `false`.
* `type` {string} Can be one of `'udp4'`, `'upd6'`, or `'udp6-only'` to
use IPv4, IPv6, or IPv6 with dual-stack mode disabled.
**Default**: `'udp4'`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do this, should we update net and dgram to also accept 'udp6-only'? It seems odd that we deviate from the type + ipv6Only option pairing only in this place

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, eventually we should, yes. I just don't want to do it in this PR.

doc/api/quic.md Outdated Show resolved Hide resolved
doc/api/quic.md Outdated Show resolved Hide resolved
doc/api/quic.md Outdated Show resolved Hide resolved
doc/api/quic.md Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 14, 2020

@jasnell
Copy link
Member Author

jasnell commented Jul 14, 2020

CIs are good

@jasnell jasnell added the quic Issues and PRs related to the QUIC implementation / HTTP/3. label Jul 14, 2020
Since the `ipv6Only` option was mutually exclusive with
using `'udp6'`, making it it's own type simplifies things
a bit.
Doesn't need to be a function
Restricting this to pre-bind keeps things simple
Ensure that the QuicSocket is properly destroyed if the QuicEndpoint
is destroyed directly rather than through QuicSocket destroy
@ruyadorno
Copy link
Member

QUIC related, needs to be backported to v14.x 😊

@jasnell
Copy link
Member Author

jasnell commented Jul 22, 2020

None of the quic related prs should be backported

@ruyadorno
Copy link
Member

ruyadorno commented Jul 22, 2020

thanks for letting me know @jasnell!

I'm watching nodejs/github-bot#267 and in the meantime will update the issues I touched today to have the correct label 😊

cjihrig pushed a commit that referenced this pull request Jul 23, 2020
Since the `ipv6Only` option was mutually exclusive with
using `'udp6'`, making it it's own type simplifies things
a bit.

PR-URL: #34283
Reviewed-By: Anna Henningsen <anna@addaleax.net>
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
PR-URL: #34283
Reviewed-By: Anna Henningsen <anna@addaleax.net>
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
PR-URL: #34283
Reviewed-By: Anna Henningsen <anna@addaleax.net>
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
Doesn't need to be a function

PR-URL: #34283
Reviewed-By: Anna Henningsen <anna@addaleax.net>
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
Restricting this to pre-bind keeps things simple

PR-URL: #34283
Reviewed-By: Anna Henningsen <anna@addaleax.net>
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
PR-URL: #34283
Reviewed-By: Anna Henningsen <anna@addaleax.net>
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
Ensure that the QuicSocket is properly destroyed if the QuicEndpoint
is destroyed directly rather than through QuicSocket destroy

PR-URL: #34283
Reviewed-By: Anna Henningsen <anna@addaleax.net>
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
PR-URL: #34283
Reviewed-By: Anna Henningsen <anna@addaleax.net>
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
This is the start of a conversion over to a fully Promise-centric API
for the QUIC implementation.

PR-URL: #34283
Reviewed-By: Anna Henningsen <anna@addaleax.net>
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
PR-URL: #34283
Reviewed-By: Anna Henningsen <anna@addaleax.net>
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
PR-URL: #34283
Reviewed-By: Anna Henningsen <anna@addaleax.net>
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
QuicClientSession and QuicServerSessions are now always immediately
ready for use when they are created, so no more need to track
ready state.

PR-URL: #34283
Reviewed-By: Anna Henningsen <anna@addaleax.net>
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
PR-URL: #34283
Reviewed-By: Anna Henningsen <anna@addaleax.net>
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
PR-URL: #34283
Reviewed-By: Anna Henningsen <anna@addaleax.net>
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
PR-URL: #34283
Reviewed-By: Anna Henningsen <anna@addaleax.net>
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
PR-URL: #34283
Reviewed-By: Anna Henningsen <anna@addaleax.net>
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
When entering the closing or draining periods, servers should wait
three times the current probe timeout before releasing session
state.

PR-URL: #34283
Reviewed-By: Anna Henningsen <anna@addaleax.net>
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
PR-URL: #34283
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. quic Issues and PRs related to the QUIC implementation / HTTP/3.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants