Skip to content

Introduce notified_owned for thread-safe notification futures. #7391

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.

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

Closed
wants to merge 2 commits into from

Conversation

Sagebati
Copy link

@Sagebati Sagebati commented Jun 6, 2025

Allows returning an owned notification future that can be moved across threads. Inspired by #7231

Motivation

A common pattern when using Notify is to launch tasks and then notify them at the same time using notify_waiters. In the current API it can be quite challenging because you don't know if the task got the time to register before notifying. A good way of doing it would be to register then launch the task. Problem to do that you would need the result future from notified() (aka Notified<'_>) to be Send.

Ex:

let n = Arc::new(Notify::new()); // We can't use notified here because we need to send it.

tokio::spawn({
  let n = n.clone();
  async move {
    // if we need the code to be correct we need to register the waiter here
    tokio::sleep(10).await;
    n.notified().await;
    println!("event received");
}})

n.notify_waiters();

Ex: Workaround using a oneshot and Shared<_>

let (sx, rx) = oneshot();
let rx = rx.shared();

for _ in 0..10 {
  let rx = rx.clone();
  tokio::spawn(async move { rx.clone().await; println!("event received"); });
}

sx.send(());

Solution

Added a new method notified_owned that returns Send Notified<'static>.

Allows returning an owned notification future that can be moved across threads. Adds `NotifyRef` enum to manage borrowed and owned references internally.
@github-actions github-actions bot added the R-loom-sync Run loom sync tests on this PR label Jun 6, 2025
Allows returning an owned notification future that can be moved across threads. Adds `NotifyRef` enum to manage borrowed and owned references internally.
Copy link
Member

@ADD-SP ADD-SP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think #7231 is a reasonable use case , and this issue is similar to #1998.

Could you add some tests against this new feature? Also we'd better aligning the naming pattern of tokio::sync::OwnedSemaphorePermit instead of Notified<'static>, how about OwnedNotified?

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-sync Module: tokio/sync labels Jun 7, 2025
Comment on lines +378 to +382
#[derive(Debug)]
enum NotifyRef<'a> {
Owned(Arc<Notify>),
Ref(&'a Notify),
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we want more future types, but if we do, it has to be a new type. An enum pays a cost for everyone using notified, and we don't want to go that route.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair, I didn't take into account the cost of the access, any workaround for not repeating the implementation between Notified and NotifiedOwned ? I suppose using an union is cursed, maybe a macro ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's avoid unions or macros. How about extracting common logic into shared functions?

@ariaandika
Copy link
Contributor

Hi, any update on this ? if you’re no longer working on it, id be happy to take it over.

@Sagebati
Copy link
Author

Sagebati commented Jul 18, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-sync Module: tokio/sync R-loom-sync Run loom sync tests on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants