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

Implement Sink and Stream for WsClient #907

Merged
merged 5 commits into from
Oct 30, 2021

Conversation

FSMaxB
Copy link
Contributor

@FSMaxB FSMaxB commented Oct 8, 2021

This implements Sink and Stream for WsClient.

Why is this useful?

If you use tokio-tungstenite as your websocket client, you will usually have a tokio-tungstenite::WebSocketStream to work with, which exclusively uses the Sink and Stream traits for sending and receiving messages. Implementing Sink and Stream for WsClient as well allows for writing code that works with both the real tokio-tungstenite::WebSocketStream and WsClient by relying only on the Sink and Stream traits in the implementation, especially if you combine it with #903 which provides conversions between warp::ws::Message and tungstenite::protocol::Message.

Why not use tokio::sync::mpsc::unbounded_channel

Initially I tried to implement this using the tokio mpsc channels that WsClient was using, but found it to be impossible.
Implementing Stream isn't an issue because of the Stream implementation provided in tokio-stream and because tokio::sync::mpsc::UnboundedReceiver has a poll_recv method exactly for that purpose.
Implementing Sink however doesn't work because there is no way that poll_flush or poll_close can notify a task to be polled again.

Therefore this PR switches the channel implementation from tokio to futures, since futues::channel::mpsc::UnboundedSender and UnboundedReceiver already implement Sink and Stream and the implementation on WsClient then only need to proxy their calls to the underlying channel.

@FSMaxB
Copy link
Contributor Author

FSMaxB commented Oct 10, 2021

I'm going to rebase this shortly since merging #906 broke this pull request. This PR now also needs to add futures-channel on top of the futures-util dependency currently on master.

@jxs jxs merged commit 1948044 into seanmonstar:master Oct 30, 2021
@jxs
Copy link
Collaborator

jxs commented Oct 30, 2021

Hi, and sorry for the delay! Makes sense, thanks!

@FSMaxB FSMaxB deleted the wsclient-sink-stream branch October 30, 2021 13:40
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.

None yet

2 participants