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

await_holding_lock reports explicitly dropped mutex guards as if they weren't dropped #6446

Open
jazeved0 opened this issue Dec 12, 2020 · 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

@jazeved0
Copy link

jazeved0 commented Dec 12, 2020

I tried this code:

#[deny(clippy::pedantic)]
pub async fn doesnt_work() {
    use std::collections::HashSet;
    use std::sync::Mutex;
    use std::time::Duration;

    let lock: Mutex<HashSet<u8>> = Mutex::new(HashSet::new());
    let mut set = lock.lock().unwrap();
    set.insert(0);
    drop(set);
    tokio::time::sleep(Duration::from_secs(1)).await;
}

I expected to see this happen: it should pass Clippy with no linting errors because the MutexGuard is explicitly dropped before the await point with a call to std::mem::drop.

Instead, this happened: Clippy failed the lint check because of a violation of clippy::await_holding_lock:

$ cargo clippy
...
error: this MutexGuard is held across an 'await' point. Consider using an async-aware Mutex type or ensuring the MutexGuard is dropped before calling await.
  --> src/lib.rs:8:9
   |
8  |     let mut set = lock.lock().unwrap();
   |         ^^^^^^^
   |
note: the lint level is defined here
  --> src/lib.rs:1:8
   |
1  | #[deny(clippy::pedantic)]
   |        ^^^^^^^^^^^^^^^^
   = note: `#[deny(clippy::await_holding_lock)]` implied by `#[deny(clippy::pedantic)]`
note: these are all the await points this lock is held through
  --> src/lib.rs:8:5
   |
8  | /     let mut set = lock.lock().unwrap();
9  | |     set.insert(0);
10 | |     drop(set);
11 | |     tokio::time::sleep(Duration::from_secs(1)).await;
12 | | }
   | |_^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#await_holding_lock

error: aborting due to previous error

Running with --verbose does not provide any additional information.

Additionally, of note, the following code does pass Clippy:

#[deny(clippy::pedantic)]
pub async fn does_work() {
    use std::collections::HashSet;
    use std::sync::Mutex;
    use std::time::Duration;

    let lock: Mutex<HashSet<u8>> = Mutex::new(HashSet::new());
    {
        let mut set = lock.lock().unwrap();
        set.insert(0);
    }
    tokio::time::sleep(Duration::from_secs(1)).await;
}

Meta

  • cargo clippy -V: clippy 0.0.212 (7eac88a 2020-11-16)
  • rustc -Vv:
    rustc 1.48.0 (7eac88abb 2020-11-16)
    binary: rustc
    commit-hash: 7eac88abb2e57e752f3302f02be5f3ce3d7adfb4
    commit-date: 2020-11-16
    host: x86_64-unknown-linux-gnu
    release: 1.48.0
    LLVM version: 11.0
    
Backtrace

$ RUST_BACKTRACE=1 cargo clippy
    Checking bug_report v0.1.0 (/home/jazev/dev/architus/logs/bug_report)
error: this MutexGuard is held across an 'await' point. Consider using an async-aware Mutex type or 
ensuring the MutexGuard is dropped before calling await.
  --> src/lib.rs:8:9
   |
8  |     let mut set = lock.lock().unwrap();
   |         ^^^^^^^
   |
note: the lint level is defined here
  --> src/lib.rs:1:8
   |
1  | #[deny(clippy::pedantic)]
   |        ^^^^^^^^^^^^^^^^
   = note: `#[deny(clippy::await_holding_lock)]` implied by `#[deny(clippy::pedantic)]`
note: these are all the await points this lock is held through
  --> src/lib.rs:8:5
   |
8  | /     let mut set = lock.lock().unwrap();
9  | |     set.insert(0);
10 | |     drop(set);
11 | |     tokio::time::sleep(Duration::from_secs(1)).await;
12 | | }
   | |_^
   = help: for further information visit https://rust-lang.github.io/rust- 
clippy/master/index.html#await_holding_lock

error: aborting due to previous error

error: could not compile `bug_report`

To learn more, run the command again with --verbose.

@jazeved0 jazeved0 added the C-bug Category: Clippy is not doing the correct thing label Dec 12, 2020
@jazeved0 jazeved0 changed the title clippy::await_holding_lock reports explicitly dropped mutex guards as if they weren't dropped await_holding_lock reports explicitly dropped mutex guards as if they weren't dropped Dec 12, 2020
@ebroto
Copy link
Member

ebroto commented Dec 13, 2020

Thanks for reporting this.

This is a duplicate of #6353 as the underlying code is the same, but I will leave this open so other users can find the problem searching for either lint.

@phansch phansch added the I-false-positive Issue: The lint was triggered on code it shouldn't have label Dec 18, 2020
bors added a commit that referenced this issue Jan 17, 2021
Explicitly document false positives

Adds documentation for known false positives for the `await_holding*` lints.

Issues:

#6353
#6446
bors added a commit that referenced this issue Jan 17, 2021
Explicitly document false positives

Adds documentation for known false positives for the `await_holding*` lints.

Issues:

#6353
#6446

changelog: document FPs for the ``await_holding_*`` lints
@flip1995
Copy link
Member

Fixing this would involve fixing/improving drop-tracking in rustc when it comes to building the generator_interior_types collection. drop-tracking is currently disabled by default though. So nothing much that could be done here on the Clippy side.

bors added a commit that referenced this issue Feb 18, 2022
Fix `await_holding_lock` not linting `parking_lot` Mutex/RwLock

This adds tests for `RwLock` and `parking_lot::{Mutex, RwLock}`, which were added before in 2dc8c08, but never tested in UI tests. I noticed this while reading [fasterthanli.me](https://fasterthanli.me/articles/a-rust-match-made-in-hell) latest blog post, complaining that Clippy doesn't catch this for `parking_lot`. (Too many people read his blog, he's too powerful)

Some more things:
- Adds a test for #6446
- Improves the lint message

changelog: [`await_holding_lock`]: Now also lints for `parking_lot::{Mutex, RwLock}`
@Sytten
Copy link

Sytten commented Jun 10, 2024

Has anything changed in the last year+ concerning drop tracking? @flip1995

@flip1995
Copy link
Member

I don't know. With a quick grep though the Rust codebase, I couldn't find the drop_tracking option anymore. So it might be enabled by default now. But I'm not sure.

@urkle
Copy link

urkle commented Oct 4, 2024

Drop tracking was enabled by default in rust 1.74 ( https://github.com/rust-lang/rust/releases/tag/1.74.0 )

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

Successfully merging a pull request may close this issue.

6 participants