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

Add a ByteWriter wrapper that implements AsyncWrite for a Sink<Message> #152

Merged
merged 2 commits into from
Feb 18, 2025

Conversation

joshtriplett
Copy link
Contributor

This is useful for programs that want to treat a WebSocket as a stream
of bytes.


Additional bits that could be added in future PRs:

  • Implementing the tokio version of AsyncWrite if tokio is enabled
  • ByteReader to implement AsyncRead for a Stream<Item = Message>

@joshtriplett
Copy link
Contributor Author

joshtriplett commented Feb 17, 2025

Also, as a design note: this is built around Sink<Message> because that way it works with SplitSink in addition to WebSocketStream. It'd be quite doable to work directly on WebSocketStream, but then it won't work on SplitSink.

If you want ByteWriter to work without Sink support, it may make sense for WebSocketStream to have native split support. Open to designing this however you would prefer.

…essage>`

This is useful for programs that want to treat a WebSocket as a stream
of bytes.
@sdroege
Copy link
Owner

sdroege commented Feb 18, 2025

If you want ByteWriter to work without Sink support, it may make sense for WebSocketStream to have native split support. Open to designing this however you would prefer.

How would such a native split() support for WebSocketStream look like though? There's not really a replacement for Sink, or is there nowadays?

Implementing the tokio version of AsyncWrite if tokio is enabled

That seems easy to add here directly. Can you do that?

@joshtriplett
Copy link
Contributor Author

If you want ByteWriter to work without Sink support, it may make sense for WebSocketStream to have native split support. Open to designing this however you would prefer.

How would such a native split() support for WebSocketStream look like though? There's not really a replacement for Sink, or is there nowadays?

Something along the lines of WebSocketStream::split() returning a WebSocketSender that has the send function, and a WebSocketReceiver that implements Stream.

I think I can make this implementation forward-compatible with being able to do that.

Implementing the tokio version of AsyncWrite if tokio is enabled

That seems easy to add here directly. Can you do that?

Sure; just wanted to review the basic design with you first.

@sdroege
Copy link
Owner

sdroege commented Feb 18, 2025

Something along the lines of WebSocketStream::split() returning a WebSocketSender that has the send function, and a WebSocketReceiver that implements Stream.

Sounds like a good plan to me. And WebSocketSender would implement Sink if the feature is enabled, and in the future whatever new trait comes up as a replacement for Sink.

@joshtriplett
Copy link
Contributor Author

Something along the lines of WebSocketStream::split() returning a WebSocketSender that has the send function, and a WebSocketReceiver that implements Stream.

Sounds like a good plan to me. And WebSocketSender would implement Sink if the feature is enabled, and in the future whatever new trait comes up as a replacement for Sink.

Right. And if the sink feature is enabled, ByteWriter supports any Sink, otherwise it only supports WebSocketSender and WebSocketStream.

@joshtriplett
Copy link
Contributor Author

joshtriplett commented Feb 18, 2025

OK, I've added a commit implementing tokio::io::AsyncWrite. I think this is ready to merge.

As far as I can tell, this design is forward-compatible with supporting WebSocketSender in the future.

@sdroege
Copy link
Owner

sdroege commented Feb 18, 2025

Looks good to me. You'd do the AsyncRead / split() part in another PR soonish, or should I take a look at adding that in the next days? I wouldn't want to make a release with only one half :)

@joshtriplett
Copy link
Contributor Author

@sdroege I'm working on AsyncRead now (will be a separate PR).

Less confident in adding a native split; we'll see how that goes.

(Aside: Having to write Poll-based trait implementations is draining; I really hope we get native versions someday that use async fn.)

@sdroege sdroege merged commit a6f8f9a into sdroege:main Feb 18, 2025
8 checks passed
@joshtriplett joshtriplett deleted the bytewriter branch February 18, 2025 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants