Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

Remove lock around JsonRpcRequestProcessor #10417

Merged
merged 3 commits into from
Jun 8, 2020

Conversation

garious
Copy link
Contributor

@garious garious commented Jun 4, 2020

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 into JsonRpcRequestProcessor, and make JsonRpcRequestProcessor implement Metadata instead.

@garious garious requested a review from CriesofCarrots June 4, 2020 23:54
@codecov
Copy link

codecov bot commented Jun 5, 2020

Codecov Report

Merging #10417 into master will decrease coverage by 0.0%.
The diff coverage is 89.0%.

@@            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
CriesofCarrots previously approved these changes Jun 5, 2020
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.

Gave this a test drive, worked fine.

@mvines
Copy link
Contributor

mvines commented Jun 5, 2020

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

@garious
Copy link
Contributor Author

garious commented Jun 5, 2020

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 mergify bot dismissed CriesofCarrots’s stale review June 5, 2020 17:19

Pull request has been modified.

@garious
Copy link
Contributor Author

garious commented Jun 5, 2020

@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 JsonRpcRequestProcessor functions, but that's a bit commitment to JSON RPC. We might want to consider different binary formats, so that we don't need to do crazy things like serialize Transaction as a base58 string just to bypass the bloated JSON default.

CriesofCarrots
CriesofCarrots previously approved these changes Jun 5, 2020
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.

✨ 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.

@garious
Copy link
Contributor Author

garious commented Jun 7, 2020

@CriesofCarrots, did you want to do those additional tests pre-merge or post-merge?

@CriesofCarrots
Copy link
Contributor

did you want to do those additional tests pre-merge or post-merge?

Post-merge was my plan. Sorry to be ambiguous.

@mergify mergify bot dismissed CriesofCarrots’s stale review June 8, 2020 02:06

Pull request has been modified.

@garious garious added the automerge Merge this Pull Request automatically once CI passes label Jun 8, 2020
@solana-grimes solana-grimes merged commit af8c21c into solana-labs:master Jun 8, 2020
mergify bot pushed a commit that referenced this pull request Jun 8, 2020
automerge

(cherry picked from commit af8c21c)

# Conflicts:
#	core/src/rpc.rs
mergify bot pushed a commit that referenced this pull request Jun 8, 2020
automerge

(cherry picked from commit af8c21c)
solana-grimes pushed a commit that referenced this pull request Jun 8, 2020
svenski123 pushed a commit to svenski123/solana that referenced this pull request Jun 8, 2020
svenski123 pushed a commit to svenski123/solana that referenced this pull request Jun 8, 2020
solana-grimes pushed a commit that referenced this pull request Jun 8, 2020
ryoqun pushed a commit that referenced this pull request Jun 9, 2020
…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>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants