Upgrading v4 to v5 panic: Listener was never inserted into the list

[jbr]

I'm unsure if I'm holding the new interface wrong somehow or if I've encountered a bug.

Given this diff that attempts to track the event-listener interface changes, I'm encountering a deterministic panic like:

event-listener-5.2.0/src/lib.rs:1251:36:
listener was never inserted into the list

and as far as I can tell the code reduces to

let n = AtomicUsize::from(1);
let event = Event::new();
let mut listener = event.listen();
loop {
    if 0 == n.load(Ordering::SeqCst) {
        return Poll::Ready(());
    };
    ready!(Pin::new(&mut listener).poll(cx));
}

Do I need to replace the listener every time I poll it? Am I holding this wrong? Is this a bug?

If I'm using it wrong I'll happily contribute documentation to help others avoid this error

[notgull]

It seems you are using the future like this:

let mut listener = event.listen();
loop {
    if 0 == n.load(Ordering::SeqCst) {
        return Poll::Ready(());
    };
    ready!(Pin::new(&mut listener).poll(cx));
} 

It should be used like this:

let mut listener = None;
loop {
    if 0 == n.load(Ordering::SeqCst) {
        return Poll::Ready(());
    };
    if let Some(listener) = listener.as_mut() {
        ready!(Pin::new(&mut listener).poll(cx));
        listener = None;
    } else {
        listener = Some(event.listen());
    }
}

Polling the EventListener after it has returned Ready will panic, and further polls should use a completely new EventListener instead.

The v4 API kept track of this state inside of the EventListener, but this caused confusion and was removed in v5.

[notgull]

If you think this could be made clearer in documentation I would accept a PR.

[jbr]

Would it make sense to just always return ready once it has ever been ready instead of panicking? That seems a lot easier to document and explain.

[notgull]

Rust's documentation seems to imply that a panic is the preferred option in this case.

[jbr]

It looks like EventListener holds an InnerListener that contains a reference to the Event. Would it work to have a configurable option that allows the listener to reregister itself on completion? It feels awkward to require user code to match Pin::new(&mut listener).poll(cx) { Poll::Ready(()) => { *listener = event.listen(); ... }, ... }

[notgull]

It's a bad idea to have the EventListener automatically insert itself into the list. Generally, the flow should be:

• Check your condition.
• Register the listener into the list.
• Check your condition, ideally with stronger ordering.
• Wait on the listener.

If you register the listener into the list without actively polling an event, it can lead to strange behavior. E.g. your future received a notification meant for another task