-
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
src: quic #44325
base: main
Are you sure you want to change the base?
src: quic #44325
Conversation
I think we should better off in tagging this "dont-land" on all lines. Wdyt? |
Yeah likely a good idea |
To-do list:
|
I'll likely get involved once there is a public API. I'm no internals expert (more of an internals breaker) |
5c0053e
to
161f525
Compare
I've opened a few small PRs with commits from this PR (or adaptations thereof) in the hope that those can land, potentially making it easier to review this PR.
The first two should be able to land without requiring any changes here. The third will conflict with the commits here and will also require a small change due to an API change in ngtcp2 (see #44622 (comment)). Note that these changes only affect existing code in Node.js and do not add any features. Additions should remain in this PR. |
@jasnell how close is this to getting a JS API? I have the possibility of some time that can be allocated in late november / early december where I can do a similar process to the previous pull request on a JS API. Probably finding a few bugs along the way. |
Adds an optional third argument to `defineEventHandler()` to specify an event name that is separate from the name used for the property. This will be used, for instance, by the quic implementation to support generating the `on...` events where the event name itself is hyphenated. For instance `defineEventHandler(target, 'streamreset', 'stream-reset')` Separated out from nodejs#44325 Signed-off-by: James M Snell <jasnell@gmail.com>
@tniessen ... smaller now :-) |
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.
I reviewed the src/crypto changes, not the other parts.
So between your review and @Qard's, does that cover most or all of it? Along those lines, how much of what's unreviewed is in new versus old areas? Like if the bulk of this PR is for a new API, and everyone not using that API is unaffected, then that entire new API could pretty much land with cursory review (since it's low risk) and all we need to focus reviewing effort on is the parts that overlap with existing/stable APIs. |
Any chance to split the javascript file into multiple files? |
I'm still not done reading through all of it. Like I said, it's taking a lot of time. 😅 |
|
||
const opts = { | ||
...options, | ||
depth: options.depth == null ? null : options.depth - 1 |
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.
Perhaps, it might be better to check for NaN
here, before arithmetics
alpn, | ||
servername, | ||
preferredAddressStrategy, | ||
undefined, // Connection ID Factory, not currently used. |
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.
huh? 🤔
Alrighty, codebase all LGTM @jasnell you da men 🦾 |
I've got the ability to run my tests that caught memory leaks last time again once there's a js API. I'm no native expert. I only know enough native js to break things or make small fixes (currently). |
I haven't looked at src/quic/crypto.* (1500 lines) at all yet. Maybe someone else has? |
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.
I've left some notes about the JS part.
Also, much of the tests seem to not really assert anything and just validate that it doesn't crash. Perhaps in place of a proper public API we could have more in-depth tests as examples of how all this fits together? I assume when a public API is made we could probably delete much of the internal tests, so could also use that to provide code examples with lots of comments explaining the use.
sendInfoHeaders(headers) { | ||
if (this.#inner === undefined) | ||
throw new ERR_INVALID_STATE('The stream has been closed.'); | ||
this.#inner.sendHEaders(QUIC_HEADERS_KIND_INFO, |
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.
this.#inner.sendHEaders(QUIC_HEADERS_KIND_INFO, | |
this.#inner.sendHeaders(QUIC_HEADERS_KIND_INFO, |
} = options; | ||
validateBoolean(terminal, 'options.terminal'); | ||
|
||
this.#inner.sendHEaders(QUIC_HEADERS_KIND_INITIAL, |
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.
this.#inner.sendHEaders(QUIC_HEADERS_KIND_INITIAL, | |
this.#inner.sendHeaders(QUIC_HEADERS_KIND_INITIAL, |
this.#endpoint = endpoint; | ||
this.#inner[kOwner] = this; | ||
this.#stats = new SessionStats(this.#inner.stats); | ||
this.#state = new SessionState(this.#inner.state); |
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.
Not really a fan of having two so similarly-named things so close together. Very easy to misread/mistype. Might be a good idea to rename one of these. 🤔
this.#inner = options[kCreateInstance](); | ||
this.#inner[kOwner] = this; | ||
this.#state = new EndpointState(this.#inner.state); | ||
this.#stats = new EndpointStats(this.#inner.stats); |
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.
Again, very similarly named things in the same place is easily misread.
The changes to |
I have pulled off more of the implementation bits into a separate PR here: #47263 The plan is to keep peeling pieces off into separate smaller PRs as much as possible. |
Do we know what else is needed here? |
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.
Most of my commits are very out of sync with your new work and would require re-testing however the ones highlighted in this review look to be matching with the current source.
There is also some comits related to keeping objects alive that look like they should still apply but would require testing.
We saw lots of crashes if the endpoint was allowed to close while streams were bound etc in the old implementation and generally resolved this by adding references between objects as required (e.g that the outbound source requires its backing packet)
I can go about validating these when theres a JS API I bet.
|
||
pos = nullptr; | ||
|
||
if (++packetSendCount == maxPacketCount) { |
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.
Is this how you plan to deal with the stream blocked = infinite resends?
I did it a different way in my fork. In my fork I ensured that a stream that returned NGTCP2_ERR_STREAM_DATA_BLOCKED would not be requeued within this call (HalleyAssist@a2c6fbe)
DEBUG_ARGS(session_, "Stream Data: %s", stream_data); | ||
|
||
if (!packet) { | ||
packet = CreateStreamDataPacket(); |
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.
This packet needs to retain the outbound source or node may crash
See HalleyAssist@60d9bbc for how I did it
void Session::SendConnectionClose() { | ||
CHECK(!NgCallbackScope::InNgCallbackScope(*this)); | ||
|
||
if (is_destroyed() || is_in_draining_period() || state_->silent_close) return; |
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.
This fix is the same as I found in HalleyAssist@f505f7a 👍
} | ||
|
||
int DefaultApplication::GetStreamData(StreamData* stream_data) { | ||
if (stream_queue_.IsEmpty()) return 0; |
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.
This short circuit bypasses stream_data->stream.reset(stream);
My notes don't say why but this was an issue.
Updated alternatiive to #38233 ...
Significant changes here. This focuses entirely on the internal API surface and does not expose any public API yet. That is intentional to allow us to work through this and was agreed on at the last collaborators summit.
The tests here are woefully incomplete and will be evolved over time.
It's a very big PR. If folks want help getting started on how to review I'm happy to walk through it. The prior PR bit rotted badly and had to be nearly completely redone.
Expect bugs. This will continue to be a work in progress for a bit.
/cc @mcollina @splitice