rust-lang / rust-clippy

A bunch of lints to catch common mistakes and improve your Rust code. Book: https://doc.rust-lang.org/clippy/
https://rust-lang.github.io/rust-clippy/
Other
11.3k stars 1.52k forks source link

`await_holding_lock` lint is erroneously triggered when the guard is explicitly dropped before the await. #9208

Open peterjoel opened 2 years ago

peterjoel commented 2 years ago

Summary

The code below triggers await_holding_lock. However, the guards are dropped explicitly before each .await so there should not be a problem.

Lint Name

await_holding_lock

Reproducer

I tried this code:

use std::{collections::*, sync::{Arc, RwLock}};
use thiserror::Error;
use tokio::sync::broadcast;

#[derive(Error, Debug, Clone, Default)]
#[error("There was an error")]
pub struct Error;

#[derive(Debug, Default)]
struct CacheEntry {
    prev_result: Option<Result<i32, Error>>,
    sender: Option<broadcast::Sender<Result<i32, Error>>>,
}

#[derive(Debug, Clone)]
pub struct Cache {
    cache: Arc<RwLock<HashMap<i32, CacheEntry>>>,
}

impl Cache {
    pub async fn retrieve(&self, key: i32) -> Result<i32, Error> {
        let guard = self.cache.read().unwrap();
        let val = guard.get(&key);
        match val {
            Some(entry) => match entry.prev_result.clone() {
                Some(result) => result,
                None => {
                    let mut receiver = entry.sender.as_ref().unwrap().subscribe();
                    drop(guard);
                    receiver.recv().await.unwrap()
                }
            },
            None => {
                drop(guard);
                let mut write_guard = self.cache.write().unwrap();
                let entry = write_guard.entry(key).or_default();
                let mut receiver = entry.sender.as_ref().unwrap().subscribe();
                drop(write_guard);
                receiver.recv().await.unwrap()
            }
        }
    }
}

I saw this happen:

warning: this `MutexGuard` is held across an `await` point
  --> src/lib.rs:22:13
   |
22 |         let guard = self.cache.read().unwrap();
   |             ^^^^^
   |
   = note: `#[warn(clippy::await_holding_lock)]` on by default
   = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await
note: these are all the `await` points this lock is held through
  --> src/lib.rs:22:9
   |
22 | /         let guard = self.cache.read().unwrap();
23 | |         let val = guard.get(&key);
24 | |         match val {
25 | |             Some(entry) => match entry.prev_result.clone() {
...  |
41 | |         }
42 | |     }
   | |_____^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#await_holding_lock

warning: this `MutexGuard` is held across an `await` point
  --> src/lib.rs:35:21
   |
35 |                 let mut write_guard = self.cache.write().unwrap();
   |                     ^^^^^^^^^^^^^^^
   |
   = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await
note: these are all the `await` points this lock is held through
  --> src/lib.rs:35:17
   |
35 | /                 let mut write_guard = self.cache.write().unwrap();
36 | |                 let entry = write_guard.entry(key).or_default();
37 | |                 let mut receiver = entry.sender.as_ref().unwrap().subscribe();
38 | |                 drop(write_guard);
39 | |                 receiver.recv().await.unwrap()
40 | |             }
   | |_____________^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#await_holding_lock

I expected to see this happen:

We expected no errors because the guards are explicitly dropped before each await point.

Version

rustc 1.62.0 (a8314ef7d 2022-06-27)
binary: rustc
commit-hash: a8314ef7d0ec7b75c336af2c9857bfaf43002bfc
commit-date: 2022-06-27
host: x86_64-unknown-linux-gnu
release: 1.62.0
LLVM version: 14.0.5

Additional Labels

No response

jplatte commented 1 year ago

This is almost certainly caused by https://github.com/rust-lang/rust/issues/57478, so this depends on https://github.com/rust-lang/rust/issues/69663.

Also note that this lint was already generalized and put into rustc as must_not_suspend, however that is off by default because of the same bug.

xxshady commented 3 weeks ago

another duplicate of https://github.com/rust-lang/rust-clippy/issues/6446?