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 :-) |
Sure, but it would be helpful to get some insight into what kind of division would make it easiest. A good place to start, however, would be to just take a look at the README.md. It is detailed and provides background that would be necessary to review the rest so if we just started with that as a review point, treating is as the "spec" for the work here, I think that would be valuable.
Same here. |
heya! sorry, I don't see any team tagged before this comment (maybe that's why?). I would review if I knew anything about this feature-set. |
I've been gradually making my way through it when I have time but...it's a lot. 😅 I really wish we didn't discourage refactoring without an actual feature deliverable because there's a whole lot of stuff in here that could really just be several PRs refactoring specific components like crypto to prepare for QUIC needs. There's a bunch of stuff in here I would feel reasonably competent reviewing but also a bunch I would not. Giving a LGTM for the whole thing would feel a bit disingenuous to me as it is. 😐 I definitely appreciate all the amazing work you put into this though @jasnell! |
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. |
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