quic: additional quic refactors/cleanups #34160
Closed
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
jasnell
added
the
quic
Use is_* and set_* pattern for native object flags to improve readability in the code.
jasnell
force-pushed
the
quic-cleanups-3
branch
2 times, most recently
from
092153e
to
244b88e
Compare
This comment has been minimized.
This comment has been minimized.
jasnell
force-pushed
the
quic-cleanups-3
branch
3 times, most recently
from
0071050
to
bab96a7
Compare
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.
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.
|
Updated QUIC CI: https://ci.nodejs.org/job/node-test-commit/39444/ |
addaleax
reviewed
Jul 3, 2020
addaleax
reviewed
Jul 3, 2020
The QuicSession can be destroyed during garbage collection and the onSessionDestroy callback was happening in the destructor.
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 |
addaleax
approved these changes
Jul 3, 2020
|
Updated QUIC CI: https://ci.nodejs.org/job/node-test-commit/39450/ |
|
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. |
jasnell
added a commit
that referenced
this pull request
Jul 5, 2020
PR-URL: #34160 Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell
added a commit
that referenced
this pull request
Jul 5, 2020
Use is_* and set_* pattern for native object flags to improve readability in the code. PR-URL: #34160 Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell
added a commit
that referenced
this pull request
Jul 5, 2020
PR-URL: #34160 Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell
added a commit
that referenced
this pull request
Jul 5, 2020
PR-URL: #34160 Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell
added a commit
that referenced
this pull request
Jul 5, 2020
PR-URL: #34160 Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell
added a commit
that referenced
this pull request
Jul 5, 2020
The QuicSession can be destroyed during garbage collection and the onSessionDestroy callback was happening in the destructor. PR-URL: #34160 Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell
added a commit
that referenced
this pull request
Jul 5, 2020
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. PR-URL: #34160 Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell
added a commit
that referenced
this pull request
Jul 5, 2020
PR-URL: #34160 Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell
added a commit
that referenced
this pull request
Jul 5, 2020
PR-URL: #34160 Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell
added a commit
that referenced
this pull request
Jul 5, 2020
PR-URL: #34160 Reviewed-By: Anna Henningsen <anna@addaleax.net>
|
Landed in 56dbe46...1b1e985 |
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.
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