Skip to content
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

Impl tokio_util::sync::WaitForCancellationFutureOwned #5153

Merged
merged 19 commits into from
Nov 21, 2022
Merged

Impl tokio_util::sync::WaitForCancellationFutureOwned #5153

merged 19 commits into from
Nov 21, 2022

Conversation

NobodyXu
Copy link
Contributor

@NobodyXu NobodyXu commented Nov 1, 2022

Signed-off-by: Jiahao XU Jiahao_XU@outlook.com

Motivation

tokio_util::sync::WaitForCancellationFuture takes a reference to the CancellationToken, which is hard to use in structs where you don't want a lifetime.

Solution

tokio_util::sync::WaitForCancellationFutureOwned provides an alternative where CancellationToken is taken by value so that WaitForCancellationFutureOwned does not contain any lifetime (but self-referenced) and can be used without worrying about lifetime.

Alternatives

User use ouroboros to create self-reference struct, but it is not zero-cost.
Other self-reference crates that are zero-cost are experimental (e.g. self-reference).

Or the user can do:

let boxed_fut: Pin<Box<(dyn Future<Output = ()> + Send)>> = Box::pin(async move {
    cancellation_token.cancalled()
});

which isn't zero-cost since it requires one extra allocation and also a pointer to vtable for the trait Future.

User could also uses a SmallBox with pre-allocated space on stack to store the future, though it's more complicated and not zero-cost either (extra pointer to vtable and it might incur heap allocation if the pre-allocated space on stack is too small).

Fixes: #4466

@github-actions github-actions bot added the R-loom Run loom tests on this PR label Nov 1, 2022
@Darksonn Darksonn added A-tokio-util Area: The tokio-util crate M-sync Module: tokio/sync labels Nov 1, 2022
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

I'm rather uncomfortable with adding an implementation that looks like this.

tokio-util/src/sync/cancellation_token.rs Outdated Show resolved Hide resolved
tokio-util/src/sync/cancellation_token.rs Outdated Show resolved Hide resolved
tokio-util/src/sync/cancellation_token.rs Outdated Show resolved Hide resolved
NobodyXu and others added 17 commits November 22, 2022 01:01
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
for field `future`.

Since `Notified` does not implemented `Unpin`, it must not be moved but
`do_drop` and `poll` moves it.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
…o_drop_future`

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
to cast `Notified` to `Notified<'a>` where `'a` is the lifetime of
`&'a mut self`.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Co-authored-by: Alice Ryhl <aliceryhl@google.com>
…fied`

and remove the `Option<T>` wrapper.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Co-authored-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
tokio-util/src/sync/cancellation_token.rs Outdated Show resolved Hide resolved
tokio-util/src/sync/cancellation_token.rs Outdated Show resolved Hide resolved
tokio-util/src/sync/cancellation_token.rs Outdated Show resolved Hide resolved
tokio-util/src/sync/cancellation_token.rs Outdated Show resolved Hide resolved
tokio-util/src/sync/cancellation_token.rs Outdated Show resolved Hide resolved
Co-authored-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

@Darksonn Darksonn enabled auto-merge (squash) November 21, 2022 14:35
@Darksonn Darksonn merged commit 304b515 into tokio-rs:master Nov 21, 2022
@NobodyXu NobodyXu deleted the impl-WaitForCancellationFutureOwned branch November 22, 2022 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio-util Area: The tokio-util crate M-sync Module: tokio/sync R-loom Run loom tests on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an owned version of WaitForCancellationFuture
2 participants