Remove lock around JsonRpcRequestProcessor#10417
Remove lock around JsonRpcRequestProcessor#10417solana-grimes merged 3 commits intosolana-labs:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10417 +/- ##
=========================================
- Coverage 81.6% 81.5% -0.1%
=========================================
Files 296 296
Lines 69360 69340 -20
=========================================
- Hits 56633 56569 -64
- Misses 12727 12771 +44 |
CriesofCarrots
left a comment
CriesofCarrots
left a comment
There was a problem hiding this comment.
Gave this a test drive, worked fine.
|
I'm on board for a v1.2 and v1.1 backport here, otherwise I think we're looking at RPC rebase hell for a couple months. But your call on the risk |
|
Okay, I'll put the rest of the changes in this PR as well, for easy backporting and, if needed, reverting. @CriesofCarrots, even if it seems to work fine, the concern here would be deadlocks caused by grabbing 2 of the inner locks in opposite order from different RPC threads. |
mergify
Bot
dismissed
CriesofCarrots’s stale review
Pull request has been modified.
|
@CriesofCarrots, care to take another look at this? I think this is enough for this first PR. I think the next step is to inline all the |
CriesofCarrots
left a comment
CriesofCarrots
left a comment
There was a problem hiding this comment.
✨ Nice to see all those read-unwraps disappear. This does seem like a nice discrete chunk. I'll do some additional testing with various rpc loads and use-cases (like break) to keep an eye on that lock.
|
@CriesofCarrots, did you want to do those additional tests pre-merge or post-merge? |
Post-merge was my plan. Sorry to be ambiguous. |
mergify
Bot
dismissed
CriesofCarrots’s stale review
Pull request has been modified.
automerge (cherry picked from commit af8c21c) # Conflicts: # core/src/rpc.rs
automerge (cherry picked from commit af8c21c)
…10445) * address warnings from 'rustup run beta cargo clippy --workspace' minor refactoring in: - cli/src/cli.rs - cli/src/offline/blockhash_query.rs - logger/src/lib.rs - runtime/src/accounts_db.rs expect some performance improvement AccountsDB::clean_accounts() * address warnings from 'rustup run beta cargo clippy --workspace --tests' * address warnings from 'rustup run nightly cargo clippy --workspace --all-targets' * rustfmt * fix warning stragglers * properly fix clippy warnings test_vote_subscribe() replace ref-to-arc with ref parameters where arc not cloned * Remove lock around JsonRpcRequestProcessor (#10417) automerge * make ancestors parameter optional to avoid forcing construction of empty hash maps Co-authored-by: Greg Fitzgerald <greg@solana.com>
4 participants
Problem
JsonRpcRequestProcessorhas fields with locks and is wrapped with a lock. Not clear why the lock is needed at both levels.Summary of Changes
Attempt to remove the outer lock. If we're able to remove this lock, then we can also move the remaining
Metafields intoJsonRpcRequestProcessor, and makeJsonRpcRequestProcessorimplementMetadatainstead.