Skip to content

Commit

Permalink
bugfix: Disable early-out notify optimization
Browse files Browse the repository at this point in the history
The notify() function contains two optimizations:

- If the `inner` field is not yet initialized (i.e. no listeners have been
  created yet), it returns `0` from the function early.
- If the atomic `notified` field indicates that the current notification would
  do nothing, it returns `0` early as well.

`loom` testing has exposed race conditions in these optimizations that can cause
notifications to be missed, leading to deadlocks. This commit removes these
optimizations in the hope of preventing these deadlocks. Ideally we can restore
them in the future.

Closes #138
cc #130 and #133

Signed-off-by: John Nunley <dev@notgull.net>
  • Loading branch information
notgull committed May 28, 2024
1 parent fdbe437 commit 85e3474
Show file tree
Hide file tree
Showing 3 changed files with 3 additions and 26 deletions.
19 changes: 3 additions & 16 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ use sync::Arc;
#[cfg(not(loom))]
use sync::WithMut;

use notify::{Internal, NotificationPrivate};
use notify::NotificationPrivate;
pub use notify::{IntoNotification, Notification};

/// Inner state of [`Event`].
Expand Down Expand Up @@ -437,21 +437,8 @@ impl<T> Event<T> {
// Make sure the notification comes after whatever triggered it.
notify.fence(notify::Internal::new());

if let Some(inner) = self.try_inner() {
let limit = if notify.is_additional(Internal::new()) {
usize::MAX
} else {
notify.count(Internal::new())
};

// Notify if there is at least one unnotified listener and the number of notified
// listeners is less than `limit`.
if inner.needs_notification(limit) {
return inner.notify(notify);
}
}

0
let inner = unsafe { &*self.inner() };
inner.notify(notify)
}

/// Return a reference to the inner state if it has been initialized.
Expand Down
5 changes: 0 additions & 5 deletions src/no_std.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,6 @@ impl<T> crate::Inner<T> {
drop(self.try_lock());
}

pub(crate) fn needs_notification(&self, _limit: usize) -> bool {
// TODO: Figure out a stable way to do this optimization.
true
}

/// Add a new listener to the list.
///
/// Does nothing if the list is already registered.
Expand Down
5 changes: 0 additions & 5 deletions src/std.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,6 @@ impl<T> crate::Inner<T> {
}
}

/// Whether or not this number of listeners would lead to a notification.
pub(crate) fn needs_notification(&self, limit: usize) -> bool {
self.notified.load(Ordering::Acquire) < limit
}

/// Add a new listener to the list.
pub(crate) fn insert(&self, mut listener: Pin<&mut Option<Listener<T>>>) {
let mut inner = self.lock();
Expand Down

0 comments on commit 85e3474

Please sign in to comment.