From a61652104bbee1e2877c4af166441d067e26a4ba Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Fri, 29 Apr 2022 16:32:46 +0800 Subject: [PATCH] Avoid holding lock guards in match expressions (#24805) * Avoid holding bank forks read lock for RPC requests * Avoid using lock guards in temporaries * revert fetch stage change --- core/src/banking_stage.rs | 6 ++--- core/src/cluster_info_vote_listener.rs | 3 ++- core/src/warm_quic_cache_service.rs | 6 ++--- .../optimistically_confirmed_bank_tracker.rs | 3 ++- rpc/src/rpc.rs | 3 +-- rpc/src/rpc_service.rs | 6 ++--- rpc/src/rpc_subscriptions.rs | 23 ++++++++----------- 7 files changed, 22 insertions(+), 28 deletions(-) diff --git a/core/src/banking_stage.rs b/core/src/banking_stage.rs index 81de5584ae..8656f1ece4 100644 --- a/core/src/banking_stage.rs +++ b/core/src/banking_stage.rs @@ -2094,11 +2094,11 @@ fn next_leader_x( where F: FnOnce(&ContactInfo) -> SocketAddr, { - if let Some(leader_pubkey) = poh_recorder + let leader_pubkey = poh_recorder .lock() .unwrap() - .leader_after_n_slots(FORWARD_TRANSACTIONS_TO_LEADER_AT_SLOT_OFFSET) - { + .leader_after_n_slots(FORWARD_TRANSACTIONS_TO_LEADER_AT_SLOT_OFFSET); + if let Some(leader_pubkey) = leader_pubkey { cluster_info.lookup_contact_info(&leader_pubkey, port_selector) } else { None diff --git a/core/src/cluster_info_vote_listener.rs b/core/src/cluster_info_vote_listener.rs index a61433407d..dc3d2aa87a 100644 --- a/core/src/cluster_info_vote_listener.rs +++ b/core/src/cluster_info_vote_listener.rs @@ -366,7 +366,8 @@ impl ClusterInfoVoteListener { // Always set this to avoid taking the poh lock too often time_since_lock = Instant::now(); // We will take this lock at most once every `BANK_SEND_VOTES_LOOP_SLEEP_MS` - if let Some(current_working_bank) = poh_recorder.lock().unwrap().bank() { + let current_working_bank = poh_recorder.lock().unwrap().bank(); + if let Some(current_working_bank) = current_working_bank { Self::check_for_leader_bank_and_send_votes( &mut bank_vote_sender_state_option, current_working_bank, diff --git a/core/src/warm_quic_cache_service.rs b/core/src/warm_quic_cache_service.rs index d7013710a6..04014eddf4 100644 --- a/core/src/warm_quic_cache_service.rs +++ b/core/src/warm_quic_cache_service.rs @@ -36,11 +36,11 @@ impl WarmQuicCacheService { let slot_jitter = thread_rng().gen_range(-CACHE_JITTER_SLOT, CACHE_JITTER_SLOT); let mut maybe_last_leader = None; while !exit.load(Ordering::Relaxed) { - if let Some(leader_pubkey) = poh_recorder + let leader_pubkey = poh_recorder .lock() .unwrap() - .leader_after_n_slots((CACHE_OFFSET_SLOT + slot_jitter) as u64) - { + .leader_after_n_slots((CACHE_OFFSET_SLOT + slot_jitter) as u64); + if let Some(leader_pubkey) = leader_pubkey { if maybe_last_leader .map_or(true, |last_leader| last_leader != leader_pubkey) { diff --git a/rpc/src/optimistically_confirmed_bank_tracker.rs b/rpc/src/optimistically_confirmed_bank_tracker.rs index 2b2acec6e5..7fdb5b905b 100644 --- a/rpc/src/optimistically_confirmed_bank_tracker.rs +++ b/rpc/src/optimistically_confirmed_bank_tracker.rs @@ -204,7 +204,8 @@ impl OptimisticallyConfirmedBankTracker { debug!("received bank notification: {:?}", notification); match notification { BankNotification::OptimisticallyConfirmed(slot) => { - if let Some(bank) = bank_forks.read().unwrap().get(slot) { + let bank = bank_forks.read().unwrap().get(slot); + if let Some(bank) = bank { let mut w_optimistically_confirmed_bank = optimistically_confirmed_bank.write().unwrap(); diff --git a/rpc/src/rpc.rs b/rpc/src/rpc.rs index 9bc0089e55..011d2c8d38 100644 --- a/rpc/src/rpc.rs +++ b/rpc/src/rpc.rs @@ -209,10 +209,8 @@ impl JsonRpcRequestProcessor { #[allow(deprecated)] fn bank(&self, commitment: Option) -> Arc { debug!("RPC commitment_config: {:?}", commitment); - let r_bank_forks = self.bank_forks.read().unwrap(); let commitment = commitment.unwrap_or_default(); - if commitment.is_confirmed() { let bank = self .optimistically_confirmed_bank @@ -250,6 +248,7 @@ impl JsonRpcRequestProcessor { CommitmentLevel::SingleGossip | CommitmentLevel::Confirmed => unreachable!(), // SingleGossip variant is deprecated }; + let r_bank_forks = self.bank_forks.read().unwrap(); r_bank_forks.get(slot).unwrap_or_else(|| { // We log a warning instead of returning an error, because all known error cases // are due to known bugs that should be fixed instead. diff --git a/rpc/src/rpc_service.rs b/rpc/src/rpc_service.rs index 740d3bcd32..dd1659593b 100644 --- a/rpc/src/rpc_service.rs +++ b/rpc/src/rpc_service.rs @@ -296,8 +296,7 @@ impl RequestMiddleware for RpcRequestMiddleware { fn process_rest(bank_forks: &Arc>, path: &str) -> Option { match path { "/v0/circulating-supply" => { - let r_bank_forks = bank_forks.read().unwrap(); - let bank = r_bank_forks.root_bank(); + let bank = bank_forks.read().unwrap().root_bank(); let total_supply = bank.capitalization(); let non_circulating_supply = solana_runtime::non_circulating_supply::calculate_non_circulating_supply(&bank) @@ -309,8 +308,7 @@ fn process_rest(bank_forks: &Arc>, path: &str) -> Option { - let r_bank_forks = bank_forks.read().unwrap(); - let bank = r_bank_forks.root_bank(); + let bank = bank_forks.read().unwrap().root_bank(); let total_supply = bank.capitalization(); Some(format!("{}", lamports_to_sol(total_supply))) } diff --git a/rpc/src/rpc_subscriptions.rs b/rpc/src/rpc_subscriptions.rs index 2e8325eb08..6b2c401ff2 100644 --- a/rpc/src/rpc_subscriptions.rs +++ b/rpc/src/rpc_subscriptions.rs @@ -145,7 +145,8 @@ where X: Clone + Default, { let mut notified = false; - if let Some(bank) = bank_forks.read().unwrap().get(slot) { + let bank = bank_forks.read().unwrap().get(slot); + if let Some(bank) = bank { let results = bank_method(&bank, params); let mut w_last_notified_slot = subscription.last_notified_slot.write().unwrap(); let (filter_results, result_slot) = @@ -442,7 +443,7 @@ fn initial_last_notified_slot( bank_forks: &RwLock, block_commitment_cache: &RwLock, optimistically_confirmed_bank: &RwLock, -) -> Slot { +) -> Option { match params { SubscriptionParams::Account(params) => { let slot = if params.commitment.is_finalized() { @@ -456,18 +457,10 @@ fn initial_last_notified_slot( block_commitment_cache.read().unwrap().slot() }; - if let Some((_account, slot)) = bank_forks - .read() - .unwrap() - .get(slot) - .and_then(|bank| bank.get_account_modified_slot(¶ms.pubkey)) - { - slot - } else { - 0 - } + let bank = bank_forks.read().unwrap().get(slot)?; + Some(bank.get_account_modified_slot(¶ms.pubkey)?.1) } - _ => 0, + _ => None, } } @@ -754,6 +747,7 @@ impl RpcSubscriptions { &block_commitment_cache, &optimistically_confirmed_bank, ) + .unwrap_or(0) }); } NotificationEntry::Unsubscribed(params, id) => { @@ -942,7 +936,8 @@ impl RpcSubscriptions { SubscriptionParams::Block(params) => { num_blocks_found.fetch_add(1, Ordering::Relaxed); if let Some(slot) = slot { - if let Some(bank) = bank_forks.read().unwrap().get(slot) { + let bank = bank_forks.read().unwrap().get(slot); + if let Some(bank) = bank { // We're calling it unnotified in this context // because, logically, it gets set to `last_notified_slot + 1` // on the final iteration of the loop down below.