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.

[thomaseizinger]

Instead of passing the Handler along, how about we add a callback to ConnectionHandler where it can produce a final message that is then passed to the behaviour:

trait ConnectionHandler {
	fn on_close(self) -> Option<Self::OutEvent>;
}

[thomaseizinger]

Instead of passing the Handler along, how about we add a callback to ConnectionHandler where it can produce a final message that is then passed to the behaviour:

trait ConnectionHandler {
	fn on_close(self) -> Option<Self::OutEvent>;
}

Together with #3099, this would remove all mentions of Handler in the event I think.

[mxinden]

Would that on_close OutEvent implement Clone? If not, what do we gain from the on_close method? If it does, how do we pass streams around in libp2p-relay through the OutEvent?

[thomaseizinger]

Would that on_close OutEvent implement Clone? If not, what do we gain from the on_close method? If it does, how do we pass streams around in libp2p-relay through the OutEvent?

FromSwarm can implement Clone conditionally on the OutEvent being Clone!

It makes a lot more sense for the event to be clone rather than the entire handler. It allows users to clone the event in most cases.

For the libp2p-relay behaviour, FromSwarm would simply not be clone.

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