Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upquic: additional quic refactors/cleanups #34160
Open
+943
−844
Conversation
jasnell
force-pushed the
jasnell:quic-cleanups-3
branch
2 times, most recently
from
092153e
to
244b88e
This comment was marked as outdated.
This comment was marked as outdated.
jasnell
force-pushed the
jasnell:quic-cleanups-3
branch
3 times, most recently
from
0071050
to
bab96a7
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Because of the timing of qlog events emitted by ngtcp2, it becomes difficult to handle those as events on the QuicSession object because the final qlog entry is not emitted until the ngtcp2_conn is freed, which can occur when the object is being garbage collected (meaning, we a: can't call out to javascript and b: don't have an object we can use to emit the event). This refactors it into a QLogStream object that allows the qlog data to be piped out using a separate Readable stream.
|
@addaleax... updated, PTAL |
zbot473
commented
Jul 3, 2020
|
What's the difference between a vanilla QUIC implementation with the proper ALPN and QuicTransport? I know this probably isn't the best place to talk about this, but I am going to hold off on making a new issue until this PR is merged. |
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.
jasnell commentedJul 1, 2020
•
edited
This is ready for review...
The first three commits are general improvements to improve maintainability, readability, and quality of the c++ code...
This commit refactors the QuicSession close/destroy flow to (a) make it more correct and (b) make it far easier to reason about.
The shared state between the C++ side and the JavaScript side was using an AliasedArray where everything was uint64_t, which is wasteful given that most of the state fields are simple booleans. Using an AliasedStruct allows it to be more compact but does make reading it a bit more complicated on the JS side. Overall, this should be easier to maintain.
The onSessionDestroy callback was calling out to JavaScript during the
QuicSessionC++ object deconstruction, which could happen during garbage collection, so that's a no-no. This eliminates that callback.What it says on the tin.
Also self-explanatory
When the
QuicSessionis GC'd without a proper destroy, the timers weren't being freed properly. Makesure they are stopped.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes