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

This seems actually not stable(Dead lock) #143

Closed
beckend opened this issue Aug 18, 2024 · 6 comments · Fixed by #145
Closed

This seems actually not stable(Dead lock) #143

beckend opened this issue Aug 18, 2024 · 6 comments · Fixed by #145
Assignees

Comments

@beckend
Copy link
Contributor

beckend commented Aug 18, 2024

https://github.com/beckend/event-listener/tree/event_listener_bug

cargo run --release --example mutex

https://github.com/beckend/event-listener/blob/event_listener_bug/examples/mutex.rs#L127-L151

It dead locks.

@beckend beckend changed the title This seems actually is not stable. This seems actually is not stable(Dead lock) Aug 18, 2024
@beckend beckend changed the title This seems actually is not stable(Dead lock) This seems actually not stable(Dead lock) Aug 18, 2024
@notgull notgull self-assigned this Aug 18, 2024
@notgull
Copy link
Member

notgull commented Aug 22, 2024

Thanks for the report! This seems tricky to replicate, let me see if I can loom it.

@nazar-pc
Copy link

Simple reproduction found in smol-rs/async-lock#91:

use event_listener::Event;
use std::pin::pin;

fn main() {
    futures::executor::block_on(async {
        let event = Event::new();

        let listen_a = pin!(event.listen());
        let listen_b = pin!(event.listen());

        drop(listen_a);

        event.notify(1);
        listen_b.await;
    });
}

The reason is explained by clippy:

warning: call to `std::mem::drop` with a value that does not implement `Drop`. Dropping such a type only extends its contained lifetimes
  --> main.rs:11:9
   |
11 |         drop(listen_a);
   |         ^^^^^^^^^^^^^^
   |
note: argument has type `std::pin::Pin<&mut event_listener::EventListener>`
  --> main.rs:11:14
   |
11 |         drop(listen_a);
   |              ^^^^^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#drop_non_drop
   = note: `#[warn(clippy::drop_non_drop)]` on by default

No feedback that one of the listeners was dropped.

@notgull
Copy link
Member

notgull commented Sep 20, 2024

Thanks for the repro! I would like to take some time to set up look tests for this.

@nazar-pc
Copy link

The test is simple:

#[test]
fn drop_notified3() {
    let event = Event::new();

    let l1 = pin!(event.listen());
    let mut l2 = pin!(event.listen());

    drop(l1);
    assert_eq!(event.notify(1), 1);
    assert!(is_notified(&mut l2));
}

I'm trying to come up with a fix now.

@nazar-pc
Copy link

Hm... looks like false alarm, it didn't work because pin!() extended lifetime of the inner value and that means Drop::drop is never called. Back to square one I guess.

@notgull
Copy link
Member

notgull commented Oct 10, 2024

Quick update on this. I've gotten my hands on hardware that's good enough that I can use it to run Loom tests. Standby while I fully instrument this crate.

notgull added a commit that referenced this issue Oct 29, 2024
I forgot to run notify() when I rewrote this example without unsafe
code. It looks like it prevents deadlocks in this mutex implementation.

I believe this fixes #143.

Signed-off-by: John Nunley <[email protected]>
notgull added a commit that referenced this issue Oct 29, 2024
I forgot to run notify() when I rewrote this example without unsafe
code. It looks like it prevents deadlocks in this mutex implementation.

I believe this fixes #143.

Signed-off-by: John Nunley <[email protected]>
notgull added a commit that referenced this issue Oct 31, 2024
I forgot to run notify() when I rewrote this example without unsafe
code. It looks like it prevents deadlocks in this mutex implementation.

I believe this fixes #143.

Signed-off-by: John Nunley <[email protected]>
@notgull notgull closed this as completed in 633b2c6 Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants