GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — src: quic by jasnell · Pull Request #44325 · nodejs/node

Conversation

@jasnell

@jasnell jasnell commented Aug 21, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
nwtgck, tniessen, espoal, bricss, shrujalshah28, ZYSzys, pan93412, nicolashenry, siygle, MomenNano, and 36 more reacted with thumbs up emoji tniessen, espoal, derhuerst, bricss, cola119, ZYSzys, MomenNano, Spade-and-Archer, 2color, erulabs, and 17 more reacted with hooray emoji tniessen, bricss, RaisinTen, analytik, stafyniaksacha, MomenNano, 2color, Yogu, fed135, erulabs, and 19 more reacted with heart emoji bricss, RaisinTen, tniessen, Properko, stafyniaksacha, MomenNano, Spade-and-Archer, 2color, erulabs, ronag, and 11 more reacted with rocket emoji
All reactions
  • 👍 46 reactions
  • 🎉 27 reactions
  • ❤️ 29 reactions
  • 🚀 21 reactions
@nodejs-github-bot nodejs-github-bot added dont-land-on-v14.x needs-ci PRs that need a full CI run. quic Issues and PRs related to the QUIC transport implementation. labels Aug 21, 2022
@jasnell jasnell mentioned this pull request Aug 21, 2022
quic: add quic #38233
Closed
@jasnell jasnell added semver-minor PRs that contain new features and should be released in the next minor version. experimental Issues and PRs related to experimental features. labels Aug 21, 2022
@mcollina

mcollina commented Aug 21, 2022

Copy link
Copy Markdown
Member

I think we should better off in tagging this "dont-land" on all lines. Wdyt?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Aug 21, 2022

Copy link
Copy Markdown
Member Author

Yeah likely a good idea
ruyadorno reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Aug 21, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

To-do list:

  • Investigate build failures on various CI platforms
    • Build failure on macos
    • other build failure on Windows
    • ngtcp2 build failure on Windows
  • Socketaddress bug
  • Keep expanding tests to identify where the bugs are
  • Fix those bugs
  • Fix annoying format-cpp failures
  • ASAN failure
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell
jasnell force-pushed the new-new-quic branch from 125eebb to 4f25588 Compare August 21, 2022 17:08
@splitice

splitice commented Aug 22, 2022

Copy link
Copy Markdown

I'll likely get involved once there is a public API. I'm no internals expert (more of an internals breaker)
jasnell, codeswhite, and RuslanPDF reacted with thumbs up emoji
All reactions
  • 👍 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ruyadorno ruyadorno added dont-land-on-v16.x labels Aug 22, 2022
@jasnell
jasnell force-pushed the new-new-quic branch 2 times, most recently from 5c0053e to 161f525 Compare August 27, 2022 18:32
@tniessen tniessen mentioned this pull request Sep 12, 2022
src: remove node_bob.h and node_bob-inl.h #44607
Closed
tniessen
tniessen reviewed Sep 12, 2022
View reviewed changes
Comment thread lib/internal/socketaddress.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/event_target.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/node_http_common.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
This was referenced Sep 12, 2022
src: avoid copy when creating Blob #44616
Merged
lib,test: fix bug in InternalSocketAddress #44618
Merged
deps: update to ngtcp2 0.8.1 and nghttp3 0.7.0 #44622
Merged
@tniessen

tniessen commented Sep 12, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

  • src: avoid copy when creating Blob #44616 replaces d5df573 (#44325) and f6b6cb7 (#44325).
  • lib,test: fix bug in InternalSocketAddress #44618 replaces 24ef1f8 (#44325) (and adds a test case).
  • deps: update to ngtcp2 0.8.1 and nghttp3 0.7.0 #44622 replaces 8efe1dc (#44325) and d1a397b (#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 #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.
janbrasna reacted with heart emoji janbrasna reacted with rocket emoji bricss and janbrasna reacted with eyes emoji
All reactions
  • ❤️ 1 reaction
  • 🚀 1 reaction
  • 👀 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tniessen
tniessen reviewed Sep 15, 2022
View reviewed changes
Comment thread src/crypto/crypto_common.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
@espoal espoal mentioned this pull request Oct 10, 2022
Add experimental support for io_uring #34388
Closed
@splitice

splitice commented Oct 14, 2022

Copy link
Copy Markdown

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell added a commit to jasnell/node that referenced this pull request Oct 16, 2022
@jasnell
lib: add ability to add separate event name to defineEventHandler
f0a3b03 
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 <[email protected]>
@jasnell jasnell mentioned this pull request Oct 16, 2022
lib: add ability to add separate event name to defineEventHandler #45032
Merged
@jasnell

jasnell commented Oct 16, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell
jasnell force-pushed the new-new-quic branch from 2652d56 to 2e1802f Compare October 16, 2022 23:09
@jasnell

jasnell commented Oct 16, 2022

Copy link
Copy Markdown
Member Author

@tniessen ... smaller now :-)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell
jasnell requested a review from tniessen October 16, 2022 23:10
@Qard

Qard commented Dec 5, 2022

Copy link
Copy Markdown
Member

I'm still not done reading through all of it. Like I said, it's taking a lot of time. 😅
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss
bricss reviewed Dec 5, 2022
View reviewed changes
Comment thread lib/internal/quic/quic.js

const opts = {
...options,
depth: options.depth == null ? null : options.depth - 1

@bricss bricss Dec 5, 2022

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
bricss
bricss reviewed Dec 5, 2022
View reviewed changes
Comment thread lib/internal/quic/quic.js
alpn,
servername,
preferredAddressStrategy,
undefined, // Connection ID Factory, not currently used.

@bricss bricss Dec 5, 2022

Copy link
Copy Markdown
Contributor

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? 🤔

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bricss

bricss commented Dec 5, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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? 🤔
espoal and lin72h reacted with hooray emoji
All reactions
  • 🎉 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@splitice

splitice commented Dec 5, 2022

Copy link
Copy Markdown

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).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bnoordhuis

bnoordhuis commented Dec 6, 2022

Copy link
Copy Markdown
Member

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Qard
Qard requested changes Dec 7, 2022
View reviewed changes

@Qard Qard left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/quic/quic.js
sendInfoHeaders(headers) {
if (this.#inner === undefined)
throw new ERR_INVALID_STATE('The stream has been closed.');
this.#inner.sendHEaders(QUIC_HEADERS_KIND_INFO,

@Qard Qard Dec 5, 2022

Copy link
Copy Markdown
Member

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.

Suggested change
this.#inner.sendHEaders(QUIC_HEADERS_KIND_INFO,
this.#inner.sendHeaders(QUIC_HEADERS_KIND_INFO,

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/quic/quic.js
} = options;
validateBoolean(terminal, 'options.terminal');

this.#inner.sendHEaders(QUIC_HEADERS_KIND_INITIAL,

@Qard Qard Dec 5, 2022

Copy link
Copy Markdown
Member

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.

Suggested change
this.#inner.sendHEaders(QUIC_HEADERS_KIND_INITIAL,
this.#inner.sendHeaders(QUIC_HEADERS_KIND_INITIAL,

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/quic/quic.js
this.#endpoint = endpoint;
this.#inner[kOwner] = this;
this.#stats = new SessionStats(this.#inner.stats);
this.#state = new SessionState(this.#inner.state);

@Qard Qard Dec 5, 2022

Copy link
Copy Markdown
Member

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. 🤔

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
RohanTalip, Treycos, and mozhan1 reacted with thumbs up emoji
All reactions
  • 👍 3 reactions
Comment thread lib/internal/quic/quic.js
this.#inner = options[kCreateInstance]();
this.#inner[kOwner] = this;
this.#state = new EndpointState(this.#inner.state);
this.#stats = new EndpointStats(this.#inner.stats);

@Qard Qard Dec 5, 2022

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jasnell jasnell mentioned this pull request Dec 19, 2022
src: update src/crypto/* in preparation for quic #45912
Closed
@jasnell

jasnell commented Dec 19, 2022

Copy link
Copy Markdown
Member Author

The changes to src/crypto have been moved out to #45912 to allow for incremental/independent review.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasonwilliams jasonwilliams mentioned this pull request Dec 24, 2022
HTTP2/3 Support nodejs/undici#399
Closed
@mscdex mscdex mentioned this pull request Mar 3, 2023
Support for QUIC protocol on official release #46921
Closed
@agilan2001 agilan2001 mentioned this pull request Mar 4, 2023
feat: add quic support mqttjs/MQTT.js#1298
Closed
@jasnell jasnell mentioned this pull request Mar 27, 2023
quic: implement various utilities classes to be used by the quic impl #47263
Closed
@jasnell

jasnell commented Mar 27, 2023

Copy link
Copy Markdown
Member Author

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.
espoal, lin72h, aduh95, erictuvesson, kotonefami, RafaelGSS, o-t-w, Theofilos-Chamalis, bricss, prostreyd, and 9 more reacted with thumbs up emoji guillaumekh and olizilla reacted with heart emoji bricss, espoal, erictuvesson, o-t-w, prostreyd, and rogerpadilla reacted with eyes emoji
All reactions
  • 👍 19 reactions
  • ❤️ 2 reactions
  • 👀 6 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell
jasnell marked this pull request as draft March 27, 2023 00:34
@jasnell jasnell mentioned this pull request Mar 29, 2023
quic: add additional utilities for quic (part 2) #47289
Closed
@metcoder95 metcoder95 mentioned this pull request Apr 17, 2023
feat: Add H2 support nodejs/undici#2061
Merged
16 of 20 tasks
@targos targos removed the dont-land-on-v14.x label May 3, 2023
@Eomm Eomm mentioned this pull request Jun 20, 2023
QUIC implementation fastify/fastify#2379
Open
@darrachequesne darrachequesne mentioned this pull request Jun 21, 2023
QUIC support socketio/socket.io#3769
Closed
@targos targos removed the dont-land-on-v16.x label Sep 27, 2023
@CommanderStorm CommanderStorm mentioned this pull request Feb 11, 2024
Support HTTP/3 louislam/uptime-kuma#4475
Closed
@CommanderStorm CommanderStorm mentioned this pull request Mar 7, 2024
Support for selecting HTTP client version (HTTP/1.0, HTTP/1.1, HTTP/2, HTTP/3) louislam/uptime-kuma#4559
Open
@Faolain

Faolain commented Jul 17, 2024

Copy link
Copy Markdown

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?
rekire, Code002Lover, nikelborm, Yeshey, CarlOlson, pleerock, and richardeschloss reacted with thumbs up emoji
All reactions
  • 👍 7 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
splitice
splitice reviewed Aug 1, 2024
View reviewed changes

@splitice splitice left a comment

Copy link
Copy Markdown

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/quic/session.cc

pos = nullptr;

if (++packetSendCount == maxPacketCount) {

@splitice splitice Aug 1, 2024

Copy link
Copy Markdown

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/quic/session.cc
DEBUG_ARGS(session_, "Stream Data: %s", stream_data);

if (!packet) {
packet = CreateStreamDataPacket();

@splitice splitice Aug 1, 2024

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/quic/session.cc
void Session::SendConnectionClose() {
CHECK(!NgCallbackScope::InNgCallbackScope(*this));

if (is_destroyed() || is_in_draining_period() || state_->silent_close) return;

@splitice splitice Aug 1, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

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 👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/quic/session.cc
}

int DefaultApplication::GetStreamData(StreamData* stream_data) {
if (stream_queue_.IsEmpty()) return 0;

@splitice splitice Aug 1, 2024

Copy link
Copy Markdown

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@robertsLando robertsLando mentioned this pull request Aug 8, 2024
feat: add quic support mqttjs/MQTT.js#1925
Draft
@alexander-akait alexander-akait mentioned this pull request Aug 20, 2024
Please support server: 'quic' webpack/webpack-dev-server#4976
Closed
@jasnell jasnell closed this Sep 28, 2024
@derhuerst derhuerst mentioned this pull request Mar 4, 2026
QUIC support #23064
Closed
@Jing-yilin Jing-yilin mentioned this pull request Mar 7, 2026
feat: add QUIC transport backend as default fallback ReScienceLab/agent-world-network#37
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis left review comments
@anonrig anonrig anonrig left review comments
@Qard Qard Qard requested changes
@tniessen tniessen Awaiting requested review from tniessen
@mcollina mcollina Awaiting requested review from mcollina
+2 more reviewers
@splitice splitice splitice left review comments
@bricss bricss bricss left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

experimental Issues and PRs related to experimental features. needs-ci PRs that need a full CI run. quic Issues and PRs related to the QUIC transport implementation. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

15 participants

@jasnell @mcollina @splitice @tniessen @bnoordhuis @JakobJingleheimer @Qard @GeoffreyBooth @ronag @bricss @Faolain @anonrig @ruyadorno @targos @nodejs-github-bot
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.