client/network-gossip: Integrate GossipEngine tasks into Future impl #4767
Conversation
`GossipEngine` spawns two tasks, one for a periodic tick, one to forward messages from the network to subscribers. These tasks hold an `Arc` to a `GossipEngineInner`. To reduce the amount of shared ownership (locking) this patch integrates the two tasks into a `Future` implementation on the `GossipEngine` struct. This `Future` implementation can now be called from a single owner, e.g. the `finality-grandpa` `NetworkBridge`. As a side effect this removes the requirement on the `network-gossip` crate to spawn tasks and thereby removes the requirement on the `finality-grandpa` crate to spawn any tasks. This is part of a greater effort to reduce the number of owners of components within `finality-grandpa`, `network` and `network-gossip` as well as to reduce the amount of unbounded channels. For details see d4fbb89, f0c1852 and 5afc777.
bin/node-template/src/service.rs
Outdated
| @@ -160,7 +160,6 @@ pub fn new_full<C: Send + Default + 'static>(config: Configuration<C, GenesisCon | |||
| grandpa_link, | |||
| service.network(), | |||
| service.on_exit(), | |||
| service.spawn_task_handle(), | |||
There was a problem hiding this comment.
None of finality-grandpa nor network-gossip need an executor to spawn tasks after this patch. Thus the logic passing said executor to those crates is removed in bin/node, bin/node-template, client/service and a lot of tests.
| @@ -444,6 +442,12 @@ impl<B: BlockT, N: Network<B>> Future for NetworkBridge<B, N> { | |||
| } | |||
| } | |||
|
|
|||
| match self.gossip_engine.poll_unpin(cx) { | |||
There was a problem hiding this comment.
The GossipEngine needs to be polled by the NetworkBridge.
| cx.waker().wake_by_ref(); | ||
| } | ||
| } | ||
|
|
||
| Poll::Pending | ||
| Future::poll(Pin::new(&mut self.network), cx) |
There was a problem hiding this comment.
Bug originally fixed in #4766.
| @@ -36,101 +37,42 @@ pub struct GossipEngine<B: BlockT> { | |||
| struct GossipEngineInner<B: BlockT> { | |||
| state_machine: ConsensusGossip<B>, | |||
| network: Box<dyn Network<B> + Send>, | |||
| periodic_maintenance_interval: futures_timer::Delay, | |||
| network_event_stream: Compat01As03<Box<dyn Stream01<Error = (), Item = Event> + Send>>, | |||
| engine_id: ConsensusEngineId, | |||
There was a problem hiding this comment.
Once this lands there is no need for GossipEngineInner anymore, thus the engine_id does not need to be duplicated either.
| })); | ||
|
|
||
| let gossip_engine = GossipEngine { | ||
| inner: inner.clone(), | ||
| engine_id, | ||
| }; | ||
|
|
||
| let res = executor.spawn({ |
There was a problem hiding this comment.
These are the two tasks previously being spawned. You can find the new Future implementation further below.
| fn poll(mut self: Pin<&mut Self>, cx: &mut Context) -> Poll<Self::Output> { | ||
| let this = &mut *self; | ||
|
|
||
| while let Poll::Ready(Some(Ok(event))) = this.network_event_stream.poll_next_unpin(cx) { |
There was a problem hiding this comment.
This is copy and pasted from the previously spawned logic. In the long run I think we should at least log errors on this channel.
|
🛑 I have not yet created the corresponding Polkadot pull-request. But in the meantime I think this is worth taking a look. 🛑 (Polkadot will also need to poll the |
|
Sorry for the delay. Something like: impl<B: BlockT> Future for GossipEngine<B> {
type Output = ();
fn poll(self: Pin<&mut Self>, cx: &mut Context) -> Poll<Self::Output> {
let inner = self.inner.lock();
let fut = inner.run();
futures::pin_mut!(fut);
fut.poll(cx)
}
}
impl<B: BlockT> GossipEngineInner<B> {
async fn run(&mut self) { ... }
}The |
There was a problem hiding this comment.
changes lgtm, needs a merge with master. I also like what @tomaka proposed.
With paritytech/substrate#4767 a `GossipEngine` does not spawn its own tasks, but relies on its owner to poll it. This patch removes the unneeded executor argument and spawns the gossip engine as an orphaned task onto the executor instead.
With paritytech/substrate#4767 a `GossipEngine` does not spawn its own tasks, but relies on its owner to poll it. This patch removes the unneeded executor argument and spawns the gossip engine as an orphaned task onto the executor instead.
|
I talked with @tomaka in regards to the suggestion above: Using an
on the other hand it would make the following two constraints previously enforced by the compiler only enforced by code comments.
I would prefer to stick with the |
With paritytech/substrate#4767 a `GossipEngine` does not spawn its own tasks, but relies on its owner to poll it. This patch removes the unneeded executor argument and spawns the gossip engine as an orphaned task onto the executor instead.
GossipEnginespawns two tasks, one for a periodic tick, one to forwardmessages from the network to subscribers. These tasks hold an
Arcto aGossipEngineInner.To reduce the amount of shared ownership (locking) this patch integrates
the two tasks into a
Futureimplementation on theGossipEnginestruct. This
Futureimplementation can now be called from a singleowner, e.g. the
finality-grandpaNetworkBridge.As a side effect this removes the requirement on the
network-gossipcrate to spawn tasks and thereby removes the requirement on the
finality-grandpacrate to spawn any tasks.This is part of a greater effort to reduce the number of owners of
components within
finality-grandpa,networkandnetwork-gossipaswell as to reduce the amount of unbounded channels. For details see
d4fbb89, f0c1852 and 5afc777.
Once this simplification lands we can get rid of the construct of a
GossipEngineholding anArc<Mutex<GossipEngineInner>>,given that
GossipEngineonly has a single owner.