Open flub opened 2 months ago
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()
orreset()
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 finish
es 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.
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 forfinish()
.To wait for the peer to receive all buffered stream data, see
stopped()
.Mail fail if
finishe()
orreset()
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.
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;
}
};
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.
Finish used to mean I'm done sending more data, not close the connection and drop the buffers
How can I get the previous behavior?
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.
so the equivalent of tx.finish().await?
is tx.finish()?; tx.stopped().await?
now. thanks for the explanation. that fixed all the test cases.
In Quinn 0.10
SendStream::finish()
used to return an error if the connection was already closed. Now it still returnsOk(())
.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 returnsOk(())
:Is there a conscious reason this behaviour changed? I was expecting finish to return an error at this point.