Removing the handler field from FromSwarm::ConnectionClosed in favor of ConnectionHandler::poll_close

[thomaseizinger]

Currently, FromSwarm::ConnectionClosed contains a handler field which makes handling the event klunky in many ways:

• It cannot implement Clone
• It adds a type-parameter
• We cannot unify SwarmEvent and FromSwarm despite them containing the same data

The usecase for returning the handler is to allow the behaviour to extract data from a closed connection which would other get lost as connections can close at any point. Nothing is currently using this functionality but it makes sense in general so we want to keep it.

We can solve this by adding a poll_close function as described by @mxinden below: #3046 (comment)

modified   swarm/src/handler.rs
@@ -158,6 +158,15 @@ pub trait ConnectionHandler: Send + 'static {
         >,
     >;
 
+    /// Gracefully close the [`ConnectionHandler`].
+    ///
+    /// Implementations may transfer one or more events to their [`NetworkBehaviour`] implementation
+    /// by emitting [`Poll::Ready`] with [`Self::ToBehaviour`]. Implementations should eventually
+    /// return [`Poll::Ready(None)`] to signal successful closing.
+    fn poll_close(&mut self, cx: &mut Context<'_>) -> Poll<Option<Self::ToBehaviour>> {
+        Poll::Ready(None)
+    }
+
     /// Adds a closure that turns the input event into something else.
     fn map_in_event<TNewIn, TMap>(self, map: TMap) -> MapInEvent<Self, TNewIn, TMap>
     where

Upon closing a connection, poll_close would be called until it returns Ready(None) and all returned events would be delivered to the behaviour.

[rkuhn]

BTW: the fact that FromSwarm::ConnectionClosed does not contain the reason is the only reason why I need to feed a SwarmEvent back into a network behaviour. The reason is the same as for the handler.

May I suggest the very simple solution of putting both the handler and the error in Arc and call it a day?

[thomaseizinger]

May I suggest the very simple solution of putting both the handler and the error in Arc and call it a day?

I'd prefer a solution where we don't need to employ this. Arc screams "I am shared with other components" whereas that simply isn't true in this case which I think makes this the wrong design.

BTW: the fact that FromSwarm::ConnectionClosed does not contain the reason is the only reason why I need to feed a SwarmEvent back into a network behaviour. The reason is the same as for the handler.

With "reason" you mean the value returned in ConnectionHandler::Close? That is a tricky one. Each ConnectionHandler is constructed by exactly one NetworkBehaviour. Thus, semantically, the error would only belong to that one NetworkBehaviour and be None for all others. However, once one ConnectionHandler closes the connection, all handlers will be shut down.

Overall, I agree with you that the error should be fed back to the NetworkBehaviour and not be emitted through the SwarmEvent. I'd like us to move away from SwarmEvents where possible.

[rkuhn]

I proposed Arc because the current usage does indeed lead to the value being shared — once handed out of the swarm, once handed down inside the swarm’s behaviour. If there is only one destination then the problems will of course go away. OTOH this constraint can be severely limiting to a generic recipient.

[thomaseizinger]

@elenaf9 suggested in #3099 (comment), that we could just remove the handler from the ConnectionClosed event. This would certainly be the easiest solution but is obviously a breaking change without a replacement. I'll reach out to our users and see if anyone is currently relying on this!

[divagant-martian]

Hi, we don't use this functionality atm 👍

[thomaseizinger]

With #3927, removing the handler field is the only blocker for expressing SwarmEvent as a composition of FromSwarm and NetworkBehaviour::ToSwarm.

Fundamentally, the only use of returning the ConnectionHandler back to the NetworkBehaviour is that a NetworkBehaviour can extract state from a now closed connection.

We could also solve this differently:

1. Introduce ConnectionEvent::Closing event that notifies the ConnectionHandler that the connection is going away. The reasons for this can be:

• an io::Error
• all handlers returned KeepAlive::No
• Swarm or NetworkBehaviour requested for the connection to be shut-down
• Until swarm: Remove ConnectionHandler::Error #3591 is finished, it could also be the reason the ConnectionHandler itself went away but if we decide to implement this before swarm: Remove ConnectionHandler::Error #3591 then I'd not include that.

2. Extend swarm::Connection to keep polling a ConnectionHandler once it is shutting down until it returns Poll::Pending.
3. Ignore any events other than ConnectionHandlerEvent::Custom and deliver the remaining events to the NetworkBehaviour.

This would allow a ConnectionHandler to return unfinished work back to its behaviour. Implementations would be encouraged to do this as quickly as possible to not delay the shutdown for too long.

Thoughts?

[mxinden]

Extend swarm::Connection to keep polling a ConnectionHandler once it is shutting down until it returns Poll::Pending.

How about adding the following. Enabling a graceful shutdown of a ConnectionHandler. Allowing the ConnectionHandler to transfer any in-flight state back to the NetworkBehaviour.

modified   swarm/src/handler.rs
@@ -158,6 +158,15 @@ pub trait ConnectionHandler: Send + 'static {
         >,
     >;
 
+    /// Gracefully close the [`ConnectionHandler`].
+    ///
+    /// Implementations may transfer one or more events to their [`NetworkBehaviour`] implementation
+    /// by emitting [`Poll::Ready`] with [`Self::ToBehaviour`]. Implementations should eventually
+    /// return [`Poll::Ready(None)`] to signal successful closing.
+    fn poll_close(&mut self, cx: &mut Context<'_>) -> Poll<Option<Self::ToBehaviour>> {
+        Poll::Ready(None)
+    }
+
     /// Adds a closure that turns the input event into something else.
     fn map_in_event<TNewIn, TMap>(self, map: TMap) -> MapInEvent<Self, TNewIn, TMap>
     where

(Note the default implementation.)

[thomaseizinger]

This is a great idea!

I'll update the original issue description with this solution.