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

Add AbortRegistration::handle #2712

Merged
merged 2 commits into from Mar 10, 2023
Merged

Add AbortRegistration::handle #2712

merged 2 commits into from Mar 10, 2023

Conversation

ZhennanWu
Copy link
Contributor

@ZhennanWu ZhennanWu commented Feb 28, 2023

Allow to create AbortHandle from AbortRegistration. The implementation is obvious. Since AbortHandle is already Clone, this API should not create too much problem.

Motivation

The intended use case is pretty niche. I'll try to explain. There is this single scenario where we have access to AbortRegistration, but can't have access to AbortHandle.

I'm trying to create a zero-cost abstraction over the different async executors. One major concern is to ensure the uniformity of cancellation propagation between different async executor backends. When I try to implement a cancel-on-drop semantic, due to limitations of async-std, it requires an Abortable wrapper to ensure the cancellation propagation. (as others have also found out).

Users may well combine a cancel-on-drop spawn with an Abortable, which results in two consecutive Abortable wrappers in async-std, which may be flattened into one. For zero-cost abstraction, a more specific "cancel-on-drop-with-Abortable" semantic is needed to avoid the extra cost on async-std. Which, unfortunately, has no other signature than the following.

pub trait SpawnAbortableScoped: Send + Sync + 'static {
    type JoinHandle<Output: Send + 'static>: JoinHandle<Output>;
    fn spawn_abortable_scoped<F, T: Send + 'static>(
        &self,
        f: F,
        reg: futures::future::AbortRegistration,
    ) -> Self::JoinHandle<Result<T, futures::future::Aborted>>
    where
        F: Future<Output = T> + Send + 'static;
}

We cannot add an AbortHandle parameter since other executors do not need AbortHandle to propagate the cancellation. Adding one violates zero-cost abstraction. However, on async-std we do need an AbortHandle for cancellation propagation. This prompts the need for this API.

@taiki-e
Copy link
Member

taiki-e commented Mar 5, 2023

Thanks for the PR. This looks good to me.

I thought new_* might be a little clearer on the name, but I have no strong opinion.

@ZhennanWu
Copy link
Contributor Author

ZhennanWu commented Mar 5, 2023

I thought new_* might be a little clearer on the name

The new_handle name under AbortRegistration may be a little confusing. How about naming it after StopSource::token(), so we name it as AbortRegistration::handle()?

Edit: I mean by "new_handle" it kind of suggests the creation of a entirely unrelated "new" abortable, especially when there is "AbortHandle::new_pair" around

@ZhennanWu
Copy link
Contributor Author

@taiki-e I've changed the name to AbortRegistration::handle for now. After some implementation using this new API, now I think AbortRegistration::handle is indeed a better choice. Just like the StopSource::token()

Compare

let handle = abort_reg.create_handle()

to

let handle = abort_reg.new_handle()

to

let handle = abort_reg.handle()

The last one express the fixed dependency of handle on the registration, and its shared reference nature.

@taiki-e taiki-e changed the title add AbortRegistration::create_handle Add AbortRegistration::handle Mar 10, 2023
@taiki-e taiki-e merged commit bc32e4b into rust-lang:master Mar 10, 2023
23 checks passed
@taiki-e taiki-e added A-future Area: futures::future 0.3-backport: pending The maintainer accepted to backport this to the 0.3 branch, but backport has not been done yet. labels Mar 10, 2023
taiki-e pushed a commit that referenced this pull request Mar 11, 2023
Co-authored-by: ZhennanWu <znjameswu@gmail.com>
@taiki-e taiki-e mentioned this pull request Mar 11, 2023
@taiki-e taiki-e added 0.3-backport: completed and removed 0.3-backport: pending The maintainer accepted to backport this to the 0.3 branch, but backport has not been done yet. labels Mar 11, 2023
taiki-e pushed a commit that referenced this pull request Mar 11, 2023
Co-authored-by: ZhennanWu <znjameswu@gmail.com>
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Mar 15, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [futures](https://rust-lang.github.io/futures-rs) ([source](https://github.com/rust-lang/futures-rs)) | dependencies | patch | `0.3.26` -> `0.3.27` |

---

### Release Notes

<details>
<summary>rust-lang/futures-rs</summary>

### [`v0.3.27`](https://github.com/rust-lang/futures-rs/blob/HEAD/CHANGELOG.md#&#8203;0327---2023-03-11)

[Compare Source](rust-lang/futures-rs@0.3.26...0.3.27)

-   Add `TryFlattenUnordered` ([#&#8203;2577](rust-lang/futures-rs#2577), [#&#8203;2590](rust-lang/futures-rs#2590), [#&#8203;2606](rust-lang/futures-rs#2606), [#&#8203;2607](rust-lang/futures-rs#2607))
-   Add `AbortHandle::is_aborted` ([#&#8203;2710](rust-lang/futures-rs#2710))
-   Add `AbortRegistration::handle` ([#&#8203;2712](rust-lang/futures-rs#2712))
-   Make `BiLock` strict-provenance compatible ([#&#8203;2716](rust-lang/futures-rs#2716))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS42LjAiLCJ1cGRhdGVkSW5WZXIiOiIzNS42LjAifQ==-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1816
Reviewed-by: crapStone <crapstone@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants