SendStream::finish() no longer errors when connection is closed

[flub]

In Quinn 0.10 SendStream::finish() used to return an error if the connection was already closed. Now it still returns Ok(()).

Full example code at https://gist.github.com/flub/56450fb16ec7c93998e7feedcb5659a0 but here are the relevant server and client pieces:

The server accepts a uni-directional stream, reads some data so that we know both the server and client have opened the stream successfully. Then it closes the connection with a custom code.

After connecting, creating and writing to the stream the client waits for the stream to be closed by the remote. Now the client tries to call .finish(), which I would expect to error. However it returns Ok(()):

async fn run_server(endpoint: Endpoint) {
    println!(
        "[server] accepting: local_addr={}",
        endpoint.local_addr().unwrap()
    );
    let incoming = endpoint.accept().await.unwrap();
    let conn = incoming.await.unwrap();
    println!(
        "[server] connection accepted: addr={}",
        conn.remote_address()
    );
    println!("[server] accepting uni");
    let mut recv_stream = conn.accept_uni().await.unwrap();
    println!("[server] reading");
    let mut buf = [0u8; 5];
    recv_stream.read_exact(&mut buf).await.unwrap();
    println!("[server] closing");
    conn.close(7u8.into(), b"bye");
}

async fn run_client(endpoint: Endpoint, server_addr: SocketAddr) {
    println!(
        "[client] connecting: local_addr={}",
        endpoint.local_addr().unwrap()
    );
    let conn = endpoint
        .connect(server_addr, "localhost")
        .unwrap()
        .await
        .unwrap();
    println!("[client] connected: addr={}", conn.remote_address());
    println!("[client] creating uni");
    let mut send_stream = conn.open_uni().await.unwrap();
    println!("[client] sending hello");
    send_stream.write_all(b"hello").await.unwrap();
    println!("[client] waiting for close");
    let err = conn.closed().await;
    println!("[client] closed: {err:?}");
    let finish = send_stream.finish();
    println!("[client] finish ret: {finish:?}");
}

Is there a conscious reason this behaviour changed? I was expecting finish to return an error at this point.

[Ralith]

Thanks for the report! I believe this was intended. In particular, the current behavior is described accurately by the rustdoc:

May fail if [and only if] finish() or reset() was previously called.

The reasoning here was that it's common for peers to routinely stop a stream or close an entire connection upon reading some data without waiting for the stream to be finished. If the sender finishes all streams and considers finish on stopped streams or closed connections to be an error, then this is very likely to produce unhelpful error signals, and may do so unpredictably depending on timing. We've had a number of issues opened due to this behavior in earlier releases.

I appreciate that it's counterintuitive that calling finish succeeds on a connection you know to be closed, but I think this is consistent with the semantics of finish. Consider that a connection might also become closed in the time between calling finish and the next opportunity for the connection to transmit any packets, resulting in the same lack of effect that you get from calling finish after closing.

I'm interested in feedback about how much sense this makes to you, and where we could improve in documentation or API to make things more obvious or convenient. You can reconstruct the previous behavior by waiting on SendStream::stopped after finishing, but I think the errors you might receive from doing so are usually not interesting.

[flub]

I guess it is reasonable to not give an error, after all the peer already knows no more data will be written on the stream if it is closed and as you say stops and finishes may be crossing on the wire anyway.

I think the docs can be a little better though. Right now I'm not sure how to interpret that error and wonder if calling .finish().ok() is the only sensible way to call it. Which means it could not even return anything from an API point of view?

My attempt at rewriting the docs right now would look something like:

Notify the peer that no more data will ever be written to this stream.

After calling this the remote's [RecvStream::read_to_end] will complete. The remote may already have stopped the stream itself, maybe because it knew this situation already from the application-level protocol. This also means the remote is already aware that no more data will be written on this stream so is considered a success for finish().

To wait for the peer to receive all buffered stream data, see stopped().

Mail fail if finishe() or reset() was previously called locally. This error is harmless and serves only to indicate that the caller may have incorrect assumptions about the stream's state.

[dvc94ch]

there seems to be another behavioural change. calling finish used to wait for the data to be transfered. now on an unidirectional stream, this causes an incoming substream which immediately returns an error. this doesn't seem to be the case for bidirectional streams.

async fn send_request<P: Protocol>(tx: &mut SendStream, req: &P::Request) -> Result<()> {
    tx.write_u16(P::ID).await?;
    let bytes = bincode::serialize(req)?;
    tx.write_all(&bytes).await?;
    tx.finish()?;
    Ok(())
}

                let uni = conn2.accept_uni().await;
                let rx = match uni {
                    Ok(rx) => rx,
                    Err(_) => {
                        // Err(ApplicationClosed(ApplicationClose { error_code: 0, reason: b"" }))
                        break;
                    }
                };

[Ralith]

this causes an incoming substream which immediately returns an error

Sorry, I don't understand what you're trying to communicate here.

Err(ApplicationClosed(ApplicationClose { error_code: 0, reason: b"" }))

This error means that the peer application has closed the connection. If you don't want that to happen, don't do that.

[dvc94ch]

Finish used to mean I'm done sending more data, not close the connection and drop the buffers

[dvc94ch]

How can I get the previous behavior?

[Ralith]

finish does not close the connection. You calling close or dropping all outstanding references to it does. Don't do that until you know it's safe to do so.

[dvc94ch]

so the equivalent of tx.finish().await? is tx.finish()?; tx.stopped().await? now. thanks for the explanation. that fixed all the test cases.