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

RFC: Less complex, footgun free API #104

Closed
notgull opened this issue Dec 19, 2023 · 2 comments · Fixed by #105
Closed

RFC: Less complex, footgun free API #104

notgull opened this issue Dec 19, 2023 · 2 comments · Fixed by #105

Comments

@notgull
Copy link
Member

notgull commented Dec 19, 2023

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
Copy link
Member

zeenix commented Dec 19, 2023

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
Copy link
Member

fogti commented Dec 19, 2023

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.))

notgull added a commit that referenced this issue Dec 20, 2023
Minimal amount of changes to make EventListener a heap-allocated type
again. The existence of the EventListener implies that it is already
listening; accordingly the new() and listen() methods on EventListener
have been removed.

cc #104

Signed-off-by: John Nunley <dev@notgull.net>
notgull added a commit to smol-rs/async-process that referenced this issue Dec 26, 2023
cc smol-rs/event-listener#104

Signed-off-by: John Nunley <dev@notgull.net>
notgull added a commit that referenced this issue Jan 15, 2024
Minimal amount of changes to make EventListener a heap-allocated type
again. The existence of the EventListener implies that it is already
listening; accordingly the new() and listen() methods on EventListener
have been removed.

cc #104

Signed-off-by: John Nunley <dev@notgull.net>
notgull added a commit that referenced this issue Feb 3, 2024
Minimal amount of changes to make EventListener a heap-allocated type
again. The existence of the EventListener implies that it is already
listening; accordingly the new() and listen() methods on EventListener
have been removed.

cc #104

Signed-off-by: John Nunley <dev@notgull.net>
notgull added a commit that referenced this issue Feb 3, 2024
Minimal amount of changes to make EventListener a heap-allocated type
again. The existence of the EventListener implies that it is already
listening; accordingly the new() and listen() methods on EventListener
have been removed.

cc #104

Signed-off-by: John Nunley <dev@notgull.net>
notgull added a commit that referenced this issue Feb 3, 2024
Minimal amount of changes to make EventListener a heap-allocated type
again. The existence of the EventListener implies that it is already
listening; accordingly the new() and listen() methods on EventListener
have been removed.

cc #104

Signed-off-by: John Nunley <dev@notgull.net>
notgull added a commit to smol-rs/async-process that referenced this issue Feb 12, 2024
cc smol-rs/event-listener#104

Signed-off-by: John Nunley <dev@notgull.net>
notgull added a commit that referenced this issue Apr 11, 2024
Minimal amount of changes to make EventListener a heap-allocated type
again. The existence of the EventListener implies that it is already
listening; accordingly the new() and listen() methods on EventListener
have been removed.

cc #104

Signed-off-by: John Nunley <dev@notgull.net>
notgull added a commit that referenced this issue May 4, 2024
Minimal amount of changes to make EventListener a heap-allocated type
again. The existence of the EventListener implies that it is already
listening; accordingly the new() and listen() methods on EventListener
have been removed.

cc #104

Signed-off-by: John Nunley <dev@notgull.net>
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