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

Locking fixes for versions of Dashmap v5.1.0 and beyond #23765

Merged
merged 1 commit into from
Mar 21, 2022

Conversation

carllin
Copy link
Contributor

@carllin carllin commented Mar 18, 2022

Problem

Bumping dashmap version to v5.1.0 caused validator with secondary indexes to stall, and thus had to be reverted: #23592

Dashmap v5.1.0 switched over to using parking_lot RwLocks. Because these locks can be write-prioritized, this means that behavior like:

Thread 1:  Grabs and holds Read(A) lock, tries to grab Read(A) later while holding original Read(A)
Thread 2: Grabs Write(A)

can deadlock if Thread 2 grabs the write lock between the first and second attempts to grab Read(A) by Thread 1.

Summary of Changes

The attempt to grab read lock here:

let outer_keys = self.reverse_index.get(inner_key).unwrap_or_else(|| {
self.reverse_index
.entry(*inner_key)
.or_insert(RwLock::new(Vec::with_capacity(1)))
.downgrade()
});
, then later here
("num_secondary_keys", self.index.len() as i64, i64),
is an example of such a scenario where the deadlock described above can occur. We fix this by ensuring the first lock drops out of scope.

TODO: Check use cases across the rest of the code base

Fixes #

@carllin carllin requested a review from t-nelson March 18, 2022 05:53
@carllin carllin changed the title Locking fixes for Dashmap v5.1.0 and beyond Locking fixes for Dashmap v5.1.0+ Mar 18, 2022
@carllin carllin changed the title Locking fixes for Dashmap v5.1.0+ Locking fixes for versions of Dashmap >= v5.1.0 Mar 18, 2022
@carllin carllin changed the title Locking fixes for versions of Dashmap >= v5.1.0 Locking fixes for versions of Dashmap v5.1.0 and beyond Mar 18, 2022
Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

change here lgtm. are you planning to examine/address other DashMap usage in this PR or separately?

@carllin
Copy link
Contributor Author

carllin commented Mar 18, 2022

are you planning to examine/address other DashMap usage in this PR

Think I'll do it in this PR

@carllin carllin merged commit f34434f into solana-labs:master Mar 21, 2022
jstarry pushed a commit to jstarry/solana that referenced this pull request May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants