Remove EventListener::new footgun

[zeenix]

As discussed in #90 and #89, EventListener::new has a big footgun: it takes &Event ref but doesn't add itself to listen to the event passed. This is highly unintuitive IMO and if possible, we should remedy this. Two options here:

1. Add a new method that doesn't take a Event ref and deprecate new in favor of that. OR
2. (if possible) make listen() implicit in the first poll of EventListener and deprecate EventListener::listen.

I'd very much prefer the 2, since it removes the footgun completely.

[notgull]

I agree with the idea that new() should take no parameters and listen() should take the &Event parameter; it makes more sense that way. Unfortunately that would be breaking change as this point, and deprecation/introduction of new methods introduces its own footguns, as mixing the two types of methods would lead to unexpected panics. However, I disagree with the second option, as it would make polling/waiting on the EventListener much more tricky.

If the listener is linked into the list on the first poll of the Future, it makes it more difficult to receive events. Until the listener is fully linked into the list, it will not receive events. Meaning, if an event occurs while the listener is being atomically linked into the list, it will not receive that event. This makes it necessary to check any atomic flags before and after the list is inserted. This could would become necessary:

loop {
    if check_the_flag() {
        break;
    }

    let mut listener = EventListener::new(&event);
    pin!(listener);

    // Poll the future once to insert it into the list.
    future::poll_once(listener.as_mut()).await;

    if check_the_flags() {
        break;
    }

    // Now we can fully await on the listener.
    listener.await;
}

This introduces a much larger footgun, however, as a naive user may just try to .await on the future without making sure the condition that they're waiting on hasn't changed after the insert. This code will deadlock unpredictably:

loop {
    if check_the_flag() {
        break;
    }

    let listener = EventListener::new(&event);
    pin!(listener);
    listener.await;
}

Inserting on first poll also neglects the wait(), use case, which doesn't really have a "poll" mechanism at all. So the above deadlock would become the mandatory way of using wait(), unless you do wait_timeout(Duration::from_secs(0)) or something to "register" it first. But that adds its own additional complexity.

One possible way around this is to have the first await or wait() on the EventListener insert itself into the list and succeed immediately, so that the user can check the flag before awaiting/wait()ing on the listener a second time, which would then wait properly. But this has its own nuances/drawbacks and should be discussed further if this is the direction that we want to go.

[zeenix]

Thanks so much for explaining in detail.

One possible way around this is to have the first await or wait() on the EventListener insert itself into the list and succeed immediately, so that the user can check the flag before awaiting/wait()ing on the listener a second time, which would then wait properly. But this has its own nuances/drawbacks and should be discussed further if this is the direction that we want to go.

That doesn't seem extremely bad to me but yeah, I agree it's still a bit weird/unexpected behavior. As I wrote on the Matrix channel, I've no objection on doing a 4.0 release that fixes it. If we do that, IMO we should yank the 3.0 on crates.io.

[notgull]

Sounds good. But we should probably wait until we roll out the rest of smol's breaking changes before then.

[zeenix]

But we should probably wait until we roll out the rest of smol's breaking changes before then.

That would delay zbus 4.0 even further than it already is. :( Why don't we do both:

• Deprecate new for another method for now (since there will be no args, I think we can just go for Default::default impl).
• In the next break, remove the new method. Alternatively, we can keep new but just remove its argument.

[taiki-e]

event-listener is not used by any other smol crates public API, so I think it is okay to make a new breaking release (4.0) right now if needed.

[zeenix]

event-listener is not used by any other smol crates public API, so I think it is okay to make a new breaking release right now if needed.

Oh, even better!