Skip to content

Conversation

@dignifiedquire
Copy link
Contributor

Allows using unstable tokio feature when enabled to name tasks spawned.

Allows using unstable tokio feature when enabled to name tasks spawned.
@github-actions
Copy link

github-actions bot commented Dec 1, 2025

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/n0-future/pr/20/docs/n0_watcher/

Last updated: 2025-12-01T21:39:52Z

Copy link
Member

@matheus23 matheus23 left a comment

Choose a reason for hiding this comment

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

Love this.
Would love to eventually remove other spawn variants in favor of only having named ones, in an effort to "standardize on a style" in a way with n0-future.

tokio::task::Builder::new()
.name(name)
.spawn(future)
.expect("doesn't fails")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.expect("doesn't fails")
.expect("doesn't fail")

Copy link
Member

Choose a reason for hiding this comment

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

Maybe nice if it says why it doesn't fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because I looked at the implementation 🤷 not sure what else to say

Copy link
Member

Choose a reason for hiding this comment

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

what the...

Okay maybe sth like "impl doesn't fail".

@flub
Copy link
Contributor

flub commented Dec 2, 2025

I'm going to be dissenting. I think we should adopt the task::Builder approach from tokio unstable. In particular because that allows us to add other methods, and I want to add .instrument():

let task = n0_future::task::Builder::new()
    .name("actor")
    .instrument()  // by default .span() is set to info_span(name)
    .span(tracing::info_span!("my-actor", field="value"))
    .spawn(n0_future::future::ready(true));

// or a shorthand for the same without customising the span
let task = n0_futrue::task::Builder::with_name("actor")
    .instrument()
    .spawn(n0_future::future::ready(true));

@dignifiedquire
Copy link
Contributor Author

I'm going to be dissenting. I think we should adopt the task::Builder approach from tokio unstable.

Compared to task::spawn I just don't think we would end up using it automatically, already the addition of with_name makes it a mouth full, so I think we would just see many tasks without the name, rather than making it easy to adopt giving a name by default

@flub
Copy link
Contributor

flub commented Dec 2, 2025

I'm going to be dissenting. I think we should adopt the task::Builder approach from tokio unstable.

Compared to task::spawn I just don't think we would end up using it automatically, already the addition of with_name makes it a mouth full, so I think we would just see many tasks without the name, rather than making it easy to adopt giving a name by default

We already require folks to remember to use async move { ... }.instrument(info_span!("...")). I think we can manage to change our convention.

@dignifiedquire
Copy link
Contributor Author

We already require folks to remember to use async move { ... }.instrument(info_span!("...")). I think we can manage to change our convention.

fair, that .instrument is quite painful, so the builder would actually be an improvement

@matheus23
Copy link
Member

I'm thinking of cutting another release of this given the recent PRs merged. Should we attempt to get this into the release as well?

@dignifiedquire
Copy link
Contributor Author

I thought more about the suggestion from @flub, so I would like to hold off on this, and experiment with a builder approach some more.

Copy link
Member

@matheus23 matheus23 left a comment

Choose a reason for hiding this comment

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

Let's experiment with a builder-like approach first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants