From 21b34bf6dd483a3e7d5f92d3768969369aa78d31 Mon Sep 17 00:00:00 2001 From: John Nunley Date: Wed, 15 Nov 2023 06:01:17 -0800 Subject: [PATCH] breaking: Fix the EventListener::new() footgun 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 --- benches/bench.rs | 4 +- src/lib.rs | 114 +++++++++++++++++++++++++++++------------------ src/std.rs | 5 --- 3 files changed, 72 insertions(+), 51 deletions(-) diff --git a/benches/bench.rs b/benches/bench.rs index a3412c0..55557a6 100644 --- a/benches/bench.rs +++ b/benches/bench.rs @@ -9,7 +9,7 @@ const COUNT: usize = 8000; fn bench_events(c: &mut Criterion) { c.bench_function("notify_and_wait", |b| { let ev = Event::new(); - let mut handles = iter::repeat_with(|| EventListener::new(&ev)) + let mut handles = iter::repeat_with(EventListener::new) .take(COUNT) .collect::>(); @@ -17,7 +17,7 @@ fn bench_events(c: &mut Criterion) { for handle in &mut handles { // SAFETY: The handle is not moved out. let listener = unsafe { Pin::new_unchecked(handle) }; - listener.listen(); + listener.listen(&ev); } ev.notify(COUNT); diff --git a/src/lib.rs b/src/lib.rs index 4b2ce87..88e1038 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -268,8 +268,8 @@ impl Event { /// use event_listener::{Event, EventListener}; /// /// let event = Event::new(); - /// let mut listener = Box::pin(EventListener::new(&event)); - /// listener.as_mut().listen(); + /// let mut listener = Box::pin(EventListener::new()); + /// listener.as_mut().listen(&event); /// ``` /// /// It creates a new listener, pins it to the heap, and inserts it into the linked list @@ -280,8 +280,8 @@ impl Event { /// [`EventListener`] is `listen`ing before waiting on it; panics may occur otherwise. #[cold] pub fn listen(&self) -> Pin>> { - let mut listener = Box::pin(EventListener::new(self)); - listener.as_mut().listen(); + let mut listener = Box::pin(EventListener::new()); + listener.as_mut().listen(self); listener } @@ -711,7 +711,7 @@ pin_project_lite::pin_project! { /// } /// }); /// - /// let listener = EventListener::new(&event); + /// let listener = EventListener::new(); /// /// // Make sure that the event listener is pinned before doing anything else. /// // @@ -731,7 +731,7 @@ pin_project_lite::pin_project! { /// } else { /// // We need to insert ourselves into the list. Since this insertion is an atomic /// // operation, we should check the flag again before waiting. - /// listener.as_mut().listen(); + /// listener.as_mut().listen(&event); /// } /// } /// ``` @@ -764,6 +764,12 @@ unsafe impl Sync for EventListener {} impl core::panic::UnwindSafe for EventListener {} impl core::panic::RefUnwindSafe for EventListener {} +impl Default for EventListener { + fn default() -> Self { + Self::new() + } +} + impl fmt::Debug for EventListener { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("EventListener") @@ -785,29 +791,34 @@ impl EventListener { /// use event_listener::{Event, EventListener}; /// /// let event = Event::new(); - /// let listener = EventListener::new(&event); + /// let listener = EventListener::new(); /// /// // Make sure that the listener is pinned and listening before doing anything else. /// let mut listener = Box::pin(listener); - /// listener.as_mut().listen(); + /// listener.as_mut().listen(&event); /// ``` - pub fn new(event: &Event) -> Self { - let inner = event.inner(); - - let listener = Listener { - event: unsafe { Arc::clone(&ManuallyDrop::new(Arc::from_raw(inner))) }, - listener: None, - }; - - Self { listener } + pub fn new() -> Self { + Self { + listener: Listener { + event: None, + listener: None, + }, + } } /// Register this listener into the given [`Event`]. /// /// This method can only be called after the listener has been pinned, and must be called before /// the listener is polled. - pub fn listen(self: Pin<&mut Self>) { - self.listener().insert(); + pub fn listen(mut self: Pin<&mut Self>, event: &Event) { + 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 inner = event.insert(inner); + inner.insert(listener); // Make sure the listener is registered before whatever happens next. notify::full_fence(); @@ -821,13 +832,13 @@ impl EventListener { /// use event_listener::{Event, EventListener}; /// /// let event = Event::new(); - /// let mut listener = Box::pin(EventListener::new(&event)); + /// let mut listener = Box::pin(EventListener::new()); /// /// // The listener starts off not listening. /// assert!(!listener.is_listening()); /// /// // After listen() is called, the listener is listening. - /// listener.as_mut().listen(); + /// listener.as_mut().listen(&event); /// assert!(listener.is_listening()); /// /// // Once the future is notified, the listener is no longer listening. @@ -922,7 +933,7 @@ impl EventListener { /// assert!(!listener2.as_mut().discard()); /// ``` pub fn discard(self: Pin<&mut Self>) -> bool { - self.listener().discard() + self.project().listener.discard() } /// Returns `true` if this listener listens to the given `Event`. @@ -939,7 +950,11 @@ impl EventListener { /// ``` #[inline] pub fn listens_to(&self, event: &Event) -> bool { - ptr::eq::>(&**self.inner(), event.inner.load(Ordering::Acquire)) + if let Some(inner) = &self.listener.event { + return ptr::eq::>(&**inner, event.inner.load(Ordering::Acquire)); + } + + false } /// Returns `true` if both listeners listen to the same `Event`. @@ -956,15 +971,19 @@ impl EventListener { /// assert!(listener1.same_event(&listener2)); /// ``` pub fn same_event(&self, other: &EventListener) -> bool { - ptr::eq::>(&**self.inner(), &**other.inner()) + if let (Some(inner1), Some(inner2)) = (self.inner(), other.inner()) { + return ptr::eq::>(&**inner1, &**inner2); + } + + false } fn listener(self: Pin<&mut Self>) -> Pin<&mut Listener>>> { self.project().listener } - fn inner(&self) -> &Arc> { - &self.listener.event + fn inner(&self) -> Option<&Arc>> { + self.listener.event.as_ref() } } @@ -978,14 +997,18 @@ impl Future for EventListener { pin_project_lite::pin_project! { #[project(!Unpin)] + #[project = ListenerProject] struct Listener>> where B: Unpin, { // The reference to the original event. - event: B, + event: Option, // The inner state of the listener. + // + // This is only ever `None` during initialization. After `listen()` has completed, this + // should be `Some`. #[pin] listener: Option>, } @@ -997,9 +1020,9 @@ pin_project_lite::pin_project! { fn drop(mut this: Pin<&mut Self>) { // If we're being dropped, we need to remove ourself from the list. let this = this.project(); - let inner = (*this.event).borrow(); - - inner.remove(this.listener, true); + if let Some(inner) = this.event { + (*inner).borrow().remove(this.listener, true); + } } } } @@ -1008,14 +1031,6 @@ unsafe impl> + Unpin + Send> Send for Listener unsafe impl> + Unpin + Sync> Sync for Listener {} impl> + Unpin> Listener { - /// Register this listener with the event. - fn insert(self: Pin<&mut Self>) { - let this = self.project(); - let inner = (*this.event).borrow(); - - inner.insert(this.listener); - } - /// Wait until the provided deadline. #[cfg(all(feature = "std", not(target_family = "wasm")))] fn wait_internal(mut self: Pin<&mut Self>, deadline: Option) -> Option { @@ -1059,7 +1074,11 @@ impl> + Unpin> Listener { unparker: TaskRef<'_>, ) -> Option { let mut this = self.project(); - let inner = (*this.event).borrow(); + let inner = (*this + .event + .as_ref() + .expect("must listen() on event listener before waiting")) + .borrow(); // Set the listener's state to `Task`. if let Some(tag) = inner.register(this.listener.as_mut(), unparker).notified() { @@ -1096,17 +1115,24 @@ impl> + Unpin> Listener { /// active listener. fn discard(self: Pin<&mut Self>) -> bool { let this = self.project(); - let inner = (*this.event).borrow(); - inner - .remove(this.listener, false) - .map_or(false, |state| state.is_notified()) + if let Some(inner) = this.event.as_ref() { + (*inner) + .borrow() + .remove(this.listener, false) + .map_or(false, |state| state.is_notified()) + } else { + false + } } /// Poll this listener for a notification. fn poll_internal(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { let mut this = self.project(); - let inner = (*this.event).borrow(); + let inner = match &this.event { + Some(inner) => (*inner).borrow(), + None => panic!(""), + }; // Try to register the listener. match inner diff --git a/src/std.rs b/src/std.rs index 4b9c13e..d0d0144 100644 --- a/src/std.rs +++ b/src/std.rs @@ -70,16 +70,11 @@ impl crate::Inner { } /// Add a new listener to the list. - /// - /// Does nothing is the listener is already registered. pub(crate) fn insert(&self, mut listener: Pin<&mut Option>>) { let mut inner = self.lock(); // SAFETY: We are locked, so we can access the inner `link`. let entry = unsafe { - if listener.is_some() { - return; - } listener.as_mut().set(Some(Listener { link: UnsafeCell::new(Link { state: Cell::new(State::Created),