Skip to content

Conversation

@bnoordhuis
Copy link
Contributor

@bnoordhuis bnoordhuis commented Oct 1, 2021

Allows using tokio::task::spawn_local() without further setup.

Reapplies 33f0a1f, reverted for making tokio::task::block_in_place()
panic with the multi-threaded variant of #[tokio::test].

This commit creates the LocalSet only for the single-threaded runtime.
Still a significant Quality-of-Life improvement, IMO.

edit: refs #4142

Allows using tokio::task::spawn_local() without further setup.

Reapplies 33f0a1f, reverted for making tokio::task::block_in_place()
panic with the multi-threaded variant of #[tokio::test].

This commit creates the LocalSet only for the single-threaded runtime.
Still a significant Quality-of-Life improvement, IMO.
@Darksonn Darksonn added A-tokio-macros Area: The tokio-macros crate M-runtime Module: tokio/runtime labels Oct 2, 2021
@bnoordhuis
Copy link
Contributor Author

Feedback, por favor?

@Darksonn
Copy link
Contributor

Darksonn commented Oct 7, 2021

I haven't forgotten you, but I only look at non-simple things once a week, and I didn't get around to this PR this week.

@bnoordhuis
Copy link
Contributor Author

Quick recap: 33f0a1f was reverted because the newly introduced LocalSet interacts badly with block_in_place().

// This probably means we are on the basic_scheduler or in a
// LocalSet, where it is _not_ okay to block.
panic!("can call blocking only when running on the multi-threaded runtime");

Copy link
Contributor

@Noah-Kennedy Noah-Kennedy left a comment

Choose a reason for hiding this comment

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

I've tested it with #[tokio::main] locally, and that works as well. We should get a couple more reviewers on this though; I am not familiar enough with some of the implementation details for localset to know if this leads to any changes in performance, and others may have opinions on this.

@Darksonn
Copy link
Contributor

Darksonn commented Oct 8, 2021

One of the concerns we had with the original PR is that it gives #[tokio::main] and #[tokio::test] an important place where their behavior differs, namely that only one of them allows use of spawn_local.

@carllerche
Copy link
Member

One of the concerns we had with the original PR is that it gives #[tokio::main] and #[tokio::test] an important place where their behavior differs, namely that only one of them allows use of spawn_local.

We could disable spawn_local in #[tokio::test] unless explicitly asked for?

@Darksonn
Copy link
Contributor

Right, having a local_set option on the macro is one of the options we considered in the previous discussion.

@bnoordhuis
Copy link
Contributor Author

I'm retracting this. It turned out to be (a lot) quicker to just make appropriate changes to our code base than trying to upstream this change.

@bnoordhuis bnoordhuis closed this Oct 25, 2021
@bnoordhuis bnoordhuis deleted the macro-localset-take2 branch October 25, 2021 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-tokio-macros Area: The tokio-macros crate M-runtime Module: tokio/runtime

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants