-
Notifications
You must be signed in to change notification settings - Fork 29k
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: initial experimental quic implementation #30943
Conversation
Some initial details to get started... This PR adds two new additional dependencies: ngtcp2 and nghttp3, and patches openssl for quic support.
For OpenSSL... QUIC includes built in support for TLS 1.3 but the timing of key generation is different for QUIC than it is for TCP. The BoringSSL project has implemented a new set of QUIC specific APIs that are currently being actively backported to OpenSSL 3.x. The PR, however, has not yet landed in OpenSSL. Since we are running OpenSSL 1.1.1, I further backported the APIs from that open OpenSSL 3 PR to our instance of 1.1.1. Whee! Once we are able to move to OpenSSL 3.x, will be able to avoid floating those patches. When QUIC is not being used, there are no changes to OpenSSL function at runtime. One practical limitation of this is that it means we can only use the bundled version of OpenSSL and cannot use the shared version. Additional information in a later update. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
options, | ||
this.#qlogEnabled); | ||
// We no longer need these, unset them so | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this happen in _destroy
or inside an event listener for finish
& end
?
e.g.
function onEndFinish () {
if (this.writableFinished && this.readableEnded) {
this[kHandle].resetStream(code, family);
}
}
this.on('end', onEndFinish);
this.on('finish', onEndFinish);
Or something along those lines...
options, | ||
this.#qlogEnabled); | ||
// We no longer need these, unset them so | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this read()
call required?
@ronag ... some important bits to think about with regards to the
|
Is it possible at least partly get around that with
Btw no hurry with responding to my comments. I don't mind if we take this whenever you feel is appropriate in terms of time and where you are currently focusing your attention. I would very much like to avoid making "hacks" around streams and where possible either make it work with current streams or update streams to better support the cases you require here. |
@nodejs/collaborators @nodejs/tsc @nodejs/quic ... this is ready for detailed review. Note that changes are not tested in CI yet because there is no configuration yet that uses the --experimental-quic compile flag (/cc @nodejs/build) |
@nodejs/collaborators @nodejs/tsc ... to help make reviewing easier, I'm happy to jump on a vscode liveshare session with folks to give them a walkthrough. |
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: Ouyang Yadong <oyydoibh@gmail.com> Co-authored-by: Juan Jos<C3><A9> Arboleda <soyjuanarbol@gmail.com> Co-authored-by: Trivikram Kamat <trivikr.dev@gmail.com> Co-authored-by: Denys Otrishko <shishugi@gmail.com>
CI: https://ci.nodejs.org/job/node-test-pull-request/29785/ |
Couple of relevant failures in CI.
|
PR-URL: nodejs/quic#360 Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: nodejs/quic#359 Reviewed-By: Anna Henningsen <anna@addaleax.net>
Ping @nodejs/collaborators @nodejs/tsc ... could definitely use some review on this one. Would very much like to get it landed before the 14.0.0 cut off. I know it's a massive PR and difficult to review so I'm happy to jump on the phone with anyone who wants to go through it in detail. |
Why would a new feature be semver-major? Why would an openssl change that has no impact on non-quics use-cases be semver-major? |
Can you restructure these PRs so they are more easily reviewed? It wouldn't substantially increase the complexity of managing, you would still have one branch you can rebase, but you might, occaisonally, need to force-push the two deps branches to sync them with the main one. Example (not to be commented on, just to show how much more reviewable it becomes):
|
Adds a new top level module that shadows an existing module published on the npm registry.
Just marked defensively. With the compile time flag in place now it should be fine actually. But see above for why it's still semver-major
Yes, but the concern there is that doing in a separate fork repo pulls the review conversation away from here. Perhaps if folks don't mind I can put the PR branches under this repo so we can keep the conversation here? |
I agree, and to be clear, that is exactly my suggestion/request. Where the branches are doesn't matter to the PR structure (though putting them all in nodejs/node sounds fine to me), but all 3 PRs would have to be against nodejs/node so the conversation can be here. The deps:openssl and deps:ng PRs could be "draft" (though the openssl in particular could land before the next 2, or not, whatever ends up being convenient). |
I assume its https://www.npmjs.com/package/quic that is the problem, not https://www.npmjs.com/package/http3, because the latter sounds like they would be willing to give it to Node.js. Exporting as net/quic would solve this, as would adding a no-op I'm just concerned that v14.0.0 not unnecessarily become a deadline for landing. |
Given that the |
Plausible, though I assume there are a raft of methods associated with the returned object, the top-level module also lends itself to organizing docs, net is pretty full already. |
The main implementation is separated out under |
I like that. |
Closing this PR in favor of three separate PRs that will (hopefully) make this whole thing easier to review (cc @sam-github )
Please move discussion to those PRs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please help if there is any problems.
This is the initial QUIC implementation. It is experimental, it is still unfinished, and it's complicated.
To get started with this, use
./configure --experimental-quic
orvcbuild experimental-quic
to build with the quic support enabled.EXPECT BUGS!!
This adds two new dependencies: ngtcp2 and nghttp3.
This also patches openssl to add the new boringssl quic apis. This is a floated patch that hopefully we'll be able to drop later.
It is semver-major because of the additional top level module and the changes to openssl (there are no functional changes to openssl if quic is not being used)
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes