quinn-rs / quinn

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

Clarify if `Event::ConnectionLost` is always emitted #1495

Open FlorianUekermann opened 1 year ago

FlorianUekermann commented 1 year ago

The docs for Event::ConnectionLost and L113 in the lifecycle test suggest that no such Event is emitted on the side closing the connection. However, from the documentation on is_closed() and the existence of ConnectionError::LocallyClosed I expected otherwise. Am I confusing something here, or is the documentation a bit misleading?

As a user I think emitting Event::ConnectionLost on the closing side would be nice, because it results in nicer code in the driver future (and adding a Event::ConnectionDrained would be even nicer). https://github.com/quinn-rs/quinn/blob/390f2ebeba910ef4c88ed98711f7271a4a530b5d/quinn-proto/src/connection/mod.rs#L1035-L1036 https://github.com/quinn-rs/quinn/blob/390f2ebeba910ef4c88ed98711f7271a4a530b5d/quinn-proto/src/connection/mod.rs#L3199-L3202

https://github.com/quinn-rs/quinn/blob/390f2ebeba910ef4c88ed98711f7271a4a530b5d/quinn-proto/src/connection/mod.rs#L3338-L3339 https://github.com/quinn-rs/quinn/blob/390f2ebeba910ef4c88ed98711f7271a4a530b5d/quinn-proto/src/tests/mod.rs#L92-L118

Ralith commented 1 year ago

Sorry, this thread slipped through the cracks!

I agree the doc for is_closed is misleading here. LocallyClosed is currently only used at the quinn layer. I don't immediately see any serious problems a refactor to emit ConnectionLost when closing locally, though it's not obvious that it would save more than a single line in the driver. Are you interested in making a PR?

adding a Event::ConnectionDrained would be even nicer

Exactly this is exposed via Connection::poll_endpoint_events().

boserohan commented 10 months ago

Has this issue been already tacked with? If not, can I have a crack at it?

djc commented 10 months ago

@boserohan91, sure, please go ahead!