feat(net): AsyncWrite and AsyncRead for WebSocket #379
Conversation
CBenoit
force-pushed
the
websocket-async-read-async-write
branch
from
120bc58 to
6ae0098
Compare
| # As of now, only implements `AsyncRead` and `AsyncWrite` on `WebSocket` | ||
| io-util = ["futures-io"] |
There was a problem hiding this comment.
I figured it was best to provide this as an optional feature so that futures-io is not added in the dependency graph of all dependents
| /// Leftover bytes when using `AsyncRead`. | ||
| /// | ||
| /// These bytes are drained and returned in subsequent calls to `poll_read`. | ||
| #[cfg(feature = "io-util")] | ||
| pub(super) read_pending_bytes: Option<Vec<u8>>, // Same size as `Vec<u8>` alone thanks to niche optimization |
There was a problem hiding this comment.
An alternative to Vec::drain would be to maintain an index pointing to the beginning of the "pending" region
| @@ -99,3 +100,5 @@ eventsource = [ | |||
| 'web-sys/EventSource', | |||
| 'web-sys/MessageEvent', | |||
| ] | |||
| # As of now, only implements `AsyncRead` and `AsyncWrite` on `WebSocket` | |||
| io-util = ["futures-io"] | |||
There was a problem hiding this comment.
| io-util = ["futures-io"] | |
| io-util = ["dep:futures-io"] |
This ensures futures-io is not a feature (by default, cargo adds features for optional dependencies)
There was a problem hiding this comment.
I agree with you. However, the rest of the Cargo.toml is currently not using dep: to denote a dependency. When using this notation at one place, the other places must be updated too because this effectively disables the old "implicit features". This is also technically a breaking change because these implicit features may be enabled by downstream users, though it’s probably not a big deal because there is generally little value in enabling those. I think it’s better to tackle this in a separate PR for all crates in order to keep consistency. However, if you prefer I’m okay with converting this crate as part of this PR. I just want to make sure it’s okay for you.
Maybe it’s better if I move these tests in the tests/ folder instead? I mirrored how it was done in the |
|
No, it's fine here as well. I would like to keep them consistent and this PR is not a place to move the tests. Can you merge from master and update the changelog? Merging from master should also fix the CI failures |
Implements `AsyncWrite` and `AsyncRead` on `WebSocket` behind the `io-util` feature flag.
CBenoit
force-pushed
the
websocket-async-read-async-write
branch
from
f91b372 to
06cedc4
Compare
Sounds good. I rebased on master and only updated the changelog (see the last commit). |
Implements
AsyncWriteandAsyncReadonWebSocketbehind theio-utilfeature flag.This code was originally living in an ad hoc fashion, in our IronRDP repository, and I thought it was best to upstream it. In addition to the unit tests, I can confirm it works when running long-lived RDP sessions in the browser.
Closes #25