sync: introdue sync::Notify::notified_owned()

[ariaandika]

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.

[ADD-SP]

Could you write some tests for this new feature?

[ariaandika]

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]

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]

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.

[ADD-SP]

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

[ariaandika]

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]

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]

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]

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