sync: introdue `sync::Notify::notified_owned()` #7465

ariaandika · 2025-07-18T23:43:39Z

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

Motivation

The Notify api returns Notified<'_> Future for the non-blocking wait of But having a lifetime makes it hard to use in Future since it requires self referencing lifetime.

In most if not all use cases, even in the example, the Notify will be wrapped in an Arc, so having an owned version will eliminate lifetime requirement.

Other use case mentioned in #7391 is to launch tasks and then notify them at the same time using notify_waiters.

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();

Solution

Introduced new a type NotifiedOwned, a Future that holds Arc instead of shared reference to Notify, eliminating lifetime requirement. Add corresponding Notify::notified_owned. The new method can only be called if self is within an Arc.

Extract the `Notified::poll_notified` and `Notified::drop` logic into a function. Add `NotifiedOwned` and `Notify::notified_owned`.

ADD-SP

Could you write some tests for this new feature?

tokio/src/sync/notify.rs

ariaandika · 2025-07-19T08:48:32Z

I did not know about loom arbitrary_self_types feature is, and considering the error help message, changing notified_owned to take self: Arc<Self> means callers must have the ownership of Arc<Notify>, though callers can just .clone() before calling notified_owned, which in my opinion is slightly inconvenient.

Should i proceed ?

Darksonn · 2025-07-19T13:03:33Z

Do not enable the arbitrary_self_types feature. We can skip using the loom Arc, but anything else in sync should come from Loom.

Darksonn · 2025-07-19T13:06:44Z

And yes you should take self: Arc<Self> instead of forcing a clone of the Arc on the end-user even if one is not needed.

ADD-SP

Overall looks good to me.

tokio/src/sync/notify.rs

ADD-SP

Implementation looks good to me, could you add some tests in tokio/tests/sync_notify.rs?

ariaandika · 2025-07-22T07:00:44Z

The underlying logic is exactly the same, is additional tests necessary ?

Also, is it okay if i just copied from existing tests of Notfied<'_> ? I don't know special test case for an owned version.

ADD-SP

The underlying logic is exactly the same, is additional tests necessary ?

I think additional tests are necessary as we may (not likely) to change the implementation in the future, so we might have different implementation.

In principle, tests under tokio/tests are black box tests, which should not care about the implementation details too much.

I don't know special test case for an owned version.

I think the only special case is that make sure the OwnedNotified is 'static because this is the whole point of this feature.

I guess this is just my personal flavor, it should be easy to eyeball if a type is 'static or not, seems we don't need a test to cover this.

is it okay if i just copied from existing tests of Notfied<'_> ?

Apparently, we don't want to copy such a large test code, Let me think a little bit. Feel free to share you through about this in here.

ariaandika · 2025-07-22T13:47:35Z

Well, looking on the other API, Semaphore, the tests between sync_semaphore.rs and sync_semaphore_owned.rs is pretty similar, one just swap the Semaphore::acquire and Semaphore::acquire_owned.

ADD-SP · 2025-07-23T08:00:44Z

Well, looking on the other API, Semaphore, the tests between sync_semaphore.rs and sync_semaphore_owned.rs is pretty similar, one just swap the Semaphore::acquire and Semaphore::acquire_owned.

I didn't come up with a better idea, I think it is ok to follow the approach of sync_semaphore_owned.rs.

ADD-SP

Looks good to me, thanks!

ariaandika · 2025-07-24T12:51:36Z

You're welcome, glad the feature can be added.

feat: introduce NotifiedOwned

516dc24

Extract the `Notified::poll_notified` and `Notified::drop` logic into a function. Add `NotifiedOwned` and `Notify::notified_owned`.

github-actions bot added the R-loom-sync Run loom sync tests on this PR label Jul 18, 2025

ADD-SP added C-enhancement Category: A PR with an enhancement or bugfix. A-tokio Area: The main tokio crate M-sync Module: tokio/sync labels Jul 19, 2025

ADD-SP reviewed Jul 19, 2025

View reviewed changes

tokio/src/sync/notify.rs Outdated Show resolved Hide resolved

tokio/src/sync/notify.rs Outdated Show resolved Hide resolved

tokio/src/sync/notify.rs Outdated Show resolved Hide resolved

tokio/src/sync/notify.rs Outdated Show resolved Hide resolved

ADD-SP reviewed Jul 19, 2025

View reviewed changes

tokio/src/sync/notify.rs Outdated Show resolved Hide resolved

ADD-SP reviewed Jul 19, 2025

View reviewed changes

tokio/src/sync/notify.rs Show resolved Hide resolved

ADD-SP changed the title ~~feat: introduce NotifiedOwned~~ sync: introdue sync::Notify::notified_own() Jul 19, 2025

ADD-SP changed the title ~~sync: introdue sync::Notify::notified_own()~~ sync: introdue sync::Notify::notified_owned() Jul 19, 2025

rename to OwnedNotified

54b43b6

ariaandika and others added 5 commits July 19, 2025 20:41

revert to using std Arc

e4e9c8b

docs: notified_owned example

a188280

convert projection type alias to struct

02ac88a

Merge branch 'master' into aria/notified_owned

7dee118

Merge branch 'master' into aria/notified_owned

c4d86a1

ADD-SP reviewed Jul 21, 2025

View reviewed changes

tokio/src/sync/notify.rs Outdated Show resolved Hide resolved

tokio/src/sync/notify.rs Outdated Show resolved Hide resolved

refine NotifiedOwned example

48ae648

ADD-SP reviewed Jul 22, 2025

View reviewed changes

add tests for OwnedNotify

8e0ac9e

ADD-SP approved these changes Jul 24, 2025

View reviewed changes

Merge branch 'master' into aria/notified_owned

74216dd

ADD-SP merged commit d545aa2 into tokio-rs:master Jul 26, 2025
86 checks passed

ADD-SP mentioned this pull request Jul 26, 2025

chore: prepare to release v1.47.0 #7482

Merged

ariaandika deleted the aria/notified_owned branch July 27, 2025 10:13

ADD-SP mentioned this pull request Jul 30, 2025

Owned version of Notified<'_> #7231

Closed

Uh oh!

sync: introdue sync::Notify::notified_owned() #7465

sync: introdue sync::Notify::notified_owned() #7465

Uh oh!

Conversation

ariaandika commented Jul 18, 2025

Motivation

Solution

Uh oh!

ADD-SP left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ariaandika commented Jul 19, 2025

Uh oh!

Darksonn commented Jul 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Darksonn commented Jul 19, 2025

Uh oh!

ADD-SP left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ADD-SP left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariaandika commented Jul 22, 2025

Uh oh!

ADD-SP left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariaandika commented Jul 22, 2025

Uh oh!

ADD-SP commented Jul 23, 2025

Uh oh!

ADD-SP left a comment

Choose a reason for hiding this comment

Uh oh!

ariaandika commented Jul 24, 2025

Uh oh!

Uh oh!

Uh oh!

sync: introdue `sync::Notify::notified_owned()` #7465

sync: introdue `sync::Notify::notified_owned()` #7465

Darksonn commented Jul 19, 2025 •

edited

Loading

ADD-SP left a comment •

edited

Loading

ADD-SP left a comment •

edited

Loading