WebTransport - #442
Conversation
Summary of ChangesHello @tatsuhiro-t, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request lays the groundwork for WebTransport support within the nghttp3 library. It integrates the necessary protocol elements to enable WebTransport sessions over HTTP/3, allowing for bidirectional data transfer between clients and servers. The changes encompass core library components, extending their functionality to recognize, process, and manage WebTransport-specific frames, settings, and stream behaviors, ensuring a robust foundation for future WebTransport-enabled applications. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces WebTransport support to nghttp3. It's a substantial change that adds new APIs, internal logic for handling WebTransport sessions and streams, and new frame/setting types. The changes are extensive, touching the core connection and stream management, as well as build files and public headers. The implementation seems to follow the WebTransport over HTTP/3 draft, including handling of CONNECT requests for session establishment, data streams, and session lifecycle management. New tests have been added to cover client and server scenarios. I've found a couple of issues where error return values are being ignored, which could lead to silent failures.
302c33b to
5f8fc45
Compare
788930d to
bedf22a
Compare
bedf22a to
7290a99
Compare
7290a99 to
63e65c1
Compare
d895173 to
ce32e33
Compare
ce32e33 to
e36de15
Compare
|
Any chance this can include the capability to compile to WASM (without baking in UDP socket) as a state machine? |
e36de15 to
3f6a2d8
Compare
1852cca to
cbffeac
Compare
|
|
||
| int nghttp3_conn_on_wt_stream(nghttp3_conn *conn, nghttp3_stream *stream, | ||
| int64_t session_id) { | ||
| nghttp3_stream *wt_ctrl_stream; |
There was a problem hiding this comment.
I am currently playing around with this PR and the node.js code base. (Testing against quiche-based implementations). So far it looked good with client initiated bidi stream.
Now, I am at a server initiated bidi stream, where the client is initially sending data and the server only after a while.
And I am a bit stuck at this point. The underlying quic stream is signaled via callback to the C++ objects, before the stream was identified as webtransport stream and attached to a session.
Is it possible to invoke here a callback, that a new stream has been identified as WT capable?
May be a call to nghttp3_recv_wt_datacall back with nullptr in the data?
There was a problem hiding this comment.
If I understand it correctly, you need a callback that is invoked on client side (in this particular scenario) when a stream is identified as WebTransport data stream, is that correct?
There was a problem hiding this comment.
Yes, that is correct.
But also the otherway round, if a client initiates a bidirectional stream and the server will write first, I think the same callback should be invoked. (Btw the behaviour of the browsers between firefox and chromium is here different, if I remember correctly, firefox will surface a stream only after data is sent, and chromium will surface anyway,
There was a problem hiding this comment.
and the callback should know about the session's stream id. And may be it also called when it is clear that it is not a wt stream? So that one can sort the stream out?
There was a problem hiding this comment.
Thanks. If it is not WebTransport stream, then it is a regular HTTP stream and in this case, client must speak first, and we do not need this kind of callback.
There was a problem hiding this comment.
Sure, but currently the ngtcp2 will first signals the stream existence.
The idea of the callback is a bit to sort these streams in webtransport (with session id) and non webtransport streams. But of course one can also just signal an incoming wt stream initiated by the peer.
There was a problem hiding this comment.
wt_data_stream_open callback has been added to address this specific issue.
If I add similar callback for regular HTTP stream, it would be done in main branch.
There was a problem hiding this comment.
If I add similar callback for regular HTTP stream, it would be done in main branch.
For this purpose, you can use the existing begin_headers callback that does not fire if the stream is WebTransport data stream.
|
May I ask something that may be related? Which may be a thing, that needs to be changed in this PR? (here is a node.js issue nodejs/node#63784, but I think it more a nghttp3 missing feature). Ok, I do a WT connection, the client creates a bidi stream, sends two times three bytes to the server, and it works fine. In order to do this, once the bytes are queued inside node.js stream, it calls its function Line 3128 in 9ce8d7f if it should schedule the stream. However, nghttp3_stream_require_schedule says that it does not require to be scheduled.More precisely, if I add there: I get: The reason is simple, the buffers are empty, we did not pull ever before from this stream as it comes from the server. So my question is, should something different be called instead of |
|
I think the client should call nghttp3_conn_open_wt_data_stream if it needs to send something. |
Thanks! That saved my day. I thought I must call this function only on client initiated streams. |
9ce8d7f to
52401a6
Compare
|
I have tested a nghttp3 client (inside node.js) against a quiche server using my test harness for webtransport. |
|
If the session close callback is implemented, I am sure that it has error code and message to tell the application the cause of the session closure. |
52401a6 to
74a93db
Compare
|
I removed open/close callbacks because we have no concrete use cases. |
Sounds like a good plan. I did not need the open callback for implementation in node.js, the normal callbacks were ok. Only when closing, I can use stream closing callbacks, but it misses the error code etc. So I dedicated callback will solve this. |
74a93db to
43ad57d
Compare
Yeah, error code and reason propagating to the WebTransport server and back to the client is a thing. That's one part I notice that I have to frequently bring up in WebTransport libraries. |
648d60a to
659211e
Compare
|
@martenrichter I added recv_wt_close_session callback. |
Thanks! |
659211e to
0eb76c4
Compare
|
|
||
| stream->rstate.state = NGHTTP3_REQ_STREAM_STATE_IGN_REST; | ||
|
|
||
| return conn_call_stop_sending(conn, stream, NGHTTP3_WT_SESSION_GONE); |
There was a problem hiding this comment.
I have tested this with an nghttp3 server (node.js) against a quiche client.
The problem is, that I get on the client side first an quic reset frame and the wt close session capsule arrives afterwards (result is, that the client will not receive the error code and message). The reset frame is caused by conn_call_stop_sending, which calls the callback of node.js, which says that we arrived a stop_sending and must reply with quic reset frame. And somehow this results that ngtcp2 sends the RST frame before the wt close session capsule. So I wonder if it is the intended behavior that the stop_sending callback is called. Or should the callback behave differently? Just a question, if you have a pointer how to mitigate this. Thanks!
There was a problem hiding this comment.
nghttp3_stop_sending is a callback function which is invoked when the library asks application to send STOP_SENDING to the stream identified by stream_id. QUIC application error code app_error_code indicates the reason for this action.
So it is supposed to tell QUIC stack to send STOP_SENDING. It seems like nodejs sends RESET_STREAM instead?
There was a problem hiding this comment.
Well currently the callback invokes:
https://github.com/nodejs/node/blob/ed33235bdaff6013786a432d1ff1d78edff9f627/src/quic/http3.cc#L924
which in turn invokes:
https://github.com/nodejs/node/blob/ed33235bdaff6013786a432d1ff1d78edff9f627/src/quic/streams.cc#L1796
So that sounds, as if the meaning of the functions, where confused. Thanks, your diagnosis was very helpful. I will file an error later this day.
There was a problem hiding this comment.
The problem I wrote about above, is actually caused by the last line. If I remove it at least chromium works.
The reason is as the current behaviour of nghttp3 is not according to the old draft2 spec. (Which is the one the two major browsers implement).
The old spec says:
If the sender of CLOSE_WEBTRANSPORT_SESSION does not receive a FIN after some time, it SHOULD send STOP_SENDING on the CONNECT stream.
So in the old spec, it was only ok to send the STOP_SENDING after some time, so that the other side has some time to answer and this is what failing on the chromium side (I did not have time to also debug on the firefox source),
The newer spec says:
An endpoint that sends a WT_CLOSE_SESSION capsule MUST immediately send a FIN on the CONNECT Stream. The endpoint MAY also send a STOP_SENDING with error code WT_SESSION_GONE to indicate it is no longer reading from the CONNECT stream. The recipient MUST either close or reset the stream in response. After receiving the WT_CLOSE_SESSION capsule, the receiver MAY send a STOP_SENDING with error code WT_SESSION_GONE. If any additional stream data is received on the CONNECT stream after receiving a WT_CLOSE_SESSION capsule, the stream MUST be reset with code H3_MESSAGE_ERROR.
So the implementation seems to align with this behavior, so it trips off the browser, using the old draft.
There was a problem hiding this comment.
Nice find. I leave this as is hoping that browsers eventually update their implementations to the latest spec.
There was a problem hiding this comment.
I tried to post it at chromium, but something wrong and it showed at
https://issues.chromium.org/issues/541716736
only an access denied. So no clue if this is my actual bug.
There was a problem hiding this comment.
The webtransport session is closed on this capsule, so this is not a huge issue. It is just chrome emits some errors, but given that we are still in draft versions, I do not think we need to do this kind of thing to support all versions back to this ancient one. That would complicate the implementation and worse it would bring security issue later.
There was a problem hiding this comment.
Also posted on bugzilla:
https://bugzilla.mozilla.org/show_bug.cgi?id=2060059
Well, let's hope they will fix it. In the past, I had to make often patch for the browser.
There was a problem hiding this comment.
Honestly I am not worried about this too much. WebTransport is still in draft stage, and duration that period, this kind of mismatch is highly expected.
There was a problem hiding this comment.
Well, I use it productively in my little setting with my quiche based node.js package for some years , and of course, when I switch over to node.js native on top of http3, I do not want that everything breaks... (I also fixed several bugs on the ua side).
|
Please have a look at quic-go/webtransport-go#261 (and post linked there in). Safrai seems to implement the newest spec, including the new protocol name. You may want to take a look. (A user on my webtransport package brought it to my attention fails-components/webtransport#490 (comment)), though I do not have access to Safari. |
Safari's load-bearing requirement is SETTINGS_WT_MAX_SESSIONS 0x14e9cd29 — a draft-13/14 setting that draft-15 removed (replaced by SETTINGS_WT_ENABLED 0x2c7cf000). Documented in webtransport-go's tracking issue #265 ("draft-13/14 holdover, not in draft-15"), corroborated by WebKit's own test server using the max-sessions knob. What I have access to manually confirm: my Safari receives data if server opens a stream, but can't open a stream or send. |
0eb76c4 to
2a8d932
Compare
|
draft-15 and -16 are mostly the same. case NGHTTP3_SETTINGS_ID_WT_ENABLED:
/* compat for pre draft-15 */
case NGHTTP3_SETTINGS_ID_WT_MAX_SESSIONS:
case NGHTTP3_SETTINGS_ID_WT_MAX_SESSIONS_DRAFT7:
/* compat for ancient draft */
case NGHTTP3_SETTINGS_ID_ENABLE_WEBTRANSPORT_DRAFT2:
dest->wt_enabled = ent->value != 0;
break;I am not sure what I should look at. |
I just wanted to let you know, that it behaves differently then other UA. If any action is required, I am not sure. |
|
Thanks. The webtransport situation in browser world is quite messy. As far as I know, chromium and firefox still use old drafts. Safari may be up to date. In order to run webtransport interop runner, I need to relax requirements to deal with those clients. The interop runner only includes chromium and firefox, and they work with nghttp3. |
| int nghttp3_conn_close_wt_session(nghttp3_conn *conn, int64_t session_id, | ||
| uint32_t wt_error_code, const uint8_t *msg, | ||
| size_t msglen) { | ||
| nghttp3_stream *stream; |
There was a problem hiding this comment.
I am now testing this against chromium and firefox. Both seem to close the connection ungently with e.g. "Connection lost" for chromium, when the capsule arrives. May be the capsule has the wrong value as they run draft2? Anyway, I will now update my local chromium build to current version and debug the chromium side to see what really causes the "Connection lost" it may be something else.
There was a problem hiding this comment.
The only capsule that nghttp3 sends is WT_CLOSE_SESSION. I think it is not strange if a receiver drops connection when receiving the terminal capsule.
There was a problem hiding this comment.
Well, I have a quiche based node.js based webtransport package, and this does not cause the error, but reports the correct close code and reason. (I am actually running my test suite from the package on nghttp3....). But we will see, I am familiar with both browsers and will see what the UA will do.
There was a problem hiding this comment.
It is a laborious job trying to get the same WebTransport source code to work the same on different browsers; and in different JavaScript runtimes.
It's almost technically accurate to say pick your poison and stick with that, 'cause Web compatibility is basically non-existent in this domain.
There was a problem hiding this comment.
I'd say half of the maintainers of WebTransport server/client code only care about one or two runtimes or browsers, and the other half of maintainers explicitly don't care about any runtime except the one or two they are focused on. So, thus, incompatibilities are not only expected, but bred into the process. Then, after the fact folks start trying to test. You gotta start from the start with the idea you are trying to write code that runs the same in different browsers. Few maintainers, if any do that. That's what I focus on as a field tester and browser and hacker.
There was a problem hiding this comment.
I am now debugging the Chromium side. The capsule seems to be processed fine.
The reason Chromium errors is that something is wrong with the fins on the connect stream. On the check that fails, both should be sent/received, respectively. May be my node.js code is wrong (likely), or it is something on the nghttp3. I will report back.
There was a problem hiding this comment.
Found it, the problem is below, I will post there.
There was a problem hiding this comment.
Today, I created a test with a server that creates a bidirectional stream, on which the client writes data, and the server echoes it back to the client.
Unfortunately, the test stalled, or as I discovered later, it ran into a busy loop, trying to reschedule the stream again and again after hitting a flow control block.
The stream is on the client side, and this if clause caused the trouble, as the stream could not be unscheduled.
I have added here:
if (nghttp3_client_stream_bidi(stream->node.id) || nghttp3_server_stream_bidi(stream->node.id) ) {
and it worked. Though I do not entirely understand the purpose of the if clause here. Shouldn't a unidirectional stream be unscheduled, too?
If you need more info about the calling sequence or the state of the flow control windows, I can provide it (just the stream flow control window is full).
Can you look into it?
There was a problem hiding this comment.
Thanks. This is probably the part I missed when writing this PR. I think nghttp3_stream_schedulable can be used in this if clause, which supports WebTransport streams.
There was a problem hiding this comment.
Yes, with nghttp3_stream_schedulable the test passes.
2a8d932 to
c7add5e
Compare
c7add5e to
ffc6cdb
Compare
https://datatracker.ietf.org/doc/html/draft-ietf-webtrans-http3-16
TODO
WE WONT MERGE THIS PR UNTIL WEBTRANSPORT-HTTP3 IS PUBLISHED AS RFC.