-
Notifications
You must be signed in to change notification settings - Fork 70
feat(ws): Add compio-ws: Async WebSocket support for compio runtime
#501
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds WebSocket support to the compio runtime by introducing a new compio-ws crate. It provides async WebSocket functionality built on top of the tungstenite library, with optional TLS support via rustls.
Key Changes:
- New
compio-wscrate with client/server WebSocket support - Growable sync stream adapter for bridging async/sync I/O
- TLS integration with optional certificate loading features
- Examples demonstrating usage and Autobahn test suite integration
Reviewed Changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
compio-ws/src/lib.rs |
Core WebSocket stream implementation and handshake functions |
compio-ws/src/growable_sync_stream.rs |
Buffered stream adapter with dynamic growth |
compio-ws/src/stream.rs |
MaybeTlsStream enum for plain/TLS abstraction |
compio-ws/src/rustls.rs |
TLS client support with certificate loading |
compio-ws/Cargo.toml |
Package configuration with feature flags |
compio-ws/examples/* |
Example servers and clients |
compio-ws/scripts/* |
Autobahn test automation scripts |
Cargo.toml |
Workspace configuration update |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
You might want to pass the CI first:) |
|
@Berrysoft I've fixed I've overlooked that the project uses nightly. Please run the CI again. |
compio-ws: Async WebSocket support for compio runtimecompio-ws: Async WebSocket support for compio runtime
4263f48 to
9711b92
Compare
Berrysoft
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better if there are some tests.
|
@Berrysoft I've added some tests that test basic websocket functionality and addressed the review comments. Please review when you can. |
Berrysoft
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much! There are some more comments:
- Would you want to replace the SyncStream in
compio-iodirectly? Well it's OK to keep the current one, and I'll do that if you don't want to spend more time. - I hesitated whether to make
compio-wsas a separate project, but now it seems that it's not a large codebase, so it's OK. It is necessary to also addcompio-wsas the dependency ofcompiocrate, and to add an aliasws, too. The important features ofcompio-wsshould be reflected in the features ofcompio, so that users won't need to write another individual dependency.- Note that although you have added the tests, they didn't run because they need new features. You may add the features to the
allfeature incompio.
- Note that although you have added the tests, they didn't run because they need new features. You may add the features to the
- @George-Miao is a bit busy now. He commented that
tungsteniteis a little inefficient because there's a buffer inside, and now we need to add another buffer for the gap between sync code and async code. If you have some other ideas about that, feel free to discuss about it.
| @@ -0,0 +1,174 @@ | |||
| #![cfg(feature = "connect")] | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for adding tests! This attribute could be moved to Cargo.toml as [[test]] sections.
|
|
||
| [dependencies] | ||
| rustls = { workspace = true, optional = true, default-features = false } | ||
| rustls-native-certs = { version = "0.8", optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to notice that rustls-platform-verifier is more usable than rustls-native-certs. We use the former in compio-quic.
| repository = "https://github.com/compio-rs/compio" | ||
|
|
||
| [workspace.dependencies] | ||
| compio = { path = "./compio", version = "0.16.0" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this line is removable now.
| /// Stream that can be either plain TCP or TLS-encrypted | ||
| #[derive(Debug)] | ||
| #[allow(clippy::large_enum_variant)] | ||
| pub enum MaybeTlsStream<S> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a comment: If you want it to be public, it's better to put it inside compio-tls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well this one is actually OK. If you don't move it, I think I'll do that in the future (or just make it private?).
| } | ||
| } | ||
|
|
||
| impl<S> Unpin for MaybeTlsStream<S> where S: Unpin {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be automatic. Did you meet some problems?
| let (tx, rx) = oneshot::channel(); | ||
|
|
||
| compio_runtime::spawn(async move { | ||
| let listener = TcpListener::bind("127.0.0.1:12345").await.unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Port 12345 can be used during tests so you can false-fail here.
Let's let OS choose port for us:
let listener = TcpListener::bind("127.0.0.1:0").await.unwrap();
let addr = listener.local_addr().unwrap();
let conn = TcpStream::connect(&addr).await.unwrap();
This PR adds
compio-ws, a WebSocket library that brings async WebSocket support to the compio runtime. It integrates the tungstenite WebSocket protocol implementation with compio. The structure of this project is mainly based ontokio-tungstenite.Features
rustls.GrowableSyncStream- aSyncStreamlike structure but with growable internal read/write buffers.Feature flags
Examples
You can test server and client using the following from the compio root dir:
Run
cargo run -p compio-ws --example echo_server --features connectto spin up a WS server and in a separate terminal runcargo run -p compio-ws --example client --features connect