smol-rs / event-listener

Notify async tasks or threads
Apache License 2.0
449 stars 30 forks source link

RFC: Less complex, footgun free API #104

Closed notgull closed 9 months ago

notgull commented 11 months ago

Continued from the discussion in #100

The APIs of v3.x and v4.x increase complexity at the cost of adding more footguns. Originally I'd wanted to keep the API small and simple, but this appears to have introduced a handful of different ways to mess up (e.g. panicking when creating a new EventListener without listening on it). In addition, v4.0 (#94) introduced a not-insignificant amount of overhead over the v3.x API. Therefore, as much as it pains me to have three breaking changes in this crate in such a short span of time, I think we need a better API.

Note: I am bad at naming things. None of these names are final.

Reference-Level Explanation

The idea I proposed in #100: is as follows: add an API based around a Listener trait that would look like this:

pub trait Listener<T>: Future<Output = T> + Sealed {
    fn wait(self) -> T;
    fn wait_timeout(self, timeout: Duration) -> Option<T>;
    fn wait_deadline(self, deadline: Instant) -> Option<T>;

    fn discard(self) -> Option<T>;
    fn listens_to(&self, event: &Event<T>) -> bool;
    fn same_event(&self, other: &(impl Listener<T> + ?Sized)) -> bool;
}

This Listener trait can do anything that the current EventListener type can do. It can be awaited or it can be used to block the current thread. Alternatively, it can be discarded.

To emulate the previous 2.x API, we would have an EventListener type. This is a heap-allocated event listener that can be moved freely.

impl<T> Event<T> {
    pub fn listen(&self) -> EventListener<T>;
}

pub struct EventListener<T = ()>(/* ... */);

impl Listener<T> for EventListener<T> { /* ... */ }

It would be used like this:

struct Flag {
    flag: AtomicBool,
    change_ops: Event<()>
}

impl Flag {
    async fn wait(&self) {
        loop {
            // Check the flag.
            if self.flag.load(Ordering::Relaxed) {
                return;
            }

            // Create a listener. By default it is already inserted into the list.
            let listener = self.change_ops.listen();

            // Check the flag again.
            if self.flag.load(Ordering::SeqCst) {
                return;
            }

            // Wait on the listener.
            listener.await;
        }
    }

    fn notify(&self) {
        self.flag.store(true, Ordering::Release);
        self.change_ops.notify(1);
    }
}

However, this is inefficient, as it preforms a heap allocation every time listen() is called. We also provide an API that allows one to use the stack instead of the heap, at a slightly higher complexity cost. To start, you create a StackSlot, which contains all of the state of the EventListener but stored on the stack. After being pinned, it can be transformed into a StackListener.

impl<T> Event<T> {
    pub fn stack_slot(&self) -> StackSlot<'_, T> { /* ... */ }
}

pub struct StackSlot<'ev, T>(/* ... */);
pub struct StackListener<'ev, 'listener, T>(/* ... */);

impl<'ev, T> StackSlot<'ev, T> {
    pub fn listen<'this>(self: Pin<&'this mut Self>) -> StackListener<'ev, 'this, T> { /* ... */ }
}

impl<T> Listener<T> for StackListener<'_, '_, T> { /* ... */ }

In addition, a macro will be provided that creates a StackSlot from an Event and automatically pins it to the heap.

macro_rules! listener {
    ($name:ident, $event:expr) => { /* ... */ }
}

/*
listener!(l, ev);
Expands to:
let mut l = ev.stack_slot();
let mut l = Pin::new_unchecked(&mut l);
*/

The example from above can be modified to look like this:

struct Flag {
    flag: AtomicBool,
    change_ops: Event<()>
}

impl Flag {
    async fn wait(&self) {
        listener!(slot, &self.change_ops);

        loop {
            // Check the flag.
            if self.flag.load(Ordering::Relaxed) {
                return;
            }

            // Insert the listener into the list.
            let listener = slot.listen();

            // Check the flag again.
            if self.flag.load(Ordering::SeqCst) {
                return;
            }

            // Wait on the listener.
            listener.await;
        }
    }

    fn notify(&self) {
        self.flag.store(true, Ordering::Release);
        self.change_ops.notify(1);
    }
}

@smol-rs/admins Thoughts on this?

zeenix commented 11 months ago

In addition, a macro will be provided that creates a StackSlot from an Event and automatically pins it to the heap.

I think you meant to write "stack". :)

@smol-rs/admins Thoughts on this?

Sounds good but to keep things simpler and limiting public API, I would suggest keeping everything Stack* internal (doc hidden) except for the macro as that should be the only way to do stack-based listeners.

fogti commented 11 months ago

Sounds good. (as usual before finally merging this after implementation, we should of course test it in various downstream cases to make sure nothing breaks weirdly (e.g. interactions of lifetimes, etc.))