quinn-rs / quinn

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

Reliable stream usage guidelines #1593

Closed coder137 closed 1 year ago

coder137 commented 1 year ago

As per my understanding of the docs opening a unidirectional and bidirectional stream is cheap and instantaneous.

However, For my application, I only need one bidirectional stream between 2 peers (one opens and one accepts)

  1. Is it fine to open a bidirectional reliable stream and keep it open forever?
  2. To make sure the packets are decoded with the dynamic length I have some logic like this, Is there any problem with this implementation? I am not very familiar with the internals of QUIC
async fn write_reliable(send: &mut SendStream, buffer: &mut Bytes) -> Result<()> {
    send.write_u32(buffer.len() as u32).await?;
    send.write_all_buf(buffer).await?;
    send.flush().await?;
    Ok(())
}

async fn read_reliable(recv: &mut RecvStream) -> Result<Bytes> {
    let sz = recv.read_u32().await? as usize;
    let mut buffer = BytesMut::with_capacity(sz);
    let rsz = recv.read_buf(&mut buffer).await?;
    if rsz != sz {
        Err(anyhow::anyhow!("Sz != RSz"))
    } else {
        Ok(buffer.into())
    }
}
  1. Would the Quinn team still recommend opening a unidirectional/bidirectional stream on demand? I am writing an asynchronous application where the write and read need to happen in a full duplex mode so will be using unidirectional streams to send/recv data.
  2. I see this issue (https://github.com/quinn-rs/quinn/issues/950), are there any recommendations for closing a connection WHEN reliable streams / unreliable datagram packets are still being transmitted/received? For example: I would like to do a connection.close and trigger an ApplicationClose to the connected peer. However the reliable stream fails first instead of connection.close being triggered.

Diagrams

[My method] Opening bidirectional channel once

Problem: When closing the connection the reliable channel fails first and the other peer does not get the connection.close error code.

sequenceDiagram
    title Forever open Bidirectional channel
    note over Peer1: Open Bi
    note over Peer2: Accept Bi
    par Peer 1 send
        Peer1 ->> Peer2: Tx
    and Peer 2 send
        Peer2 ->> Peer1: Tx
    end
    note over Peer1: Close connection + Send error code
    note over Peer2: Recv error code + Close connection

Opening unidirectional channels on demand

Is there a performance impact here?. This is probably my only concern.

This design should solve the flaws of the above design. If you see any other problem please point that out

sequenceDiagram
    title Unidirectional channels on demand
    par Peer 1 send
        note over Peer1: Open Uni
        note over Peer2: Accept Uni
        Peer1 ->> Peer2: Tx
        note over Peer1: Close gracefully
    and Peer 2 send
        note over Peer2: Open Uni
        note over Peer1: Accept Uni
        Peer2 ->> Peer1: Tx
        note over Peer2: Close gracefully
    end
    note over Peer1,Peer2: Any peer can close the connection with an error code
djc commented 1 year ago

It is fine to open a bidirectional stream and keep it open forever. However, it's probably not the most efficient approach. For example, you seem to be using length-prefixing to delineate messages, which is, to some extent, duplicating functionality that the QUIC protocol already supplies for you. So instead, it would likely make more sense to use one (unidirectional) stream per message. If there is a strict request/response coupling, you might have one bidirectional stream per message, where the "client" starts sending a message and the "server" then responds on the same stream -- the "client" can signal its message is complete by closing its send part of the stream IIRC.

(I've forgetten the details of how connection and stream closes are ordered, but I encourage you to read the source -- we've tried hard to make it (relatively) easy to understand. Someone else may have this knowledge paged in better.)

Ralith commented 1 year ago

Length-prefixed messages within a single long-lived stream are still a good solution if you need them to be strictly ordered, which is common.

I would like to do a connection.close and trigger an ApplicationClose to the connected peer. However the reliable stream fails first instead of connection.close being triggered.

I'm not sure what you mean by "the reliable stream fails first". Closing a connection by definition terminates all streams on that connection. If you don't want a stream to be terminated, don't close the connection yet. That might mean waiting for an application-layer response to know when a stream is no longer needed.

coder137 commented 1 year ago

Thanks for the comments @djc and @Ralith.

I was able to successfully update my application logic by opening a unidirectional channel on demand instead of the length prefixing option. The code design and implementation were simpler when doing it this way.

To clarify: Are there any performance impacts when opening a unidirectional channel on demand (from the Quinn internals perspective)? Compared to the length prefixing option?

Ralith commented 1 year ago

Streams are very cheap. The best way to get a more specific answer than that is profiling.

coder137 commented 1 year ago

Streams are very cheap. The best way to get a more specific answer than that is profiling.

Thanks for the confirmation. Will profile it and update my results here eventually.