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

Fix the EventListener::new() footgun #94

Merged
merged 5 commits into from
Nov 15, 2023
Merged

Fix the EventListener::new() footgun #94

merged 5 commits into from
Nov 15, 2023

Conversation

notgull
Copy link
Member

@notgull notgull commented Nov 6, 2023

This is a breaking change. It makes new() take no parameters in its
signature and listen() take a reference to an event in its signature.
This should avoid a footgun where a listener can be waited on without
listening on it.

Closes #91
cc @zeenix

@notgull notgull requested a review from zeenix November 6, 2023 02:43
@notgull
Copy link
Member Author

notgull commented Nov 6, 2023 •
edited

This causes a worrying performance regression.

notify_and_wait         time:   [549.58 µs 549.97 µs 550.42 µs]
                        change: [+76.620% +77.217% +77.648%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 16 outliers among 100 measurements (16.00%)
  6 (6.00%) low severe
  7 (7.00%) low mild
  1 (1.00%) high mild
  2 (2.00%) high severe

@zeenix
Copy link
Member

zeenix commented Nov 6, 2023 •
edited

This causes a worrying performance regression.

  • Is this always reproducible? I found criterion-based benchmarking to be a bit unreliable.
  • Where is the bottleneck?

@notgull
Copy link
Member Author

notgull commented Nov 12, 2023

  • Where is the bottleneck?

The bottleneck comes from the additional Option around the Listener type in EventListener. Let me try shuffling it around and seeing if the compiler likes it better.

@notgull
Copy link
Member Author

notgull commented Nov 15, 2023 

$ cargo bench                 
   Compiling event-listener v3.0.1 (/home/jtnunley/Projects/smol-rs/event-listener)
    Finished bench [optimized] target(s) in 3.15s
     Running benches/bench.rs (/hard_data/cargo_target/release/deps/bench-eeda2fe5c3372937)
notify_and_wait         time:   [207.45 µs 207.70 µs 207.95 µs]
                        change: [+7.4551% +7.6268% +7.8167%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild

I find this regression much more palatable.

@notgull
Fix the EventListener::new() footgun
e10eb26 
This is a breaking change. It makes `new()` take no parameters in its
signature and `listen()` take a reference to an event in its signature.
This should avoid a footgun where a listener can be waited on without
listening on it.

Closes #91

Signed-off-by: John Nunley <dev@notgull.net>
@notgull
Fix benchmark
597903b 
Signed-off-by: John Nunley <dev@notgull.net>
@notgull
Please clippy
cda7be5 
Signed-off-by: John Nunley <dev@notgull.net>
@notgull
Please clippy again
46e7c13 
Signed-off-by: John Nunley <dev@notgull.net>
@notgull
Move the Option<> over the event instead
1911542 
Signed-off-by: John Nunley <dev@notgull.net>
zeenix
zeenix approved these changes Nov 15, 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.

Thanks for doing this work and making sure there is no significant performance hit. 👍

@notgull notgull merged commit 21b34bf into master Nov 15, 2023
9 checks passed
@notgull notgull deleted the notgull/footgun branch November 15, 2023 14:01
This was referenced Nov 18, 2023
Merged
Merged
Merged
notgull added a commit to smol-rs/event-listener-strategy that referenced this pull request Nov 19, 2023
@notgull
breaking: Bump to event-listener v4.0.0
ce964df 
smol-rs/event-listener#94

Signed-off-by: John Nunley <dev@notgull.net>
@notgull notgull mentioned this pull request Dec 14, 2023
Closed
notgull added a commit that referenced this pull request Dec 16, 2023
@notgull
Remove listener if one already exists
4c988b2 
This commit makes it so EventListener::listen() does not overwrite
existing listeners if they already exist. If the listener is already
registered in an event listener list, it removes the listener from that
list before registering the new listener. This soundness bug was missed
during #94.

This is a patch-compatible fix for #100. We may also want an API-level
fix for #100 as well. This is up for further discussion.

Signed-off-by: John Nunley <dev@notgull.net>
@notgull notgull mentioned this pull request Dec 16, 2023
Merged
notgull added a commit that referenced this pull request Dec 16, 2023
@notgull
Remove listener if one already exists
2591e89 
This commit makes it so EventListener::listen() does not overwrite
existing listeners if they already exist. If the listener is already
registered in an event listener list, it removes the listener from that
list before registering the new listener. This soundness bug was missed
during #94.

This is a patch-compatible fix for #100. We may also want an API-level
fix for #100 as well. This is up for further discussion.

Signed-off-by: John Nunley <dev@notgull.net>
notgull added a commit that referenced this pull request Dec 18, 2023
@notgull
bugfix: Remove listener if one already exists
c2d1ccb 
This commit makes it so EventListener::listen() does not overwrite
existing listeners if they already exist. If the listener is already
registered in an event listener list, it removes the listener from that
list before registering the new listener. This soundness bug was missed
during #94.

This is a patch-compatible fix for #100. We may also want an API-level
fix for #100 as well. This is up for further discussion.

Signed-off-by: John Nunley <dev@notgull.net>
@notgull notgull mentioned this pull request Dec 19, 2023
Closed
notgull added a commit that referenced this pull request Apr 11, 2024
@notgull
bugfix: Remove listener if one already exists
5f059cc 
This commit makes it so EventListener::listen() does not overwrite
existing listeners if they already exist. If the listener is already
registered in an event listener list, it removes the listener from that
list before registering the new listener. This soundness bug was missed
during #94.

This is a patch-compatible fix for #100. We may also want an API-level
fix for #100 as well. This is up for further discussion.

Signed-off-by: John Nunley <dev@notgull.net>
notgull added a commit that referenced this pull request May 4, 2024
@notgull
bugfix: Remove listener if one already exists
c554853 
This commit makes it so EventListener::listen() does not overwrite
existing listeners if they already exist. If the listener is already
registered in an event listener list, it removes the listener from that
list before registering the new listener. This soundness bug was missed
during #94.

This is a patch-compatible fix for #100. We may also want an API-level
fix for #100 as well. This is up for further discussion.

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

Remove EventListener::new footgun
2 participants
@notgull @zeenix