search

quinn-rs / quinn

Async-friendly QUIC implementation in Rust
Apache License 2.0
3.85k stars 394 forks source link

Return `UnknownStream` on `finish` a closed stream #1575

Closed EAimTY closed 1 year ago

EAimTY commented 1 year ago

Fixes #1569

Matthias247 commented 1 year ago

Should this really return an error? I think finishing a stream could be considered an idempotent action. And there calling it multiple times with the same Ok result seems ok to me. Same goes for a stream reset which is also somewhat idempotent - but less so since the error code passed to each call can be different.

No strong feelings however - a caller could always wrap the stream to get the desired behavior (error on 2nd call or Ok on 2nd call)

Ralith commented 1 year ago

Calling finish on a stream that was previously reset should be an error, since otherwise the caller might conclude that all data was transmitted. We could track whether a stream was reset, but that'd require extra state in quinn and could not be sensibly implemented in quinn-proto. Consistently returning UnknownStream for streams which may have been closed in any way seems like the best path.

Matthias247 commented 1 year ago

That makes sense. I assumed this was just for the case where the stream fully finished by having sent all the data, and not that it had been cancelled via a reset

Ralith commented 1 year ago

This specific issue is, but a graceful fix (per above) means not tracking that information.

Ralith commented 1 year ago

Went ahead and drafted the approach I described in https://github.com/quinn-rs/quinn/pull/1588, since it's affecting multiple users. Thanks for bringing this to our attention!