-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Remove lock around JsonRpcRequestProcessor #10417
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 |
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.
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. |
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 |
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.
✨ Nice to see all those read-unwrap
s 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. |
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>
Problem
JsonRpcRequestProcessor
has 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
Meta
fields intoJsonRpcRequestProcessor
, and makeJsonRpcRequestProcessor
implementMetadata
instead.