snapview / tokio-tungstenite

Future-based Tungstenite for Tokio. Lightweight stream-based WebSocket implementation
MIT License
1.88k stars 236 forks source link

Websocket stops receiving message unexpectedly #345

Closed Lazauya closed 3 months ago

Lazauya commented 3 months ago

I've recreated this issue because the last was glitched on my end. I've been trying to get tokio-tungstenite to work, but it randomly stops receiving messages (even though the messages appear to send successfully(?)). I made a small reproducable example here. As was suggested by @agalakhov in https://github.com/snapview/tokio-tungstenite/issues/343#issuecomment-2267965665_ I tried putting the receiving code in it's own future/task, but that seems to have had the opposite effect (no messages are ever received from the server). It seems using a simple echo server (e.g. wss://echo.websocket.org/) lets it receive messages, so my issue might be in my server implementation. However, my server implementation looks almost identical to the examples.

daniel-abramov commented 3 months ago

So, first of all, this sounds like a bug in a user code.

I decided to give it a try and look into your repository:

Lazauya commented 3 months ago

So, first of all, this sounds like a bug in a user code.

I decided to give it a try and look into your repository:

* I did not verify the client code because it has a lot of irrelevant code and dependencies. If you would remove all `bevy`, `mde`, `postcard` stuff and reduce it down to plain tokio/tungstenite I'd be glad to check it. But I noticed that you ignore all the errors by using `_` often.

* The server code does not look too bad (from the perspective of tokio/tungstenite), but note that you ignore all reading errors by matching with `_` when reading a message. Should the client connection get interrupted or should any other error occur, you **won't** see an error (instead, it will end up returning `Ok(())`). I suggest treating error cases explicitly so as not to create a false impression of things going well even when they fail.

Thank you for the reply. mde is just the library in the package itself. postcard is for converting structs to Vec<u8>, so it would be difficult to remove and shouldn't affect whether the messages are received (I will eat my hat on live television if that's the issue). I can attempt to remove bevy and bevy_tokio_tasks but it will be difficult and take some time as they simplify the code for the problem I'm trying to solve by a lot. In the meantime, here's where I try receiving the message in the client:

let (socket, _) = connect_async("ws://127.0.0.1:9002")
    .await
    .expect("Could not connect to server");
let (mut write, mut read) = socket.split();

let writer = {
    async move {
        loop {
            let mut iter = req_rec.try_iter();
            match iter.next() {
                Some(message) => {
                    write
                        .send(Binary(to_stdvec(&message).unwrap()))
                        .await
                        .expect("TODO: panic message");
                }
                _ => {}
            }
        }
    }
};

let reader = async {
    loop {
        println!("Try recv");
        match read.next().await {
            Some(Ok(Binary(bin))) => {
                println!("Try receive");
                match from_bytes::<NetworkResponse>(&bin).unwrap() {
                    Numbers(fetched) => {
                        resp_sen
                            .send(fetched)
                            .expect("Could not send terrain update");
                    }
                }
                println!("Received");
            }
            _ => {
                println!("Other");
            }
        }
    }
};

let futures: [Pin<Box<dyn Future<Output = ()> + Send>>; 2] =
    [Box::pin(reader), Box::pin(writer)];

future::join_all(futures).await;

This matches the examples given for the most part, but does anything potentially look out of place?

daniel-abramov commented 3 months ago

I can attempt to remove bevy and bevy_tokio_tasks but it will be difficult and take some time as they simplify the code for the problem I'm trying to solve by a lot.

Yes, but that's the point - the bug is outside of the tokio-tungstenite. Essentially, we're debugging the user-level code.

In the meantime, here's where I try receiving the message from the client:

There are many potential issues with the code. First of all, the usage of a crossbeam channel looks incorrect in that context - the writer loop will be stuck in an infinite loop should there be no messages received on the crossbeam channel, meaning it would block the other future (the reader) from making any progress. This would perfectly explain the "I tried putting the receiving code in it's own future/task, but that seems to have had the opposite effect (no messages are ever received from the server)".

So my suggestions:

  1. Handle all error codes, remove the _ => {} part, and replace it with something meaningful to see when/if this branch is executed.
  2. Remove the crossbeam channel at that location and replace it with an unbounded channel from tokio::sync::mpsc.
  3. I'd also suggest going through the https://tokio.rs docs to better understand how the futures/tasks work and why you should not put CPU-bound tasks there (especially infinite loops).

Hope this helps!