Skip to content

Implement lockorder checking on RwLocks in debug_sync #1352

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 22, 2022
Merged

Implement lockorder checking on RwLocks in debug_sync #1352

merged 3 commits into from
Mar 22, 2022

Conversation

TheBlueMatt
Copy link
Collaborator

No description provided.

@TheBlueMatt TheBlueMatt mentioned this pull request Mar 9, 2022
Merged
@codecov-commenter
Copy link

codecov-commenter commented Mar 9, 2022 •
edited
Loading

Codecov Report

Merging #1352 (e0a57b2) into main (0e0aabe) will increase coverage by 0.62%.
The diff coverage is 93.44%.

❗ Current head e0a57b2 differs from pull request most recent head 35c8718. Consider uploading reports for the commit 35c8718 to get more accurate results

@@            Coverage Diff             @@
##             main    #1352      +/-   ##
==========================================
+ Coverage   90.57%   91.20%   +0.62%     
==========================================
  Files          72       72              
  Lines       40082    43782    +3700     
==========================================
+ Hits        36306    39933    +3627     
- Misses       3776     3849      +73
Impacted Files Coverage Δ
lightning/src/debug_sync.rs 94.79% <93.44%> (-0.27%) ⬇️
lightning/src/ln/onion_route_tests.rs 97.59% <0.00%> (-0.04%) ⬇️
lightning-background-processor/src/lib.rs 93.16% <0.00%> (+0.11%) ⬆️
lightning/src/ln/channelmanager.rs 84.88% <0.00%> (+0.27%) ⬆️
lightning/src/ln/functional_tests.rs 97.45% <0.00%> (+0.31%) ⬆️
lightning/src/routing/router.rs 92.79% <0.00%> (+0.68%) ⬆️
lightning/src/ln/payment_tests.rs 100.00% <0.00%> (+0.84%) ⬆️
lightning/src/routing/scoring.rs 95.26% <0.00%> (+1.09%) ⬆️
lightning/src/ln/shutdown_tests.rs 98.02% <0.00%> (+1.52%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e0aabe...35c8718. Read the comment docs.

jkczyz
jkczyz reviewed Mar 9, 2022
View reviewed changes
jkczyz
jkczyz reviewed Mar 9, 2022
View reviewed changes
lightning/src/debug_sync.rs Outdated
@@ -205,6 +213,7 @@ impl<T: Sized> Deref for RwLockReadGuard<'_, T> {

impl<T: Sized> Drop for RwLockReadGuard<'_, T> {
fn drop(&mut self) {
if !self.first_lock { return; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't the first read lock be released before later ones?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In theory, yea, in practice it should be exceedingly rare cause ~all our locks are RAII. I'll just add a comment noting that its a limitation.

@TheBlueMatt TheBlueMatt force-pushed the 2022-03-debug-rwlock branch from 943d228 to e0a57b2 Compare March 9, 2022 20:10
arik-so
arik-so previously approved these changes Mar 18, 2022
View reviewed changes
Copy link
Contributor

@arik-so arik-so left a comment

Choose a reason for hiding this comment

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

seems fine as far as I can tell and test

jkczyz
jkczyz previously approved these changes Mar 18, 2022
View reviewed changes
Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

Good to squash

@TheBlueMatt TheBlueMatt dismissed stale reviews from jkczyz and arik-so via 35c8718 March 18, 2022 18:54
@TheBlueMatt TheBlueMatt force-pushed the 2022-03-debug-rwlock branch from e0a57b2 to 35c8718 Compare March 18, 2022 18:54
@TheBlueMatt
Copy link
Collaborator Author

Squashed without change.

arik-so
arik-so approved these changes Mar 22, 2022
View reviewed changes
Copy link
Contributor

@arik-so arik-so left a comment

Choose a reason for hiding this comment

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

Re-ACKing

@TheBlueMatt TheBlueMatt merged commit 996d3d8 into lightningdevkit:main Mar 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arik-so arik-so arik-so approved these changes

@jkczyz jkczyz jkczyz approved these changes

Assignees

@arik-so arik-so

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@TheBlueMatt @codecov-commenter @arik-so @jkczyz