quinn icon indicating copy to clipboard operation
quinn copied to clipboard

Clarify if `Event::ConnectionLost` is always emitted

Open FlorianUekermann opened this issue 2 years ago • 3 comments

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

FlorianUekermann avatar Feb 18 '23 21:02 FlorianUekermann

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().

Ralith avatar Mar 14 '23 18:03 Ralith

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

boserohan avatar Nov 06 '23 13:11 boserohan

@boserohan91, sure, please go ahead!

djc avatar Nov 06 '23 13:11 djc