quinn-rs / quinn

Async-friendly QUIC implementation in Rust
Apache License 2.0
3.76k stars 380 forks source link

Must Write to a Bidirectional Stream Before I can Read From It #1425

Closed zicklag closed 1 year ago

zicklag commented 1 year ago

Hey there! I'm just getting started with Quinn and I'm really liking it so far.

I just ran into a weird behavior where I have the server open up a bidirectional stream and then start listening for messages from the client.

The strange part is that reading from the stream on the server will time out for any requests sent to it, unless I either drop the SendStream or otherwise write something to it first. So it's like I have to use the SendStream somehow ( and even dropping it counts as "using" it ), before the RecvStream is usable.

For my use-case it looks like I can write an empty slice to the SendStream before I try to read on the RecvStream and that fixes the issue, but it's a little strange.

zicklag commented 1 year ago

Now I'm also noticing that data I send.write_all().await?; doesn't get sent unless I send.finish().await?; with doesn't seem right either. :/

I even tried send.flush().await?; and it still didn't send a response unless I finished()'ed it.

Are you not able to have both a send and a recv active at the same time?

zicklag commented 1 year ago

Here's my code. It's not exactly minimal, with the deserialization loop, but it's still pretty small in scope.

/// After a matchmaker connection is established, it will open a bi-directional channel with the
/// client.
///
/// At this point the client is free to engage in the matchmaking protocol over that channel.
async fn impl_matchmaker(conn: Connection) -> anyhow::Result<()> {
    // Open the matchmaking channel with the client
    let (mut send, mut recv) = conn.open_bi().await?;

    // A write to the stream is required before any messages can be received for some reason
    send.write(&[]).await?;

    let mut raw_buf = [0u8; 32];
    let mut cobs_buf: CobsAccumulator<256> = CobsAccumulator::new();

    while let Ok(count) = recv.read(&mut raw_buf).await {
        let count = count.unwrap_or(0);

        // Finished reading input
        if count == 0 {
            break;
        }

        let buf = &raw_buf[..count];
        let mut window = buf;

        'cobs: while !window.is_empty() {
            window = match cobs_buf.feed::<MatchmakerRequest>(window) {
                FeedResult::Consumed => break 'cobs,
                FeedResult::OverFull(new_wind) | FeedResult::DeserError(new_wind) => new_wind,
                FeedResult::Success { data, remaining } => {
                    if let Err(e) = handle_request(data, &mut send).await {
                        error!("Error handling request: {e:?}");
                    }

                    remaining
                }
            };
        }
    }

    Ok(())
}

async fn handle_request(request: MatchmakerRequest, send: &mut SendStream) -> anyhow::Result<()> {
    info!(?request, "Got matchmaker request");

    let message = postcard::to_allocvec(&MatchmakerResponse::MatchId(7))?;
    send.write_all(&message).await?;

    // Uncommenting this line is required to get data to send
    // send.finish().await?;

    Ok(())
}
zicklag commented 1 year ago

I'm realizing that it might be more idiomatic QUIC just to use a separate stream bidirectional channel for every message instead of looping over streamed messages anyway, so I think this wont be an issue for me in my use-case.

Still, I feel like I'm either missing something, or there is a bug, for this to be happening like it is, so I'll leave this open unless you guys want to close it.

djc commented 1 year ago

IIRC if you open a stream the peer won't be notified until you start writing to it, so if the server has nothing to send maybe the client should initiate the stream?

zicklag commented 1 year ago

Yeah, I had needed the server to finish something before the client could make the next request, so I thought having the server open the channel could make sense, but think you're right and it makes more sense for the server just to have a response to the request that needs to be finished first, than to have it initiate the next channel.

Ralith commented 1 year ago

Stream writes are always sent immediately, whether or not you've finished the stream, but as @djc says, the peer doesn't see streams until you do something with them. If you can provide a minimal self-contained test case where a stream write does not appear to be sent, I can probably find time to have a look.

I had needed the server to finish something before the client could make the next request

You could just not process future messages from the client until you're done with that operation.

zicklag commented 1 year ago

I think I might have been confused about something, because after messing with it a little more things seem to be making sense now.

If I run into it again, I'll boil it down to a minimal reproducible example. Thanks!

spoorn commented 1 year ago

I'm running into the same problem here, but with uni streams. I have a server that is spawned on a thread and infinitely loops, and a client that connects with the server. The server opens up a uni stream and writes some bytes to it, but accept_uni() waits indefinitely on the client side even if I call send.flush() on the server.

I wrote up a quick example here: https://github.com/spoorn/quinnflush_mre/blob/main/src/main.rs

You can pull it locally and just run cargo run to see that the connection is established, server write() and flush() finish, but the client is indefinitely waiting for the uni stream (or at least until timeout)

Edit: Here's an example where I tried using bidirectional streams instead, where I send a packet as a handshake from both client and server, but then the server hangs when trying to read that handshake from the client: https://github.com/spoorn/quinnflush_mre/blob/bidi/src/main.rs

Ralith commented 1 year ago

Your server hangs because your code contains an infinite loop. A thread that is busy looping forever cannot engage in other activity like sending packets.

spoorn commented 1 year ago

Ah yeah, I added a sleep to the server loop and it indeed is sending the packets now, without needing flush() or finish(). Thanks!