Ensure all locks (.lock(), .read(), .write(), etc.) aren't held over if let statements

[buffalu]

Problem

See this for context:
rust-lang/rust#93883
https://www.reddit.com/r/rust/comments/ldic3m/confusing_point_regarding_scope_and_dropped_value/

I was recently running into some type of deadlock issue in block_subscribe which i narrowed down to unexpectedly holding locks across if let. Something like:

if let Some(bank) = bank_forks.read().unwrap().get(slot) {}

Proposed Solution

• Review entire codebase for holding locks for too long across these if let statements. @jstarry and @brooksprumo has started to fix this in Do not hold lock unnecessarily when hashing #24815 and Avoid holding lock guards in match expressions #24805.

[jstarry]

Once this clippy lint makes it into nightly, I think we should start applying it in CI: rust-lang/rust#93883

[jstarry]

The latest nightly rust (rust version 1.62.0-nightly (cb1219871 2022-05-08)) now has the lint I linked above and it can be run with cargo +nightly clippy -- -W clippy::significant_drop_in_scrutinee

[brooksprumo]

Clippy found these two:

warning: temporary with significant drop in match scrutinee
    --> runtime/src/bank.rs:2935:36
     |
2935 |             let vote_state = match vote_account.vote_state().deref() {
     |                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = note: requested on the command line with `-W clippy::significant-drop-in-scrutinee`
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#significant_drop_in_scrutinee

(PR #25094 to fix the above warning.)

warning: temporary with significant drop in match scrutinee
   --> gossip/src/cluster_info.rs:863:76
    |
863 |                     let epoch_slots = gossip_crds.get::<&CrdsValue>(&label)?.epoch_slots()?;
    |                                                                            ^
    |
    = note: requested on the command line with `-W clippy::significant-drop-in-scrutinee`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#significant_drop_in_scrutinee

[jstarry]

Hmm can you try running before the recent patches on master? I would have expected a few more

[brooksprumo]

Hmm can you try running before the recent patches on master? I would have expected a few more

I ran it again from commit 9a136aa684—which is the one before a61652104b "Avoid holding lock guards in match expressions (#24805)"—and got the same results (even after an explicit cargo +nightly clean too)... So that's strange...

Was there a different commit you wanted me to run against? It may be worthwhile to double check my results by running it also, so there's another datapoint. Maybe this new lint doesn't pick up everything yet?

[jstarry]

Hmm I'm thinking it's not working quite right yet then. Would be interesting to dig into why. (cc @PrestonFrom if you want to join in)

[PrestonFrom]

@jstarry Thanks for letting me know. I think I found the issue -- working on a fix but not sure on the timeline.

[jstarry]

@jstarry Thanks for letting me know. I think I found the issue -- working on a fix but not sure on the timeline.

Happy to hear that, thanks for taking a look. No rush from us, we really appreciate your work so far on that lint!