WebTransport feature rollout

[darioalessandro]

#183 is very close.

h2 integrated extended connect via a separate crate to keep it modular and clean.

We could follow suit and literally just move the WebTransportSession struct and necessary code to a separate crate.

To support this new crate, lets call it h3-webtransport, we would still have to merge all the support code into h3.

What do you think @seanmonstar @ten3roberts ?

We could also delay that decision and keep it in h3 for now until it matures.

[ten3roberts]

Yes.

The PR is quite large, as the existing code had to be adapted to fit the low level access that WebTransport requires.

However, the WebTransport handling is almost entirely contained inside a separate module, which can be moved to a separate crate.

Will be working on cutting down the PR next week

[seanmonstar]

This seems to fit with what I was asking originally about this feature:

I wanted to ask: how much if it is a protocol on top of http3? Is it like websockets? Where it just uses some generic mechanism and otherwise is implemented on top (and this could be a separate library)? Maybe just a new setting, like we did with "extended connect" in h2?

If it's clean and not unreasonable to do so, it makes sense to me. That's how hyper has handle most other "on-top-of-HTTP" protocol stuff so far: try to allow implementation outside of hyper.

[ten3roberts]

As it works now, h3 recognizes the frame types and CONNECT protocol, but won't do anything with it.

The WebTransportSession will intercept this frame in bidi streams and determine if it should hand it back to the Connection as a request, or an opened bidi stream.

Uni streams work in a similar way. The inner connection only knows about the stream type, so that it isn't discarded, but nothing more is done with it.

Datagrams use the ordinary HTTP/3 datagrams

[Ruben2424]

Moving the WebTransport code to a seperate crate sounds good to me.
I would also go further and suggest to move the Datagram stuff to a third crate (maybe h3-datagram) if possible. Because this is a seperate RFC and could be used by other protocols as well.

Btw great work so far 💪

[ten3roberts]

Sounds great. The datagram implementation is not tied to WT, so it works as a separate crate.

Are they going to live in tree, like h3-quinn.

I also need to finalize the session handling code.

There are two apis in consideration

WebTransportServer similar to an endpoint, accepts connections and managed the transport

WebTransportSession used to receive uni/bi/datagrams for this session. The wt server routes the incoming streams to the correct session. This is how quinn does with Endpoint and Connection, routing the incoming Quic frames to the correct connection, just as we route to the correct session.

Make the WebTransportSession own the connection, and make all apis return (session_id, uni), (session_id, datagram) etc, and require the session id to be provided when sending/opening.

[darioalessandro]

I propose that we keep it as a separate crate within the h3 repo as a nursery and then split?

[darioalessandro]

Regarding the 3rd repo, I would like to push that down the line, I really want to start using this feature asap and imo it is not trivial to split h3 datagrams from the server.rs stuff.

[darioalessandro]

Ok @Ruben2424 @seanmonstar @ten3roberts I split h3-webtransport into its own crate within the same repo, I would like to request a review.

[Ruben2424]

I hope I have time to take a look this weekend.

[ten3roberts]

Should the accept_bi function return both wt bidi streams and requests, as they are both server over quic bidi streams, or should requests be discarded or buffered, and be returned by another function, like accept_request?, it is quite awkward to return an enum like we do now

[darioalessandro]

Personally I just have a concern about the return type being Result<Option<AcceptedBi<C>>, Error> would it be better to return Result<AcceptedBi<C>>, Error>? by casting the Option to a Result and flattening it via

let result: Result<Result<....> = my_complex_result;
// Can't use flatten until https://doc.rust-lang.org/beta/unstable-book/library-features/result-flattening.html is merged.
let result: Result<AcceptedBi<C>>, Error> = result.and_then(convert::identity);

[Ruben2424]

What exactly speaks against the AcceptedBi enum?

[ten3roberts]

Currently, futures::AsyncRead/Write is implemented for the webtransport streams.

Should tokio::io::AsyncRead/Write be implemented as well? Makes it easier for user to later add FramedReader etc from tokio_util without compat wrappers

[darioalessandro]

Imo if it reduces line count/wrappers it sgtm! I am not a maintainer so my opinion might be weak here

[seanmonstar]

My personal opinion is that tokio's read/write traits are better: they can handle uninitialized reads, and the futures::io traits aren't stabilized or standardized across the ecosystem (where as Tokio's are "stable").

[ten3roberts]

Great.

Do you have a place where I can find more information about that, or where did you find it?

[seanmonstar]

Which information in particular? For uninitialized reads, Tokio's use a ReadBuf that can wrap uninitialized memory (and is being upstreamed into libstd), where as futures-io just uses &mut [u8]. Tokio is 1.0, where as futures-io is 0.3.

[ten3roberts]

H3 primarily uses Result<Option<_>> where Ok(None) means there is nothing more to accept, like connection closed.

I have also seen Option<Result<_>> which is easier to use when pattern matching, e.g looping accepts or using select statements.

while let Some(req) = conn.accept() {
    let req = req?;
}

With Result<Option<_>>

loop {
     if let Some(req) = conn.accept()? {
     }
}

Furthermore, Option<Result<_>> seems to be better supported in std, as the try operator works for Option, Result, but not Result Option

https://doc.rust-lang.org/std/ops/trait.Try.html

It also fits better with iterator/streams return types without requiring .transpose

What are your take som this?

[Ruben2424]

For the quic traits we are thinking about removing the Option from the return types (#177).
Maybe this approach could be used in the rest of the crate too, haven't thought about this yet.

But I think this should be implemented in a separate PR.

[ten3roberts]

Oh great, that issue didn't pop up in my search for Result and Option.

I guess we'll leave the WT API as is for now and not put any promises on API/function stability.

👍

[darioalessandro]

sgtm!

[inflation]

I guess it's inherited from quinn pre 0.9. They used to return None to indicate a locally closed stream. Now it's a proper error.

[ten3roberts]

I had to modify the quic traits to allow sending raw unframed bytes, as that is required by the webtransport spec. I.e, there is no data frame just byte after byte which can be read by the peer bit by bit, and sent bit by bit.

I did this by making the SendStream trait closer to AsyncWrite, though I am not sure if there is a better way to do it as the current approach removes the trait level B: Buf type, requiring implementors to support all buffers (h3_quinn and s2n already do this) and not specialize for a specific type. This has the disadvantage of not allowing a special impl for Bytes and another for Vec<Bytes> but no blanket impl, but has the advantage of less associated types for the user and does not contrain the user to a single type to send, I.e byte strings can be mixed with vec and bytes when sending.

What are your thoughts on this? Is there a better way to go about it?

I am more than happy to change if so required.

[Ruben2424]

What are you thinking about making a second independent trait that supports sending the required unframed bytes. Maybe WebTransportSendStreamUnframed. And leaving the current SendStream trait like it was.
Then h3-quinn could implement both traits for its SendStream struct.

This could make it also easier to separate more code into the h3-webtransport.

[ten3roberts]

That sounds like a good idea. I will do that. Then it will be up to implementations to make sure to provide both to avoid weird bound errors when the user is confounded as to why they can't call open_uni

[ten3roberts]

Could there arise a situation where a QUIC implementation allows sending framed data, but not unframed data? If so, maybe we could just add a send_raw_bytes to the existing trait and avoid creating more traits.

[ten3roberts]

Anyway, the separate trait is added which contains a method to send raw bytes, similar to AsyncWrite in tokio.