Skip to content
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

Extended connect support for Webtransport #48962

Closed
martenrichter opened this issue Jul 29, 2023 · 12 comments
Closed

Extended connect support for Webtransport #48962

martenrichter opened this issue Jul 29, 2023 · 12 comments
Labels
feature request Issues that request new features to be added to Node.js. http2 Issues or PRs related to the http2 subsystem. stale

Comments

@martenrichter
Copy link
Contributor

What is the problem this feature will solve?

I am currently considering extending my Webtransport node plugin (https://github.com/fails-components/webtransport) with support for webtransport over http/2.

I have two options, wait for the support in libquiche, which currently provides the http/3 implementation, or use node.js native http/2.

Thankfully to
#22959
node.js already supports the extended connect protocol and I thought I have already everything for an implementation.
However, https://datatracker.ietf.org/doc/draft-ietf-webtrans-http2/ also requires in clause 3.1 a SETTINGS_WEBTRANSPORT_MAX_SESSIONS setting on the client and server side.

As far as I understand, I can not do anything on the javascript side and this one needs to be implemented on node.js itself or even nghttp2, if I understand the code correctly.

Can you consider adding this to node? As it is a tiny feature.... (Of course, if required I may also provide a PR the extended connect PR did not look to hard, but I do not have much overview over node.js internals).

Or do I miss sth and I can in fact do it in js.

What is the feature you are proposing to solve the problem?

Add SETTINGS_WEBTRANSPORT_MAX_SESSIONS in the same way as extended connect is handled.

What alternatives have you considered?

Use libquiche when it is ready also for this?

@martenrichter martenrichter added the feature request Issues that request new features to be added to Node.js. label Jul 29, 2023
@lpinca lpinca added the http2 Issues or PRs related to the http2 subsystem. label Jul 29, 2023
@bnoordhuis
Copy link
Member

bnoordhuis commented Jul 29, 2023

As a general rule we don't add support for things that are still in draft mode (which webtransport is.)

Besides that, it's unclear to me what you're actually asking for. SETTINGS_WEBTRANSPORT_MAX_SESSIONS is a h3 thing, not a h2 thing.

edit: I guess you're asking for something similar to SETTINGS_MAX_CONCURRENT_STREAMS but that's implemented in nghttp2. Node merely exposes it.

@martenrichter
Copy link
Contributor Author

Yes, Webtransport is primarily a http/3 thing. But it has a newer draft specification to map it onto http2.
This is the draft https://datatracker.ietf.org/doc/draft-ietf-webtrans-http2/ .
Short summary: it uses the same extended connect mechanism also websocket over http/2 uses, and it is already supported by node.js. The only thing is, that it adds SETTINGS_WEBTRANSPORTMAX_SESSIONS to the settings object.

So a client would send:

SETTINGS
SETTINGS_ENABLE_CONNECT_PROTOCOL = 1
SETTINGS_WEBTRANSPORT_MAX_SESSIONS = 1

and a server would reply:

SETTINGS
SETTINGS_ENABLE_CONNECT_PROTOCOL = 1
SETTINGS_WEBTRANSPORT_MAX_SESSIONS = 100

This is the only thing from the new spec, I can not implement on top of node (to my current knowledge).
After this I can just take over the stream and parse the capsule protocol, which is used to map http/3 syntax to http/2.

Of course, it was clear to me, that you do not want to implement something complex, while it is still in flow. But this one is rather tiny to unlock.
And yes I know, it is more something for nghttp2, since I have already looked in the code, but I thought I would ask on the top level, since it will not help if it is only implemented in nghttp2.

@bnoordhuis
Copy link
Member

We normally take a bottom-up approach. :-)

If nghttp2 adds it, then exposing it in node should be uncontroversial.

@bnoordhuis
Copy link
Member

I see the suggestion from upstream is to use nghttp2_submit_settings() and that could work (already used internally) but right now it's wrapped by a Http2Settings object that only understands a hard-coded list of settings.

Making it understand arbitrary settings requires both a new public API and a possibly not insignificant amount of refactoring. I don't plan to work on it but pull request welcome.

@martenrichter
Copy link
Contributor Author

I was actually about the write more or less the same. But it took me some time to understand the API (I worked more with quiche for the http3 stuff).
Ok, then I will put this onto my to-do list, may take a while, I am just coding on weekends and I first have to setup up a build environment for node, which is usually the higher hurdle.

Any suggestions for the arbitrary settings api? My first idea would be an object, the key is the numeric id per spec and value is the value per spec. The other question is, what to do with settings, which are already implemented. So how to sync, for backward compatibility they should be kept.

@bnoordhuis
Copy link
Member

My off-the-cuff thinking is that a prototype method is probably the best way to go about it, because that lets you handle existing properties more easily. Like this basically:

diff --git a/src/node_http2.cc b/src/node_http2.cc
index 01d0eb3418b..54c20b2ece6 100644
--- a/src/node_http2.cc
+++ b/src/node_http2.cc
@@ -3231,6 +3231,7 @@ void Initialize(Local<Object> target,

   Local<FunctionTemplate> setting = FunctionTemplate::New(env->isolate());
   setting->Inherit(AsyncWrap::GetConstructorTemplate(env));
+  SetProtoMethod(isolate, setting, "add", Http2Settings::Add);
   Local<ObjectTemplate> settingt = setting->InstanceTemplate();
   settingt->SetInternalFieldCount(AsyncWrap::kInternalFieldCount);
   env->set_http2settings_constructor_template(settingt);

@martenrichter
Copy link
Contributor Author

martenrichter commented Jul 30, 2023

So maybe like the JS map interface get and set, as the settings will also be received and one might want to query the fields that are set?
And the key can be either the numeric value or the string literal for known (non-draft) properties?
Or just the numeric value and a set of constants is provided?

@martenrichter
Copy link
Contributor Author

Ok, I have read nghttp2 source code in more detail.
Currently, I can only send additional settings. So only an "add" prototype is possible, getters currently do not make sense as nghttp2 does not support it.
So retrieving the settings from the connected client or server is not supported by nghttp2. as every value is hardcoded. For me, this is fine as I can just assume that the setting in question is supported, all I care about is that the browser gets the correct header.

@martenrichter
Copy link
Contributor Author

Draft PR as the first sketch is now available.

@martenrichter
Copy link
Contributor Author

Ok, the PR now passed the tests and should be ready for review.

Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Feb 18, 2024
@martenrichter
Copy link
Contributor Author

Is already fully implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. http2 Issues or PRs related to the http2 subsystem. stale
Projects
None yet
Development

No branches or pull requests

3 participants