Skip to content

Conversation

@notgull
Copy link
Member

@notgull notgull commented Dec 20, 2023

This PR sets up a new API that aims to implement #104. It sets up a Listener trait, which is implemented for a heap-based EventListener and a stack-based StackListener. The latter is only able to be instantiated through the "listener!" macro.

While writing this, I realized that this breaks the hand-rolled futures that async-channel and async-lock use nowadays. For now we can burn that bridge when we get there and just use EventListener in the hand-rolled futures.

Closes #104

Copy link
Member

@zeenix zeenix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice work. I believe we're now getting to an API that's simple, sound and at the same time allows to be easily heap-free. 👍

Some nitpicks though. I do realize this is still a draft but thought you'd appreciate early feedback. :)

notgull added a commit to smol-rs/async-lock that referenced this pull request Dec 26, 2023
cc smol-rs/event-listener#105

Signed-off-by: John Nunley <dev@notgull.net>
notgull added a commit to smol-rs/async-broadcast that referenced this pull request Dec 29, 2023
cc smol-rs/event-listener#105

Signed-off-by: John Nunley <dev@notgull.net>
@notgull notgull marked this pull request as ready for review January 1, 2024 18:21
Jules-Bertholet pushed a commit to smol-rs/async-lock that referenced this pull request Jan 11, 2024
cc smol-rs/event-listener#105

Signed-off-by: John Nunley <dev@notgull.net>
@notgull
Copy link
Member Author

notgull commented Jan 16, 2024

It appears that this somehow introduces a deadlock into async-lock. See the test failure here: https://github.com/smol-rs/async-lock/actions/runs/7493609820/job/20509520336

@zeenix
Copy link
Member

zeenix commented Jan 30, 2024

It appears that this somehow introduces a deadlock into async-lock. See the test failure here: https://github.com/smol-rs/async-lock/actions/runs/7493609820/job/20509520336

Any progress on this? If this is proving difficult and will take time, perhaps we can first have other crates ported to event-handler 4 so folks don't end up depending on multiple versions of event-handler?

@notgull
Copy link
Member Author

notgull commented Jan 30, 2024

Any progress on this?

None yet. At the moment I'm still trying to replicate the deadlock using only event-listener.

If this is proving difficult and will take time, perhaps we can first have other crates ported to event-handler 4 so folks don't end up depending on multiple versions of event-handler?

I would agree with this. Most of the smol crates are already ported to event-listener v4.

@zeenix
Copy link
Member

zeenix commented Jan 30, 2024

If this is proving difficult and will take time, perhaps we can first have other crates ported to event-handler 4 so folks don't end up depending on multiple versions of event-handler?

I would agree with this. Most of the smol crates are already ported to event-listener v4.

Cool. I'll see if I can provide a PR for async-broadcast. Any others left?

@notgull
Copy link
Member Author

notgull commented Jan 31, 2024

Not to my knowledge

@notgull
Copy link
Member Author

notgull commented Jan 31, 2024

At the moment I think the deadlock is a Miri bug, so I've disabled those tests in smol-rs/async-lock@1d97f8a. I think this should be ready now.

@notgull notgull requested a review from zeenix January 31, 2024 05:03
Copy link
Member

@zeenix zeenix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alright, let's do it. :)

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>
This commit creates the Listener trait and moves most of EventListener's
functionality to that trait.

Signed-off-by: John Nunley <dev@notgull.net>
It is instantiated with the listener!() macro.

Signed-off-by: John Nunley <dev@notgull.net>
This brings in the try-lock dependency.

Signed-off-by: John Nunley <dev@notgull.net>
Signed-off-by: John Nunley <dev@notgull.net>
* Make sure Unpin is implemented for StackListener
* Purge the prelude
* Remove unused imports from doctests

Signed-off-by: John Nunley <dev@notgull.net>
@notgull notgull merged commit 6e6202b into master Feb 3, 2024
@notgull notgull deleted the notgull/break branch February 3, 2024 17:49
@notgull notgull mentioned this pull request Feb 5, 2024
notgull added a commit to smol-rs/async-broadcast that referenced this pull request Feb 7, 2024
cc smol-rs/event-listener#105

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

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 this pull request may close these issues.

RFC: Less complex, footgun free API

3 participants