client/network-gossip: Merge GossipEngine and GossipEngineInner #5042
Conversation
Given that GossipEngine and GossipEngineInner are not shared between threads anyone (public interface + background tasks), neither depends on being Send or Sync. Thus one can merge the two as done in this patch. One only needs to wrap an `Arc<Mutex<>>` around the whole structure when the owner (e.g. finality-grandpa) needs to share the gossip engine between threads.
GossipEngine in itself has no need to be Send and Sync, given that it does not rely on separately spawned background tasks anymore. Given that finality-grandpa shares the `NetworkBridge` potentially between threads its components need to be clonable, thus this patch wraps `GossipEngine` in an `Arc<Mutex<>>`.
| @@ -14,7 +14,6 @@ futures-timer = "3.0.1" | |||
| libp2p = { version = "0.16.1", default-features = false, features = ["libp2p-websocket"] } | |||
| log = "0.4.8" | |||
| lru = "0.1.2" | |||
| parking_lot = "0.10.0" | |||
There was a problem hiding this comment.
This line probably summarizes the pull request the best.
|
Considering that each gossip protocol should have its own exclusive instance of |
|
I didn't mark the PR as approved because of:
But consider it approval. |
|
This is technically an API breaking change, no? So we can't do it during the stabilization period. |
@tomaka yes, I am guessing so, given that @gnunicorn what is the right procedure here? Should I be adding the pull request to the 3.0 milestone? |
|
if it can be raised during alpha-phase, we can consider still merging this before 2.0 – I consider not-too-involved API-clean-up PRs as "fixes". but it should have the corresponding polkadot PR pending. API changes will only be restricted once we reach beta-phase. |
|
Corresponding Polkadot pull request: paritytech/polkadot#890 @gnunicorn as far as I can tell we have not yet reached the beta phase. Could you reconsider merging this? |
|
sure, just follow the process lined out to get PRs merged during the freeze. |
gnunicorn
added
A8-looksgood
and removed
A0-please_review
|
I thought we had not stabilized the consensus related parts, isn't it why this crate is at verison |
@gnunicorn thanks a bunch for the help and writing all of this down. I also added |
…tytech#5042) * client/network-gossip: Merge GossipEngine and GossipEngineInner Given that GossipEngine and GossipEngineInner are not shared between threads anyone (public interface + background tasks), neither depends on being Send or Sync. Thus one can merge the two as done in this patch. One only needs to wrap an `Arc<Mutex<>>` around the whole structure when the owner (e.g. finality-grandpa) needs to share the gossip engine between threads. * client/finality-grandpa: Wrap GossipEngine in Arc Mutex & lock it on use GossipEngine in itself has no need to be Send and Sync, given that it does not rely on separately spawned background tasks anymore. Given that finality-grandpa shares the `NetworkBridge` potentially between threads its components need to be clonable, thus this patch wraps `GossipEngine` in an `Arc<Mutex<>>`.
…tytech#5042) * client/network-gossip: Merge GossipEngine and GossipEngineInner Given that GossipEngine and GossipEngineInner are not shared between threads anyone (public interface + background tasks), neither depends on being Send or Sync. Thus one can merge the two as done in this patch. One only needs to wrap an `Arc<Mutex<>>` around the whole structure when the owner (e.g. finality-grandpa) needs to share the gossip engine between threads. * client/finality-grandpa: Wrap GossipEngine in Arc Mutex & lock it on use GossipEngine in itself has no need to be Send and Sync, given that it does not rely on separately spawned background tasks anymore. Given that finality-grandpa shares the `NetworkBridge` potentially between threads its components need to be clonable, thus this patch wraps `GossipEngine` in an `Arc<Mutex<>>`.
This pull request contains two commits, the latter supporting the former.
client/network-gossip: Merge GossipEngine and GossipEngineInner
Given that GossipEngine and GossipEngineInner are not shared between threads
anyone (see client/network-gossip: Integrate GossipEngine tasks into Future impl #4767), neither depend
on being Send or Sync. Thus one can merge the two as done in this patch. One
only needs to wrap an
Arc<Mutex<>>around the whole structure when the owner(e.g. finality-grandpa) needs to share the gossip engine between threads.
client/finality-grandpa: Wrap GossipEngine in Arc Mutex & lock it on use
GossipEngine in itself has no need to be Send and Sync, given that it
does not rely on separately spawned background tasks anymore. Given that
finality-grandpa shares the
NetworkBridgepotentially between threadsits components need to be clonable, thus this patch wraps
GossipEnginein an
Arc<Mutex<>>.This would require changes within Polkadot which I have not yet prepared, therefore please don't merge this pull request just yet.