-
Notifications
You must be signed in to change notification settings - Fork 153
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Wrap ChainSyncer::state in an Arc<Mutex<_>> and set it to Follow accordingly #914
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,7 @@ use super::sync_state::SyncState; | |
use super::sync_worker::SyncWorker; | ||
use super::{Error, SyncNetworkContext}; | ||
use amt::Amt; | ||
use async_std::sync::{channel, Receiver, RwLock, Sender}; | ||
use async_std::sync::{channel, Mutex, Receiver, RwLock, Sender}; | ||
use async_std::task::{self, JoinHandle}; | ||
use beacon::Beacon; | ||
use blocks::{Block, FullTipset, GossipBlock, Tipset, TipsetKeys, TxMeta}; | ||
|
@@ -38,7 +38,7 @@ use std::sync::Arc; | |
type WorkerState = Arc<RwLock<Vec<Arc<RwLock<SyncState>>>>>; | ||
|
||
#[derive(Debug, PartialEq)] | ||
enum ChainSyncState { | ||
pub enum ChainSyncState { | ||
/// Bootstrapping peers before starting sync. | ||
Bootstrap, | ||
/// Syncing chain with ChainExchange protocol. | ||
|
@@ -78,7 +78,7 @@ impl Default for SyncConfig { | |
/// messages to be able to do the initial sync. | ||
pub struct ChainSyncer<DB, TBeacon, V, M> { | ||
/// State of general `ChainSync` protocol. | ||
state: ChainSyncState, | ||
state: Arc<Mutex<ChainSyncState>>, | ||
|
||
/// Syncing state of chain sync workers. | ||
worker_state: WorkerState, | ||
|
@@ -142,7 +142,7 @@ where | |
); | ||
|
||
Ok(Self { | ||
state: ChainSyncState::Bootstrap, | ||
state: Arc::new(Mutex::new(ChainSyncState::Bootstrap)), | ||
worker_state: Default::default(), | ||
beacon, | ||
network, | ||
|
@@ -221,7 +221,7 @@ where | |
.await | ||
} | ||
NetworkEvent::PubsubMessage { source, message } => { | ||
if self.state != ChainSyncState::Follow { | ||
if *self.state.lock().await != ChainSyncState::Follow { | ||
// Ignore gossipsub events if not in following state | ||
return; | ||
} | ||
|
@@ -375,7 +375,7 @@ where | |
verifier: PhantomData::<V>::default(), | ||
req_window: self.sync_config.req_window, | ||
} | ||
.spawn(channel) | ||
.spawn(channel, Arc::clone(&self.state)) | ||
.await | ||
} | ||
|
||
|
@@ -424,10 +424,10 @@ where | |
.await; | ||
|
||
// Only update target on initial sync | ||
if self.state == ChainSyncState::Bootstrap { | ||
if *self.state.lock().await == ChainSyncState::Bootstrap { | ||
if let Some(best_target) = self.select_sync_target().await { | ||
self.schedule_tipset(best_target).await; | ||
self.state = ChainSyncState::Initial; | ||
*self.state.lock().await = ChainSyncState::Initial; | ||
Comment on lines
+427
to
+430
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this keeps the lock and will deadlock here, has this been tested? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not tested, but I'm quite certain that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've had issues with async-std mutexes holding the lock in this exact case in the past, not sure if it's changed or only applies to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nvm, there was a specific case where it deadlocked similar to this, and moving the read outside of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh yeah, I can totally see that happening. Thanks for pointing it out either way, I could have easily messed this up |
||
return; | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is mutex over rwlock required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not, but does
RwLock
have any benefits if we're not expecting concurrent reads?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I just was asking because there doesn't seem to be a reason to have the additional constraint (just thinking for future proofing)
It's definitely fine if this stays as is, this will probably be replaced in the future anyway