swarm/: Support generic connection management through NetworkBehaviour#2828
swarm/: Support generic connection management through NetworkBehaviour#2828mxinden wants to merge 12 commits intolibp2p:masterfrom
Conversation
When generating the `OutEvent` for a `NetworkBehaviour`, add the `where` clause of the `NetworkBehaviour` `struct` to the generated `enum`.
When generating an `OutEvent` `enum` definition for a user, derive `Debug` for that `enum`. Why not derive `Clone`, `PartialEq` and `Eq` for the generated `enum` definition? While I think it is fine to require all sub-`OutEvent`s to implement `Debug`, I don't think the same applies to traits like `Clone`. I suggest users that need `Clone` to define their own `OutEvent`.
…eneric-connection-management
thomaseizinger
left a comment
thomaseizinger
left a comment
There was a problem hiding this comment.
Hope you don't mind the early, unsolicited review :)
I've added a few thoughts but overall, this looks really interesting. I like how we move more and more to a plugin system and libp2p-swarm is becoming a kind of "executor" for these plugins.
| # TODO: Should this really be a default? | ||
| "connection-limit", |
There was a problem hiding this comment.
Long term, I would say no because of #2173. For consistency with the current way of doing things, probably yes.
| @@ -0,0 +1,24 @@ | |||
| [package] | |||
| name = "libp2p-connection-limit" | |||
There was a problem hiding this comment.
Does this have to be its own crate? I think this could also live in libp2p-swarm.
It can still be feature-flagged there if we want but looking at the dependencies of this crate, we don't really gain much at the moment from separating this out.
There was a problem hiding this comment.
Separate crate forces us to deliver on the promise of being able to implement ones own.
That said, agreed that we don't gain much more and instead add complexity.
|
|
||
| impl NetworkBehaviour for Behaviour { | ||
| type ConnectionHandler = DummyConnectionHandler; | ||
| type OutEvent = (); |
There was a problem hiding this comment.
Perhaps use Void instead?
| // TODO: Needed in the first place? Are there some common errors that we want? | ||
| #[derive(Debug)] | ||
| pub enum ReviewDenied { | ||
| Error(Box<dyn Error>), |
There was a problem hiding this comment.
This should definitely have a 'static bound.
| } | ||
| } | ||
|
|
||
| // TODO: Needed in the first place? Are there some common errors that we want? |
There was a problem hiding this comment.
Perhaps just having these function return Box<dyn Error + 'static> is enough?
There was a problem hiding this comment.
Something like LimitReached seems to very common to me, so would be nice to have that as a specifc one, maybe with an optional message for customizability
| for _ in 0..outbound { | ||
| swarm | ||
| .dial( | ||
| DialOpts::peer_id(PeerId::random()) |
There was a problem hiding this comment.
Does PeerId::random only work here because we never poll the swarm?
I think this is misleading, why not dial without PeerId instead?
|
|
||
| #[test] | ||
| fn enforces_pending_outbound_connection_limit() { | ||
| fn prop(outbound: u8) { |
There was a problem hiding this comment.
Sorry for OT but we have so many different ways of testing swarms in our repo, it makes me uneasy every time 😅
There was a problem hiding this comment.
I'll put this on my list to work on from September forward :D
| /// similar mechanisms. | ||
| external_addrs: Addresses, | ||
|
|
||
| // TODO: I think this should move into the connection manager behaviour as well. |
There was a problem hiding this comment.
Into the behaviour or into a behaviour. I was thinking the same thing when I thought about this topic. Keeping a record of banned peers can likely be a NetworkBehaviour itself IMO.
| _peer_id: Option<PeerId>, | ||
| // TODO: Maybe an iterator is better? | ||
| _addresses: &[Multiaddr], | ||
| _endpoint: Endpoint, |
There was a problem hiding this comment.
Instead of passing the endpoint, would it make sense to have two different callbacks, one for incoming and one for outgoing?
| } | ||
|
|
||
| // TODO: Make sure this and all the methods below are really implemented across all behaviours. | ||
| fn review_pending_connection( |
There was a problem hiding this comment.
I am not fully sold on the "review" terminology but I also can't think of something better off the top of my head.
There was a problem hiding this comment.
Not a fan of "review" either. Grateful for naming suggestions!
|
I am closing this in favor of #3099. |
3 participants
Description
This pull request enables generic connection management by implementing option (2) of #2824:
Very much work in progress at this point.
Links to any relevant issues
Open Questions
Change checklist