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.

Sign up for GitHub

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

Jump to bottom

Setup a faster, less footgun-prone API #105

Merged
merged 6 commits into from
Feb 3, 2024
Merged

Setup a faster, less footgun-prone API #105

merged 6 commits into from
Feb 3, 2024

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

zeenix
zeenix requested changes Dec 20, 2023
View reviewed changes
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. :)

src/lib.rs Outdated Show resolved Hide resolved
benches/bench.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
This was referenced Dec 25, 2023
Merged
Merged
notgull added a commit to smol-rs/async-lock that referenced this pull request Dec 26, 2023
@notgull
Port to event-listener v5.0.0
6fa4e41 
cc smol-rs/event-listener#105

Signed-off-by: John Nunley <[email protected]>
@notgull notgull mentioned this pull request Dec 26, 2023
Merged
@TTWNO TTWNO mentioned this pull request Dec 28, 2023
Merged
@notgull notgull mentioned this pull request Dec 29, 2023
Merged
notgull added a commit to smol-rs/async-broadcast that referenced this pull request Dec 29, 2023
@notgull
Port to event-listener v5.0
a6d844b 
cc smol-rs/event-listener#105

Signed-off-by: John Nunley <[email protected]>
@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
@notgull @Jules-Bertholet
Port to event-listener v5.0.0
7e9b1dd 
cc smol-rs/event-listener#105

Signed-off-by: John Nunley <[email protected]>
@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
zeenix
zeenix requested changes Jan 31, 2024
View reviewed changes
src/lib.rs Outdated Show resolved Hide resolved
zeenix
zeenix reviewed Feb 1, 2024
View reviewed changes
src/notify.rs Show resolved Hide resolved
zeenix
zeenix reviewed Feb 3, 2024
View reviewed changes
src/lib.rs Show resolved Hide resolved
zeenix
zeenix approved these changes Feb 3, 2024
View reviewed changes
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. :)

@notgull
feat: Move EventListener back onto the heap
fbc26e6 
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 <[email protected]>
@notgull
feat: Create Listener trait
eddd521 
This commit creates the Listener trait and moves most of EventListener's
functionality to that trait.

Signed-off-by: John Nunley <[email protected]>
@notgull
feat: Add stack-based listener
65c6f9a 
It is instantiated with the listener!() macro.

Signed-off-by: John Nunley <[email protected]>
@notgull
ex: Remove unsafe code from mutex example
585b549 
This brings in the try-lock dependency.

Signed-off-by: John Nunley <[email protected]>
@notgull
feat: Move Future impl to forwarding macro
053e6d1 
Signed-off-by: John Nunley <[email protected]>
@notgull
chore: Polish the implementation of the new API
d2a5e11 
* Make sure Unpin is implemented for StackListener
* Purge the prelude
* Remove unused imports from doctests

Signed-off-by: John Nunley <[email protected]>
@notgull notgull merged commit 6e6202b into master Feb 3, 2024
9 checks passed
@notgull notgull deleted the notgull/break branch February 3, 2024 17:49
@notgull notgull mentioned this pull request Feb 5, 2024
Merged
notgull added a commit to smol-rs/async-broadcast that referenced this pull request Feb 7, 2024
@notgull
Port to event-listener v5.0
669f0e4 
cc smol-rs/event-listener#105

Signed-off-by: John Nunley <[email protected]>
notgull added a commit to smol-rs/async-lock that referenced this pull request Feb 12, 2024
@notgull
m: Port to event-listener v5.0.0
ff4c96f 
cc smol-rs/event-listener#105

Signed-off-by: John Nunley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zeenix zeenix zeenix approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RFC: Less complex, footgun free API
2 participants
@notgull @zeenix