refactor(swarm)!: remove handler from NetworkBehaviourAction::Dial
#3328
Conversation
thomaseizinger
force-pushed
the
2824-no-handler-in-dial
branch
from
e9ed7d6 to
8e0d7d8
Compare
|
Converting to draft until #3327 is merged to prevent accidental merges. |
…ibp2p into 2824-unique-connection-ids
Instead of carrying around an enum and parsing its contents later, directly set the correct values as we construct the final `DialOpts`.
Otherwise the num-established counter is wrong.
We no longer add connections for banned peers to the pool so we don't have to filter them later.
|
This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏 |
|
This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏 |
jxs
left a comment
jxs
left a comment
There was a problem hiding this comment.
Overall looks good to me Thomas! I am not very familiar with dctur and relayyet so Max and Elena should also chime in.
| Ok(()) | ||
| } | ||
|
|
||
| pub fn spawn_connection( |
There was a problem hiding this comment.
yeah I think this fits here better, and being handled by the Swarm on Swarm::handle_pool_event after receiving ConnectionEstablished rather than on the Pool before returning it 🚀
There was a problem hiding this comment.
Yeah it turned out surprisingly clean and fixes up some weird state tracking where we had to "filter" connection IDs that are banned. Now we just don't spawn the connection in the first place :)
mxinden
left a comment
mxinden
left a comment
There was a problem hiding this comment.
This still needs further investigation:
swarm/src/lib.rs
Outdated
| // Mark the connection for the banned peer as banned, thus withholding any | ||
| // future events from the connection to the behaviour. | ||
| self.banned_peer_connections.insert(id); | ||
| self.pool.disconnect(peer_id); |
There was a problem hiding this comment.
Given that we drop connection here, is the call to pool.disconnect still needed? There should be no connection to the peer in the pool. The previous Swarm::ban_peer_id should have already called pool.disconnect.
Also, if I am not mistaken, we would previously gracefully close connections to banned peers via the pool.disconnect call. Given that connection is now just dropped, this is no longer the case, right?
There was a problem hiding this comment.
Given that we drop
connectionhere, is the call topool.disconnectstill needed? There should be no connection to the peer in the pool. The previousSwarm::ban_peer_idshould have already calledpool.disconnect.
Originally I thought that we might have other connections to this peer but I guess that isn't actually possible because we would have disconnected those at the time of banning the peer.
Also, if I am not mistaken, we would previously gracefully close connections to banned peers via the
pool.disconnectcall. Given thatconnectionis now just dropped, this is no longer the case, right?
True. I didn't bother much because this code will go away anyway once we implement it via the new NetworkBehaviour callbacks but yeah, it is a change in behaviour so we should probably keep this behaviour.
This made me think though, we should probably gracefully close connections in #3254 that have been rejected by a NetworkBehaviour.
There was a problem hiding this comment.
I've pushed some more commits, let me know what you think!
There was a problem hiding this comment.
Changes look good to me, thanks!
|
@thomaseizinger ready to merge from my end. Please trigger mergify once you are ready. Looking forward for this to land. |
libp2p#3328) We create the `ConnectionId` for the new connection as part of `DialOpts`. This allows `NetworkBehaviour`s to accurately track state regarding their own dial attempts. This patch is the main enabler of libp2p#3254. Removing the `handler` field will allow us to deprecate the `NetworkBehaviour::new_handler` function in favor of four new ones that give more control over the connection lifecycle.
We currently try to forward pending handler events in the relay client behavior twice: in `handle_established_{in, out}bound_connection` and in `on_swarm_event()` (introduced in #3328).
This is redundant because by the second time we try to forward it there won't be any pending event anymore.
This PR removes the duplicated logic from `on_swarm_event`. Forwarding it in `handle_established_{in, out}bound_connection` is more convenient as we still have direct access to the handler there and don't need to go through the swarm.
However, it requires that in `handle_established_{in, out}bound_connection` the event is also removed when the connection is relayed, in which case the event is simply discarded because the commands are only valid on direct connections.
Pull-Request: #5765.
Description
We create the
ConnectionIdfor the new connection as part ofDialOpts. This allowsNetworkBehaviours to accurately track state regarding their own dial attempts.This patch is the main enabler of #3254. Removing the
handlerfield will allow us to deprecate theNetworkBehaviour::new_handlerfunction in favor of four new ones that give more control over the connection lifecycle.Notes
NetworkBehaviours to manage connections #3254.ConnectionIds globally unique #3327.DialOpts#3335Please review this design critically. I believe it has value on its own but at the end of the day, we only do this for #3254.
Links to any relevant issues
Open Questions
Change checklist