node
node copied to clipboard
nodejs
src: quic
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
I think we should better off in tagging this "dont-land" on all lines. Wdyt?
To-do list:
- [x] Investigate build failures on various CI platforms
- [x] Build failure on macos
- [x] other build failure on Windows
- [x] ngtcp2 build failure on Windows
- [x] Socketaddress bug
- [ ] Keep expanding tests to identify where the bugs are
- [ ] Fix those bugs
- [x] Fix annoying format-cpp failures
- [ ] ASAN failure
I'll likely get involved once there is a public API. I'm no internals expert (more of an internals breaker)
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.
- https://github.com/nodejs/node/pull/44616 replaces
d5df573(#44325) andf6b6cb7(#44325). - https://github.com/nodejs/node/pull/44618 replaces
24ef1f8(#44325) (and adds a test case). - https://github.com/nodejs/node/pull/44622 replaces
8efe1dc(#44325) andd1a397b(#44325) (and updates to the latest tagged versions).
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 https://github.com/nodejs/node/pull/44622#issue-1370502677).
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.
PR #45032 replaces commit 9156797 in this PR. PR #45033 replaces commit 4fa7e02 in this PR. PR #45045 replaces commit 9c48c17 in this PR.
Separated out to help continue reducing the size here.
@nodejs/collaborators @nodejs/tsc ... while this PR is not completely done, the lack of any review / feedback from contributors whatsoever is rather disheartening. If core contributors aren't interested in getting this reviewed then I will likely abandon the effort entirely.
@tniessen ... I really appreciate the effort you put into taking a look... just need others to do so also.
Is there some way to cut this into smaller pieces? 18.5k new lines of code is simply too much to work through in any reasonable amount of time.
I mean, it's not my day job and I don't get paid for this, and I suspect that's true for most other reviewers.
Is there some way to cut this into smaller pieces?
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.
it's not my day job and I don't get paid for this...
Same here.
@nodejs/collaborators @nodejs/tsc ... while this PR is not completely done, the lack of any review / feedback from contributors whatsoever is rather disheartening. If core contributors aren't interested in getting this reviewed then I will likely abandon the effort entirely.
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!
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.
I'm still not done reading through all of it. Like I said, it's taking a lot of time. 😅
Alrighty, codebase all LGTM @jasnell you da men 🦾
Fortunately/unfortunately, I don't have much wisdom in catching C++ memory leaks
Watta about merging this as it is with alpha-beta-gamma experimental flags all over the place aka docs
And applying gradual enhancement approach to all of this? 🤔
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).
So between your review and @Qard's, does that cover most or all of it?
I haven't looked at src/quic/crypto.* (1500 lines) at all yet. Maybe someone else has?
The changes to src/crypto have been moved out to #45912 to allow for incremental/independent review.
I have pulled off more of the implementation bits into a separate PR here: https://github.com/nodejs/node/pull/47263
The plan is to keep peeling pieces off into separate smaller PRs as much as possible.
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?
