Skip to content

Commit

Permalink
bugfix: Remove listener if one already exists
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
notgull authored Dec 18, 2023
1 parent cc33cc5 commit c2d1ccb
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 1 deletion.
13 changes: 12 additions & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -810,13 +810,24 @@ impl<T> EventListener<T> {
///
/// This method can only be called after the listener has been pinned, and must be called before
/// the listener is polled.
///
/// Notifications that exist when this function is called will be discarded.
pub fn listen(mut self: Pin<&mut Self>, event: &Event<T>) {
let inner = {
let inner = event.inner();
unsafe { Arc::clone(&ManuallyDrop::new(Arc::from_raw(inner))) }
};

let ListenerProject { event, listener } = self.as_mut().project().listener.project();
let ListenerProject {
event,
mut listener,
} = self.as_mut().project().listener.project();

// If an event is already registered, make sure to remove it.

This comment has been minimized.

Copy link
@fogti

fogti Dec 18, 2023

Member

note: this comment appears to be wrong, it removes the listener from the event, not unregistering the event.

if let Some(current_event) = event.as_ref() {
current_event.remove(listener.as_mut(), false);
}

let inner = event.insert(inner);
inner.insert(listener);

Expand Down
13 changes: 13 additions & 0 deletions tests/notify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -189,3 +189,16 @@ fn notify_all_fair() {
.poll(&mut Context::from_waker(&waker3))
.is_ready());
}

#[test]
fn more_than_one_event() {
let event = Event::new();
let event2 = Event::new();

let mut listener = Box::pin(EventListener::<()>::new());
listener.as_mut().listen(&event);
listener.as_mut().listen(&event2);

drop(listener);
event.notify(1);
}

0 comments on commit c2d1ccb

Please sign in to comment.