macros: run current_thread inside LocalSet

[bnoordhuis]

Make the "current_thread" flavor run the function body in a LocalSet so
that tokio::task::spawn_local() just works.

It's very helpful when you have hundreds of tests and don't want to
repeat the same LocalSet::new().run_until(...) stanza every time.

---

Aside: I ran the rustfmt --check --edition 2018 $(find . -name '*.rs' -print) command suggested in CONTRIBUTING.md but it spits out a lot of suggestions unrelated to my change. That's with rustfmt 1.4.37-stable (a178d03 2021-07-26).

[hawkw]

The macro'How would you like to pay for your trip?s documentation should probably mention this behavior? Otherwise, though, this LGTM!

[bnoordhuis]

Done. Thanks for the review!

[bnoordhuis]

@hawkw Is there anything else I should do? Do I need to squash/rebase the commits?

[Darksonn]

We squash on merge, so that's not necessary.

[hawkw]

@hawkw Is there anything else I should do? Do I need to squash/rebase the commits?

I was just hoping to get another maintainer's opinion before going ahead and merging this one. :)

[Darksonn]

One thing I wonder is that maybe it would be better to make LocalSet a new option on the builder instead of repurposing the current_thread flavor for it.

[bnoordhuis]

As an unscientific data point: I did an informal "spot the bug" poll and no one (n=3) realized spawn_local() won't work without a LocalSet. Making the macros do the right thing automatically likely avoids much gnashing of teeth.

Maybe tokio::runtime::Builder::new_current_thread() should work the same way but that's probably more controversial?

[Darksonn]

The tokio::runtime::Builder::new_current_thread() method cannot possibly work the same way because the Runtime object is Send, but a LocalSet is not.

[Darksonn]

It is worth adding that even with the LocalSet, calling tokio::task::spawn_local() won't work inside a task spawned with tokio::spawn() as that call takes you outside the LocalSet, so you have to understand the LocalSet even if we add the LocalSet to the macro.

[bnoordhuis]

7 days have passed. Is the jury verdict in?

[carllerche]

It is worth adding that even with the LocalSet, calling tokio::task::spawn_local() won't work inside a task spawned with tokio::spawn() as that call takes you outside the LocalSet, so you have to understand the LocalSet even if we add the LocalSet to the macro.

It should be possible to make this work though, no?

[Darksonn]

We discussed this on Discord. Here's a summary:

1. Adding a LocalSet but only for the current_thread runtime is problematic because it means that spawn_local behaves differently in tests and in main.
2. Making spawn_local work inside spawn doesn't really fix the issue because you still have this difference when the LocalSet is used with a multi-threaded runtime, so the place where it doesn't work is even more of a special case.
3. Adding it as a new option or flavor does not help with the issue of discoverability.

I think we should change this to always adding a LocalSet, even for a multi_thread runtime. Then the behavior of spawn_local is "works in main and inside spawn_local calls, but not in tokio::spawn calls" for both main and test.

[carllerche]

Discussion happened in this Discord thread (hopefully the link works)

[bnoordhuis]

I'll look into making that work.

For my understanding w.r.t. to point 1: is the fear that people are going to get confused by #[tokio::main] (multi-thread) vs. #[tokio::test(flavor = "current_thread")] working differently?

#[tokio::main(flavor = "current_thread")] works like #[tokio::test(flavor = "current_thread")] with this PR. I can probably add a test to that effect, if so desired.

[Darksonn]

Regarding point 1, the issue is that tests use the current_thread runtime by default, so they would get a LocalSet by default, whereas main would not.

[bnoordhuis]

I've been going over the code and IMO there are several ways of skinning this "implicit LocalSet" cat. Can a maintainer provide some feedback on the best way forward?

My initial thinking was to:

1. add a task::local::Context to runtime::Handle
2. have runtime::EnterGuard activate and deactivate it (so LocalSet::run_until can still override it)

But that's probably less efficient than it could be because now more work needs to be done, plus the context must be thread-local.

[Darksonn]

The runtime object itself is not going to have an associated LocalSet. If we do this, it would only apply to people who use the macro.

[bnoordhuis]

I suppose I got confused by the discussion above but restricting it to just the macros certainly makes it easier to implement. PR updated, PTAL.

[bnoordhuis]

Ping - please review?

[Darksonn]

I'll put it on my todolist, but I'm too tired to review anything today.

[bnoordhuis]

Friendly ping.

[Darksonn]

This is being reverted in #4142.