-
Notifications
You must be signed in to change notification settings - Fork 30k
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 implementation #56328
Draft
jasnell
wants to merge
6
commits into
nodejs:main
Choose a base branch
from
jasnell:get-quic-working-part2
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
quic: more implementation #56328
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: James M Snell <[email protected]>
Signed-off-by: James M Snell <[email protected]>
Review requested:
|
This comment was marked as outdated.
This comment was marked as outdated.
4c96c67
to
bfaf60f
Compare
This comment was marked as outdated.
This comment was marked as outdated.
bfaf60f
to
c90a6d7
Compare
This comment was marked as outdated.
This comment was marked as outdated.
c90a6d7
to
0175f17
Compare
0175f17
to
2570ff3
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #56328 +/- ##
==========================================
+ Coverage 88.54% 88.67% +0.13%
==========================================
Files 657 659 +2
Lines 190393 191099 +706
Branches 36552 36475 -77
==========================================
+ Hits 168582 169457 +875
+ Misses 14998 14452 -546
- Partials 6813 7190 +377
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There's a lot more to do to get things fully working but this makes more incremental progress.
Several key bits:
--experimental-quic
CLI flag and thenode:quic
module behind it.test-quic-handshake
for a basic idea of the current state of the API. It's still rough but it's enough to start getting feedback.Key question for reviewers: Looking at the API documented in quic.md and demonstrated in test-quic-handshake, what API do you want to see here? This is focusing ONLY on the QUIC API at this point and does NOT touch on the http3 API.
For the server-side, the API is essentially:
For the client side it is essentially:
The API still feels rather awkward. For review, contrast with what Deno is doing here: https://github.com/denoland/deno/pull/21942/files#diff-1645ba9a2a2aac4440671da62b81ab9a723f50faffff0d0e8d016a1f991961a3
At this point there are bugs in the implementation still, yes, but let's focus on the API design at the moment. Please focus review comments on that.
A couple of bits to consider for the reivew...
In this API design, the
QuicEndpoint
represents the local binding the local UDP port. It supports being both a server and client at the same time. Use can be a bit awkward though so we need to carefully review. Specifically, a singleQuicEndpoint
can support any number ofQuicSession
s (both client and server at the same time). This gives maximum flexibility but means a more complicated API. For example, let's take a closer look at the client side example:The key question here is: does it make sense to expose
QuicEndpoint
like this? If we look at the Deno API, as an alternative, we see something like:In Deno's API, they've effectively hidden
QuicEndpoint
under their notion ofQuicConn
. While it might be the case that the same underlying UDP port is used when everconnectQuic(...)
is called, if it is that ends up being an internal implementation detail and not something that is exposed to users. Which approach is better? ExposingQuicEndpoint
the way that I am means we have more flexibility but also more complexity that may not be worth it?Alternatively, we could go with a simplified API where every call to
connect(...)
uses a separateQuicEndpoint
, so, for instance:We could still allow for direct reuse of
QuicEndpoint
in this model by allowing aQuicEndpoint
instance to be passed as an option toconnect(...)
, like:But the question here, whenever client connections are sharing a single endpoint is (a) when should the endpoint be closed, (b) should it ever be closed automatically?, (c) should it be unref'd so that if there are no active sessions but the client-side endpoint has not yet been closed, should it keep the node.js process from exiting?
Basically, looking for feedback on the API design so if you have other ideas for what color to paint the bikeshed, now is the time to discuss it.
@Qard @mcollina @anonrig @nodejs/quic