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

False-positive await_holding_lock with manual drop of MutexGuard #9683

Open
Swatinem opened this issue Oct 21, 2022 · 5 comments
Open

False-positive await_holding_lock with manual drop of MutexGuard #9683

Swatinem opened this issue Oct 21, 2022 · 5 comments
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have

Comments

@Swatinem
Copy link
Contributor

Summary

I manually drop my MutexGuard right before the await point, therefore the lock is not held across await points.

Lint Name

await_holding_lock

Reproducer

I tried this code:

async fn test_clippy_mutex_await() {
    let mutex = std::sync::Mutex::new(());
    let lock = mutex.lock().unwrap();
    drop(lock);

    (async {}).await;
}

I saw this happen:

warning: this `MutexGuard` is held across an `await` point
  --> crates\symbolicator-cache\src\lib.rs:91:9
   |
91 |     let lock = mutex.lock().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
  --> crates\symbolicator-cache\src\lib.rs:91:5
   |
91 | /     let lock = mutex.lock().unwrap();
92 | |     drop(lock);
93 | |
94 | |     (async {}).await;
95 | | }
   | |_^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#await_holding_lock
   = note: `#[warn(clippy::await_holding_lock)]` on by default

I expected to see this happen:

This should not error as the MutexGuard is not alive at the await point.

Version

rustc 1.66.0-nightly (dcb376115 2022-10-20)
binary: rustc
commit-hash: dcb376115066d111dbf5f13d5ac2a2dbe8c12add
commit-date: 2022-10-20
host: x86_64-pc-windows-msvc
release: 1.66.0-nightly
LLVM version: 15.0.2

Additional Labels

No response

@Swatinem Swatinem added C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have labels Oct 21, 2022
@kraktus
Copy link
Contributor

kraktus commented Oct 21, 2022

I'm not sure if this use case is common enough to make the lint handle it. I would throw an allow in this particular case.

@Swatinem
Copy link
Contributor Author

I’m doing that, and it is a good workaround for me.

I still think its a valid false-positive, even if the solution right now is "not worth the effort fixing".

@davidv1992
Copy link

Wanted to add that this was affecting me also today, perhaps this is more common than suggested by kraktus. (we ran into it in a resource management loop, which is for us at least a somewhat common pattern)

@zroug
Copy link

zroug commented Mar 10, 2024

I also came across this for a loop. The lock is needed at the start and end of the loop. By dropping the lock manually, the mutex only needs to be locked once per iteration instead of twice.

let mut lock = mutex.lock().unwrap();
loop {
	let future = start_computation(&lock);
	drop(lock);

	let result = future.await;

	lock = mutex.lock().unwrap();
	apply_result(&lock, result);
}

Playground

@xxshady
Copy link

xxshady commented Sep 1, 2024

duplicate of #6446?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have
Projects
None yet
Development

No branches or pull requests

5 participants