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

util: add task builder API #7180

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AlphaKeks
Copy link

Motivation

This PR implements a builder API for tokio_util::task::JoinMap as discussed in #7179.

Solution

JoinMap now has a public build_task method that returns a JoinMapBuilder. JoinMapBuilder lets you configure a name for the task and then spawn it on the JoinMap you originally got the builder from.

Because JoinMap contains a JoinSet and join_set::Builder borrows its JoinSet mutably, join_map::Builder cannot contain both a &mut JoinMap (which is required for inserting the task) and a join_set::Builder (which would borrow JoinMap::tasks). I see 2 solutions to this:

  1. refactor JoinMap::insert in such a way that it is a free function instead of a method, and then make Builder contain the individual parts of JoinMap that are required
  2. make Builder store all the same state that join_set::Builder (or task::Builder for that matter) stores

I chose to go with option 2, although in the future it may be better to switch to option 1, if task::Builder ever extends its API with callbacks and such.

The tokio_util::task::join_map module is currently also not public, so instead of mirroring join_map::Builder (where the join_map module is public), the Builder struct is re-exported as JoinMapBuilder. I'm not convinced this is the way to go, but making the module public would also require documenting it (which I am willing to do if desired).

@AlphaKeks AlphaKeks marked this pull request as draft February 26, 2025 18:25
@maminrayej maminrayej added the A-tokio-util Area: The tokio-util crate label Feb 27, 2025
@AlphaKeks AlphaKeks force-pushed the feat/joinmap-builder branch from 5ec78e3 to 8c50680 Compare February 27, 2025 16:32

Verified

This commit was signed with the committer’s verified signature.
AlphaKeks AlphaKeks
@AlphaKeks AlphaKeks force-pushed the feat/joinmap-builder branch from 8c50680 to 94b37de Compare February 27, 2025 16:38
@AlphaKeks
Copy link
Author

In order for this to work, join_map::Builder needs access to tokio::task::Builder, which is gated behind tokio's tracing feature flag and --cfg tokio_unstable. So what tokio-util needs to do is enable tokio/tracing but only if --cfg tokio_unstable is also present, which I don't think Cargo is able to do?

@mox692
Copy link
Member

mox692 commented Mar 3, 2025

Cargo cannot conditionally enable tokio/tracing based on --cfg tokio_unstable, but since tokio's tracing feature itself is always gated by --cfg tokio_unstable, I guess it doesn't really matter?

@AlphaKeks
Copy link
Author

It matters because tokio-util already has a tracing feature (implicitly by virtue of its tracing dependency being optional). I guess we could introduce a separate feature flag that specifically enables tokio/tracing and call it something other than tracing, but I'm not sure how I feel about that.

@mox692
Copy link
Member

mox692 commented Mar 6, 2025

Okay I see.

I guess we could introduce a separate feature flag that specifically enables tokio/tracing

Or, we might want to wait for task::Builder to stabilize ...

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants