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

task: add join_set::Builder for configuring JoinSet tasks #4687

Merged
merged 15 commits into from May 30, 2022

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented May 13, 2022

Motivation

The JoinSet API controls the spawning of tasks on a JoinSet --- a
future is passed into one of JoinSet's spawn methods, rather than
having the user spawn the task on their own and pass a JoinHandle into
the JoinSet. This is intended to ensure forwards-compatibility with
potential future changes where a JoinSet might allocate new tasks
differently from how the public tokio::spawn and similar APIs do.
However, it means that JoinSet needs to have a number of API methods
for various ways of spawning a task, including on a LocalSet or via
a runtime Handle.

Currently, there is no way to use a task::Builder to configure a task
prior to spawning it on a JoinSet. This is a deficiency, as it means
that (unstable) task names cannot be added to tasks spawned on a
JoinSet.

See
#4535 (comment)
for details.

Solution

This branch introduces a new JoinSet::build_task() method that returns
a new join_set::Builder type. This type is a variant of
task::Builder which exposes a similar API, but it wraps both a
task::Builder and an &mut JoinSet, and spawns the built task on that
JoinSet.

I felt like this was the best approach from the perspective of the
user-facing API. It does require a bit of boilerplate code to duplicate
the task::Builder interface for the JoinSet::builder type, but I
think the interface presented to the user of the API is much nicer than
an approach where spawn_{whatever}_with_builder methods are added to
the JoinSet type. I discussed this a bit in this comment:
#4535 (comment)

This depends on #4683, which adds spawn_on methods to the
task::Builder API. These are necessary to implement JoinSet's spawn_on
methods using a task builder, but are also probably just generally useful.

@hawkw hawkw requested review from carllerche and Darksonn May 13, 2022 19:47
@hawkw hawkw added A-tokio Area: The main tokio crate M-task Module: tokio/task labels May 13, 2022
@hawkw hawkw mentioned this pull request May 13, 2022
2 tasks
Base automatically changed from eliza/builder-spawn-on to master May 14, 2022 17:44
@hawkw
Copy link
Member Author

hawkw commented May 16, 2022

huh, seems like CI is kinda stuck on this for some reason...

@Darksonn
Copy link
Contributor

Adding a builder instead of adding more and more variations of the different spawn methods seems like a good way to handle it.

tokio/src/task/builder.rs Outdated Show resolved Hide resolved
tokio/src/task/join_set.rs Outdated Show resolved Hide resolved
tokio/src/task/join_set.rs Outdated Show resolved Hide resolved
tokio/src/task/local.rs Outdated Show resolved Hide resolved
/// [runtime handle]: crate::runtime::Handle
/// [`Handle::spawn`]: crate::runtime::Handle::spawn
#[track_caller]
pub fn spawn_on<Fut>(&mut self, future: Fut, handle: &Handle) -> JoinHandle<Fut::Output>
Copy link
Member

Choose a reason for hiding this comment

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

I think this is being pulled in from a different PR, but what about:

Builder::new()
    .handle(handle)
    .spawn(future)

Copy link
Member

Choose a reason for hiding this comment

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

In discord, Alice points out that spawn_local requires a local set not a handle, so ^^ doesn't really work out.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah...there could be a trait of some kind that Handle and LocalSet implement, but that would be a bit of a public API commitment to make, I think...

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

Generally looks fine to me. Couple of thoughts inline.

tokio/src/task/join_set.rs Show resolved Hide resolved
@github-actions github-actions bot added the R-loom Run loom tests on this PR label May 17, 2022
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@github-actions github-actions bot removed the R-loom Run loom tests on this PR label May 17, 2022
hawkw and others added 11 commits May 17, 2022 12:11
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Co-authored-by: Alice Ryhl <alice@ryhl.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
see #4687 (comment)

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw requested review from Darksonn and carllerche May 17, 2022 19:25
tokio/src/task/local.rs Outdated Show resolved Hide resolved
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@Darksonn Darksonn merged commit 2bad98f into master May 30, 2022
@Darksonn Darksonn deleted the eliza/joinset-builder branch May 30, 2022 16:23
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Jun 11, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dependencies | minor | `1.18.2` -> `1.19.1` |
| [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dev-dependencies | minor | `1.18.2` -> `1.19.1` |

---

### Release Notes

<details>
<summary>tokio-rs/tokio</summary>

### [`v1.19.1`](https://github.com/tokio-rs/tokio/releases/tag/tokio-1.19.1)

[Compare Source](tokio-rs/tokio@tokio-1.19.0...tokio-1.19.1)

##### 1.19.1 (June 5, 2022)

This release fixes a bug in `Notified::enable`. ([#&#8203;4747])

[#&#8203;4747]: tokio-rs/tokio#4747

### [`v1.19.0`](https://github.com/tokio-rs/tokio/releases/tag/tokio-1.19.0)

[Compare Source](tokio-rs/tokio@tokio-1.18.2...tokio-1.19.0)

##### 1.19.0 (June 3, 2022)

##### Added

-   runtime: add `is_finished` method for `JoinHandle` and `AbortHandle` ([#&#8203;4709])
-   runtime: make global queue and event polling intervals configurable ([#&#8203;4671])
-   sync: add `Notified::enable` ([#&#8203;4705])
-   sync: add `watch::Sender::send_if_modified` ([#&#8203;4591])
-   sync: add resubscribe method to broadcast::Receiver ([#&#8203;4607])
-   net: add `take_error` to `TcpSocket` and `TcpStream` ([#&#8203;4739])

##### Changed

-   io: refactor out usage of Weak in the io handle ([#&#8203;4656])

##### Fixed

-   macros: avoid starvation in `join!` and `try_join!` ([#&#8203;4624])

##### Documented

-   runtime: clarify semantics of tasks outliving `block_on` ([#&#8203;4729])
-   time: fix example for `MissedTickBehavior::Burst` ([#&#8203;4713])

##### Unstable

-   metrics: correctly update atomics in `IoDriverMetrics` ([#&#8203;4725])
-   metrics: fix compilation with unstable, process, and rt, but without net ([#&#8203;4682])
-   task: add `#[track_caller]` to `JoinSet`/`JoinMap` ([#&#8203;4697])
-   task: add `Builder::{spawn_on, spawn_local_on, spawn_blocking_on}` ([#&#8203;4683])
-   task: add `consume_budget` for cooperative scheduling ([#&#8203;4498])
-   task: add `join_set::Builder` for configuring `JoinSet` tasks ([#&#8203;4687])
-   task: update return value of `JoinSet::join_one` ([#&#8203;4726])

[#&#8203;4498]: tokio-rs/tokio#4498

[#&#8203;4591]: tokio-rs/tokio#4591

[#&#8203;4607]: tokio-rs/tokio#4607

[#&#8203;4624]: tokio-rs/tokio#4624

[#&#8203;4656]: tokio-rs/tokio#4656

[#&#8203;4671]: tokio-rs/tokio#4671

[#&#8203;4682]: tokio-rs/tokio#4682

[#&#8203;4683]: tokio-rs/tokio#4683

[#&#8203;4687]: tokio-rs/tokio#4687

[#&#8203;4697]: tokio-rs/tokio#4697

[#&#8203;4705]: tokio-rs/tokio#4705

[#&#8203;4709]: tokio-rs/tokio#4709

[#&#8203;4713]: tokio-rs/tokio#4713

[#&#8203;4725]: tokio-rs/tokio#4725

[#&#8203;4726]: tokio-rs/tokio#4726

[#&#8203;4729]: tokio-rs/tokio#4729

[#&#8203;4739]: tokio-rs/tokio#4739

</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 these updates again.

---

 - [x] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1394
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
A-tokio Area: The main tokio crate M-task Module: tokio/task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants