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

Chunked read/write API for RecvStream/SendStream #41

Open
MOZGIII opened this issue Jul 31, 2023 · 8 comments
Open

Chunked read/write API for RecvStream/SendStream #41

MOZGIII opened this issue Jul 31, 2023 · 8 comments
Labels
enhancement New feature or request feature

Comments

@MOZGIII
Copy link
Contributor

MOZGIII commented Jul 31, 2023

Under the hood, quinn has read_chunk and write_chunk fns (et al) - it would be great if this crate could expose a similar API.

@BiagioFesta BiagioFesta added enhancement New feature or request feature labels Jul 31, 2023
@BiagioFesta
Copy link
Owner

BiagioFesta commented Sep 23, 2023

I agree wtransport should provide a simple API to access "chunk-read-operations".

However, I am still investigating whether to make this more convenient and user-friendly (interoperability between ordered and no-ordered reads).

In the meanwhile, starting from version wtransport 0.1.5 streams have method to safely access underlying transport stream (from quinn): https://docs.rs/wtransport/0.1.5/wtransport/struct.RecvStream.html#method.quic_stream_mut

This is behind the feature quinn, so: wtransport = { version = "0.1", features = ["quinn"] } is needed.

In order to avoid API/ABI dependency breaks, wtransport re-export the version of quinn in use. So all quinn APIs are available. For example:

use wtransport::quinn::RecvStream;
// ...

@MOZGIII
Copy link
Contributor Author

MOZGIII commented Oct 4, 2023

This is great! I'll consider this done for now.

Just for better interop in the generics context - have you considered implementing AsRef/AsMut? That way it would be possible to generalize around owned streams that allow accessing the quinn streams: quinn streams themselves and the wtransport wrappers.

@BiagioFesta
Copy link
Owner

I am not really inclined to implement AsRef/AsMut as I would like to not directly expose the lower layer.

I want to let the access to QUIC layer only via feature (disabled by default), as I would like to not have the API depending from the transport. quinn library is great, but I might consider starting supporting different "QUIC backend" (such as s2n-quic).

And I would not put those (standard) traits under a feature flag.

Also considering it is not really a limitation not having AsRef/AsMut, as it can be easily workaround-ed

@MOZGIII
Copy link
Contributor Author

MOZGIII commented Oct 5, 2023

And I would not put those (standard) traits under a feature flag.

Why not? That's what I was implying - obviously these should only be available only with quinn enabled...

@MOZGIII
Copy link
Contributor Author

MOZGIII commented Oct 5, 2023

I'd actually suggest splitting the wtransport into two crates - one would be the all-guts-exposed low-level quinn-specific implementation, and the other would be sort of a higher-level details-hidden thing that would not expose the low-level internals by default but would allow switching backends, and in general target more portable uses - like supporting a web-sys backend?

I suspect this interface hiding idea is intended with eventual wasm/web target in mind anyway, but I have worked on xwebtransport and I can see some obstacles to supporting web-sys directly with the current design anyway. But maybe that is not the goal... Anyhow - having separate lower-level/less stable + higher-level/more stable crates is the best of both worlds imo. This way you can actually focus on the sane API without fighting the contradicting goals (exposing more functionality vs shielding from internal details).

@BiagioFesta
Copy link
Owner

BiagioFesta commented Oct 5, 2023

Why not? That's what I was implying - obviously these should only be available only with quinn enabled...

Because they are standard traits.

Standard traits are meant to provide consistent and expected behavior to users. Introducing a feature flag might add complexity and confusion to the codebase and documentation. Users often rely on these traits as fundamental building blocks for their applications. A feature flag for standard traits could potentially fragment the user API and lead to uncertainty about when and how to use them.

That being said, I'm open to discussing the issues you've encountered. I guess the lacking of those trait can be easily addressed with a simple method call (i.e., stream.quic_stream()); if you have a suitable practical case we can revisit that and add the traits

@BiagioFesta
Copy link
Owner

I'd actually suggest splitting the wtransport into two crates - one would be the all-guts-exposed low-level quinn-specific implementation, and the other would be sort of a higher-level details-hidden thing that would not expose the low-level internals by default but would allow switching backends, and in general target more portable uses - like supporting a web-sys backend?

I suspect this interface hiding idea is intended with eventual wasm/web target in mind anyway, but I have worked on xwebtransport and I can see some obstacles to supporting web-sys directly with the current design anyway. But maybe that is not the goal... Anyhow - having separate lower-level/less stable + higher-level/more stable crates is the best of both worlds imo. This way you can actually focus on the sane API without fighting the contradicting goals (exposing more functionality vs shielding from internal details).

That might be a good idea, which is already a part of the design of wtransport.

Indeed, wtransport is already split into two crates: wtransport and wtransport-proto. The general idea is to have most of the protocol in the latter crate and use the high-level user design (i.e., wtransport) to handle all I/O async operations (via quinn).

I am already considering the idea of having wtransport powered by multiple QUIC backends, so perhaps this approach can be leveraged too.

I need to think about it and understand what the options are and the best way to implement it.

Definitely something to include in the design plan for the future of this library. Thank you!

@MOZGIII
Copy link
Contributor Author

MOZGIII commented Oct 5, 2023

Well, I have already solved this issue with my xwebtransport codebase by adding the wrapper types - so it is trivial to wire everything through an extra call. See https://github.com/MOZGIII/xwebtransport/blob/51419f453f06aa2d3e769adb6e0aaf8ce9ace449/crates/xwebtransport-wtransport/src/types.rs.

But if I was building from scratch, rather than building a whole bunch of pretty useless newtypes, Id rather make use of quinn streams directly - this way other webtransport over quinn implementations (like webtransport-quinn) that expose quinn streams directly and wtransport could share some code that is common for the operations with specifically quinn streams. Currently, I just can't generalize around them, because wtransport API is "stubborn" about hiding the internals.
Now, while it is very reasonable for the wtransport to expose a custom type for a stream which wraps the quinn stream under the hood - it does not make a lot of sense to force-disable the possibilities composition where it is totally justified. In my case, I'd be fine with generalizing around "anything that wraps a quinn stream or is a quinn stream" - which is exactly what T: AsRef<...> T: AsMut<...> represent.


wtransport and wtransport-proto

Ah, you see, this is different! It just copied the quinn separation of concerns - but this is not enough here. It works perfectly for quinn itself because they only implement one single model under the hood. For portability over the implementation details they have switchable runtimes, but they are feature-flagged and expose the low-level stuff explicitly without hesitation - see https://github.com/quinn-rs/quinn/blob/e1e1e6e392a382fbded42ca010505fecb8fe3655/quinn/src/runtime/tokio.rs#L19. I would like that approach for wtransport - but from the experiments with web-sys and xwebtransport I don't think it would be a good design to abstract around the backend. It is just a different thing to abstract around. If you are curious why I think so - that is because I have had to land a super-flexible one-size-fits-all API here - and I don't like how verbose it is. It very much seems like it can be simplified, but it has only become visible after the research with the current version.

I suggest you take a look at what I am doing at https://github.com/MOZGIII/xwebtransport if you haven't yet. It is about time for me to build a central front-end crate xwebtransport itself (now that the common code traits and lower-level implementation are ready) that would expose the high-level APIs to hide the generics over the internals from the consumers. It is actually the whole thing that I am doing. I also have a couple of private projects that implement the same abstraction over web-sysand wtransport implementations for portability between wasm and native targets - but I got tired of reimplementing the abstraction layer over and over and put this effort into creating xwebtransport - which is a reusable generic abstraction over Web Transport.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature
Projects
None yet
Development

No branches or pull requests

2 participants