quic: start adding in the internal quic js api #53256

jasnell · 2024-06-02T03:34:30Z

While the external API for QUIC is expected to be the WebTransport API primarily, this provides the
internal API for QUIC that aligns with the native C++ QUIC components. This is the first of several PRs that will fill out this part of the implementation. The goal of doing this incrementally is to make things easier to review by doing it in smaller chunks

lib/internal/quic/quic.js

lib/internal/quic/datagrams.js

lib/internal/quic/quic.js

mcollina

Unless a massive improvement is planned on WebStream, I would recommend not using them in this lower level. They are 1 order of magnitude slower than Node streams. Using them here would limit the applicability of our Quic implementation.

Just using functions and callbacks might even be faster.

splitice · 2024-06-07T03:37:27Z

Really interesting @jasnell .

Just a reminder we have continued to apply fixes to your original attempt many of which probably still apply today (latest branch: https://github.com/HalleyAssist/node/commits/test_quic16/ - not everything in this branch is relevant). Some fixes could probably done better with more drastic changes, however they all represent observed leaks, infinite loops or crashes observed in our staging and pilot environments.

Including yesterday a fix for blocking streams creating an infinite loop in SendPendingData (which can be tested for with rx packet loss simulation). When you do get a JS api I'll try and contribute some of these directly.

Qard · 2024-06-14T19:01:44Z

lib/internal/quic/quic.js

+  instance[kStats] = createStreamStats(handle.stats);
+  instance[kState] = createStreamState(handle.state);


Makes me a bit nervous that these names are a single character apart. High chance of typos in future maintenance.

Not ignoring this comment, just plan to address it in a subsequent iteration

lib/internal/quic/quic.js

nodejs-github-bot · 2024-09-28T17:12:43Z

CI: https://ci.nodejs.org/job/node-test-pull-request/62830/

While the external API for QUIC is expected to be the WebTransport API primarily, this provides the internal API for QUIC that aligns with the native C++ QUIC components.

nodejs-github-bot · 2024-09-28T17:53:54Z

CI: https://ci.nodejs.org/job/node-test-pull-request/62831/ 💛

While the external API for QUIC is expected to be the WebTransport API primarily, this provides the internal API for QUIC that aligns with the native C++ QUIC components. PR-URL: #53256 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

PR-URL: #53256 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

jasnell · 2024-09-29T18:41:36Z

Landed in 103b843...858bce5

targos · 2024-09-30T13:19:46Z

This landed with failing Coverage CI, and now it fails for all PRs against main

targos · 2024-09-30T13:21:32Z

.nycrc

@@ -4,7 +4,7 @@
    "test/**",
    "tools/**",
    "benchmark/**",
-    "deps/**"
+    "deps/**",


This change is why coverage jobs error

While the external API for QUIC is expected to be the WebTransport API primarily, this provides the internal API for QUIC that aligns with the native C++ QUIC components. PR-URL: #53256 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

PR-URL: #53256 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

While the external API for QUIC is expected to be the WebTransport API primarily, this provides the internal API for QUIC that aligns with the native C++ QUIC components. PR-URL: nodejs#53256 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

PR-URL: nodejs#53256 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jun 2, 2024

jasnell added the quic Issues and PRs related to the QUIC implementation / HTTP/3. label Jun 2, 2024

jasnell requested review from Qard and anonrig June 2, 2024 03:35

jasnell force-pushed the quic-part-13 branch 2 times, most recently from 1babee6 to 9f2f600 Compare June 2, 2024 04:03

jasnell requested a review from mcollina June 2, 2024 04:04

jasnell force-pushed the quic-part-13 branch from 9f2f600 to f716980 Compare June 2, 2024 04:06

jasnell requested a review from tniessen June 2, 2024 14:30

jasnell force-pushed the quic-part-13 branch from f716980 to 5342557 Compare June 2, 2024 14:53

jasnell commented Jun 2, 2024

View reviewed changes

lib/internal/quic/quic.js Outdated Show resolved Hide resolved

targos added dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. labels Jun 2, 2024

jasnell commented Jun 2, 2024

View reviewed changes

lib/internal/quic/datagrams.js Outdated Show resolved Hide resolved

jasnell force-pushed the quic-part-13 branch 8 times, most recently from d4ea9c3 to f245d2c Compare June 2, 2024 22:14

VoltrexKeyva reviewed Jun 3, 2024

View reviewed changes

mcollina reviewed Jun 4, 2024

View reviewed changes

Qard approved these changes Jun 14, 2024

View reviewed changes

anonrig approved these changes Jun 14, 2024

View reviewed changes

jasnell force-pushed the quic-part-13 branch from f245d2c to 8f406a1 Compare July 6, 2024 16:31

jasnell force-pushed the quic-part-13 branch 3 times, most recently from 9dc124d to 952b293 Compare July 8, 2024 03:41

mcollina reviewed Jul 8, 2024

View reviewed changes

lib/internal/quic/quic.js Outdated Show resolved Hide resolved

lib/internal/quic/quic.js Show resolved Hide resolved

achingbrain mentioned this pull request Jul 18, 2024

Add support for QUIC libp2p/js-libp2p#1459

Open

robertsLando mentioned this pull request Aug 8, 2024

feat: add quic support mqttjs/MQTT.js#1925

Draft

jasnell force-pushed the quic-part-13 branch from 952b293 to 37deb95 Compare September 28, 2024 16:38

anonrig approved these changes Sep 28, 2024

View reviewed changes

jasnell force-pushed the quic-part-13 branch from 37deb95 to 61ffef3 Compare September 28, 2024 16:56

This comment was marked as outdated.

Sign in to view

jasnell force-pushed the quic-part-13 branch from 61ffef3 to f883756 Compare September 28, 2024 17:11

jasnell added 2 commits September 28, 2024 10:38

quic: start adding in the internal quic js api

fd1f2d0

While the external API for QUIC is expected to be the WebTransport API primarily, this provides the internal API for QUIC that aligns with the native C++ QUIC components.

src: make minor tweaks to quic c++ for c++20

82566f8

jasnell force-pushed the quic-part-13 branch from f883756 to 82566f8 Compare September 28, 2024 17:51

jasnell added a commit that referenced this pull request Sep 29, 2024

src: make minor tweaks to quic c++ for c++20

858bce5

PR-URL: #53256 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

jasnell closed this Sep 29, 2024

targos mentioned this pull request Sep 30, 2024

src: apply getCallSite optimization #55174

Closed

targos reviewed Sep 30, 2024

View reviewed changes

.nycrc

@@ -4,7 +4,7 @@

"test/**",

"tools/**",

"benchmark/**",

"deps/**"

"deps/**",

Copy link

Member

targos Sep 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is why coverage jobs error

This was referenced Sep 30, 2024

build: fix nycrc format #55188

Closed

build: fix not valid json in coverage #55179

Merged

targos pushed a commit that referenced this pull request Oct 5, 2024

src: make minor tweaks to quic c++ for c++20

295c17c

PR-URL: #53256 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

aduh95 mentioned this pull request Oct 9, 2024

V22.10.0 proposal #55343

Merged

louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024

src: make minor tweaks to quic c++ for c++20

ee925bb

PR-URL: nodejs#53256 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

quic: start adding in the internal quic js api #53256

quic: start adding in the internal quic js api #53256

jasnell commented Jun 2, 2024

mcollina left a comment

splitice commented Jun 7, 2024

Qard Jun 14, 2024

jasnell Jul 7, 2024

This comment was marked as outdated.

nodejs-github-bot commented Sep 28, 2024

nodejs-github-bot commented Sep 28, 2024 •

edited by jasnell

Loading

jasnell commented Sep 29, 2024

targos commented Sep 30, 2024

targos Sep 30, 2024

		instance[kStats] = createStreamStats(handle.stats);
		instance[kState] = createStreamState(handle.state);

quic: start adding in the internal quic js api #53256

quic: start adding in the internal quic js api #53256

Conversation

jasnell commented Jun 2, 2024

mcollina left a comment

Choose a reason for hiding this comment

splitice commented Jun 7, 2024

Qard Jun 14, 2024

Choose a reason for hiding this comment

jasnell Jul 7, 2024

Choose a reason for hiding this comment

This comment was marked as outdated.

nodejs-github-bot commented Sep 28, 2024

nodejs-github-bot commented Sep 28, 2024 • edited by jasnell Loading

jasnell commented Sep 29, 2024

targos commented Sep 30, 2024

targos Sep 30, 2024

Choose a reason for hiding this comment

nodejs-github-bot commented Sep 28, 2024 •

edited by jasnell

Loading