Skip to content
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

rpc: fix possible deadlock in rpc #26051

Merged
merged 1 commit into from
Jun 23, 2022

Conversation

BurtonQin
Copy link
Contributor

Problem

There is a possible deadlock caused by double readlock in fn get_transaction_status.

block_commitment_cache is an Arc<RwLock<...>>.

The first readlock is on L1434

solana/rpc/src/rpc.rs

Lines 1434 to 1436 in fbf7143

let r_block_commitment_cache = self.block_commitment_cache.read().unwrap();
let optimistically_confirmed_bank = self.bank(Some(CommitmentConfig::confirmed()));

The second readlock is on L253 in fn bank

solana/rpc/src/rpc.rs

Lines 250 to 254 in fbf7143

let slot = self
.block_commitment_cache
.read()
.unwrap()
.slot_with_commitment(commitment.commitment);

For more details on this kind of deadlock, see
https://www.reddit.com/r/rust/comments/urnqz8/different_behaviors_of_recursive_read_locks_in/

Summary of Changes

The fix is to move the first readlock after the calling of fn bank.
But I wonder if this is correct or optimal.

  1. I do not know if optimistically_confirmed should be protected by block_commitment_cache? OR
  2. can we drop r_block_commitment_cache immediately after the creation of confirmations on L1440-1448.

Fixes #

@mergify mergify bot added the community Community contribution label Jun 18, 2022
@mergify mergify bot requested a review from a team June 18, 2022 04:39
@codecov
Copy link

codecov bot commented Jun 18, 2022

Codecov Report

Merging #26051 (97cc753) into master (8caced6) will decrease coverage by 0.2%.
The diff coverage is 75.0%.

@@            Coverage Diff            @@
##           master   #26051     +/-   ##
=========================================
- Coverage    82.1%    81.8%   -0.3%     
=========================================
  Files         628      631      +3     
  Lines      171471   174123   +2652     
=========================================
+ Hits       140878   142595   +1717     
- Misses      30593    31528    +935     

@CriesofCarrots
Copy link
Contributor

@BurtonQin , to be clear, there is only a risk of deadlock with write-prioritized RwLocks, correct?

@BurtonQin
Copy link
Contributor Author

@BurtonQin , to be clear, there is only a risk of deadlock with write-prioritized RwLocks, correct?

Yes.

Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution

@CriesofCarrots CriesofCarrots merged commit 113b161 into solana-labs:master Jun 23, 2022
gregcusack pushed a commit to gregcusack/solana that referenced this pull request Jun 23, 2022
gregcusack added a commit that referenced this pull request Jun 29, 2022
* add three gossip metrics measuring gossip loop times

* add 5 metrics

* rm space

* rm space

* Update SECURITY.md

- fix nav link
- add bounty split policy for duplicate reports

* Add transaction index in slot to geyser plugin TransactionInfo (#25688)

* Define shuffle to prep using same shuffle for multiple slices

* Determine transaction indexes and plumb to execute_batch

* Pair transaction_index with transaction in TransactionStatusService

* Add new ReplicaTransactionInfoVersion

* Plumb transaction_indexes through BankingStage

* Prepare BankingStage to receive transaction indexes from PohRecorder

* Determine transaction indexes in PohRecorder; add field to WorkingBank

* Add PohRecorder::record unit test

* Only pass starting_transaction_index around PohRecorder

* Add helper structs to simplify test DashMap

* Pass entry and starting-index into process_entries_with_callback together

* Add tx-index checks to test_rebatch_transactions

* Revert shuffle definition and use zip/unzip

* Only zip/unzip if randomize

* Add confirm_slot_entries test

* Review nits

* Add type alias to make sender docs more clear

* Update SECURITY.md

finish filling out the table....

* rpc: fix possible deadlock in rpc (#26051)

* Add StatusCache::root_slot_deltas() and use it (#26170)

* Remove InMemAccountsIndex::map() and use map_internal directly (#26189)

* [quic]Decrement total_streams correctly (#26158)

* remove comment

* alphabetical metrics. no abbreviations

* remove trailing white space

* cargo fmt to update code format/readability

Co-authored-by: Trent Nelson <trent@solana.com>
Co-authored-by: Tyera Eulberg <tyera@solana.com>
Co-authored-by: Boqin Qin(秦 伯钦) <Bobbqqin@gmail.com>
Co-authored-by: Brooks Prumo <brooks@solana.com>
Co-authored-by: Miles Obare <bdhobare@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants