swarm: Remove NotifyHandler::All #1880
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Remove `NotifyHandler::All` thus removing the requirement for events send from a `NetworkBehaviour` to a `ProtocolsHandler` to be `Clone`. An implementor of `NetworkBehaviour` can still notify all `ProtocolHandler`s for a given peer by emitting one `NotifyHandler` event per connection to that peer.
romanb
approved these changes
Dec 9, 2020
There was a problem hiding this comment.
I am in favor as this is a good simplification. For the record here, NotifyHandler::All was explicitly introduced for substrate. So if it is no longer used there, that is a good thing. I just hope no-one else relies on it for its protocols and hence needs to adapt the code accordingly.
|
@mxinden Feel free to go ahead with this PR at your convenience. |
|
@tomaka any objections to this change? Are you aware of any (Polkadot) protocols that plan to use |
|
Go ahead, this is a positive change to me. |
10 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Suggestion
With this pull request I would like to suggest removing the ability of a
NetworkBehaviourto notify allProtocolHandlers for a given peer with a singleNetworkBehaviourActionEvent. More concretely I am suggesting to removeNotifyHandler::All. Animplementor of
NetworkBehaviourcan still notify allProtocolHandlers for a given peer by emitting oneNotifyHandlerevent per connection to that peer.Reasoning
The general ability to notify all handlers for a peer forces events send from the behaviour to the handler to be
Clone. Reason is that the swarm needs to forward a clone of the event to each handler. A behaviour that always sends events to a single peer only still has to guarantee that that event isClone.Example
As a concrete example #1838 implementing the libp2p relay spec needs to send substreams between
ProtocolHandlers. Substreams are always send to concreteProtocolHandlers, never to allProtocolHandlers. Today a substream needs to be wrapped in anArc<Mutex<_>>(or similar) in order to beClonebefore one can send it to anotherProtocolHandler. Removing the ability to send to allProtocolHandlers would relief the relay implementation of having to wrap each substream before sending.Alternative
Today the
ExpandedSwarmrequiresTInEventto beClonein order to be able to notify all handlers for a given peer. Instead of removing the feature one could as well introduce a secondTInEvent2which would not require clone, using the former to notify all handlers and the latter to notify specific handlers. A behaviour only requiring the latter could set the former to(). In my eyes this alternative would not be worth the complexity it introduces.Users
As far as I can tell
NotifyHandler::Allis not used in rust-libp2p nor Substrate. Does anyone know of any users ofNotifyHandler::All?Is anyone objecting to this change or has alternative suggestions?