quinn
quinn copied to clipboard
Clarify if `Event::ConnectionLost` is always emitted
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
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::ConnectionDrainedwould be even nicer
Exactly this is exposed via Connection::poll_endpoint_events().
Has this issue been already tacked with? If not, can I have a crack at it?
@boserohan91, sure, please go ahead!