Skip to content

add task::LocalSet API for running !Send futures #1733

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 76 commits into from
Nov 27, 2019
Merged

add task::LocalSet API for running !Send futures #1733

merged 76 commits into from
Nov 27, 2019

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Nov 5, 2019 •
edited
Loading

Motivation

In earlier versions of tokio, the current_thread::Runtime type could
be used to run !Send futures. However, PR #1716 merged the
current-thread and threadpool runtimes into a single type, which can no
longer run !Send futures. There is still a need in some cases to
support futures that don't implement Send, and the tokio-compat
crate requires this in order to provide APIs that existed in tokio
0.1.

Solution

This branch implements the API described by @carllerche in
#1716 (comment). It
adds a new LocalSet type and spawn_local function to tokio::task.
The LocalSet type is used to group together a set of tasks which must
run on the same thread and don't implement Send. These are available
when a new "rt-util" feature flag is enabled.

Currently, the local task set is run by passing it a reference to a
Runtime and a future to block_on. In the future, we may also want
to investigate allowing spawned futures to construct their own local
task sets, which would be executed on the worker that the future is
executing on.

In order to implement the new API, I've made some internal changes to
the task module and Schedule trait to support scheduling both Send
and !Send futures.

@hawkw hawkw added the C-enhancement Category: A PR with an enhancement or bugfix. label Nov 5, 2019
@hawkw hawkw self-assigned this Nov 5, 2019
@hawkw
Copy link
Member Author

hawkw commented Nov 5, 2019

I would love any feedback on the API surface of the new local module — if there's anything we can do to make it more ergonomic to use, that would be great. In particular, I wasn't really sure if we would prefer the free function for spawning on the local task set to be used as tokio::spawn_local, or as tokio::local::spawn. I could go either way...

jonhoo
jonhoo reviewed Nov 6, 2019
View reviewed changes
jonhoo
jonhoo reviewed Nov 6, 2019
View reviewed changes
jonhoo
jonhoo reviewed Nov 6, 2019
View reviewed changes
jonhoo
jonhoo reviewed Nov 6, 2019
View reviewed changes
hawkw added 19 commits November 6, 2019 10:33
@hawkw
first pass on local task set
7eee2ec 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
it works!
366ecbd 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
change API to work with multiple block ons
82857dd 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
shutdown on drop
edd8ca4 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
use marker types to indicate whether a task is Send
2e6cf55 
This adds a marker type to the `Task` struct and `Schedule` trait
indicating whether or not the task is `Send`, and changes `Task` to only
implement `Send` when the `Send` marker is present. Schedulers must
indicate whether they are capable of scheduling `Send` or `!Send` tasks.

This solution is singnificantly better than the previous one of using an
unsafe wrapper to make `!Send` tasks implement `Send`, which was
necessary due to the bounds on the `Task` struct. Although the previous
solution was correct, since the `!Send` tasks were only ever scheduled
locally, the compiler could no longer verify that this was the case.
Now, the typesystem actually _helps_ us ensure that schedulers that can
only schedule `Send` tasks will never schedule `!Send` tasks.

Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
add docs
c1e4e4a 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
re-export local mod from root, feature flag
968eea5 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
placate clippy
983b6b7 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
fix doctest
fb99e68 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
*actually* fix doctest
d6ff66e 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
fix wrong assertions
bc93168 
These assertions asserted that the scheduler was current in all calls
to its methods. However, the `TaskSet::spawn_local` method allows
scheduling tasks to be run on the `TaskSet` when it is _not_ currently
running. Therefore, these assertions are wrong.

This test moves the assertion to the _right_ place: when the scheduler
actually _runs_ scheduled tasks.

Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
fix missing pin-project without default features
17c7f7e 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
fix unused code when local is disabled
78d41b8 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
add send markers to debug output
057f415 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
fdsgkhsdfhjkgljkfdg;jklagdsf
f00f099 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
fix more feature-flaggy sadness
157e2e2 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
rename marker types
2a88b5f 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
make markers void structs
eef8727 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
remove duplicate file
1c0cea1 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw hawkw force-pushed the eliza/local-spawn branch from df978a8 to 1c0cea1 Compare November 6, 2019 18:45
@hawkw
Copy link
Member Author

hawkw commented Nov 14, 2019

@carllerche BTW, the local task set now requires the "rt-full" feature flag. this is because it now requires the "sync" feature (due to the use of AtomicWaker), which I didn't think "rt-core" should require, and we didn't want to add a separate feature for local. I think that ideally, it would be possible to enable just the single-threaded runtime + LocalSet without also enabling the threadpool runtime, but I think that can wait until we give all the feature flags a pass.

Does that seem right to you?

@hawkw hawkw mentioned this pull request Nov 15, 2019
Closed
carllerche
carllerche reviewed Nov 15, 2019
View reviewed changes
carllerche
carllerche reviewed Nov 15, 2019
View reviewed changes
carllerche
carllerche reviewed Nov 15, 2019
View reviewed changes
tokio/src/task/local.rs
}
}

fn with<F>(&self, f: impl FnOnce() -> F) -> F {
Copy link
Member

Choose a reason for hiding this comment

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

What happens when you nest with calls? Looks like bad things might happen? Either we want to allow it or disallow it via panic.

carllerche
carllerche reviewed Nov 15, 2019
View reviewed changes
@hawkw hawkw mentioned this pull request Nov 22, 2019
Closed
@asonix asonix mentioned this pull request Nov 26, 2019
Closed
hawkw added 5 commits November 26, 2019 13:00
@hawkw
Merge branch 'master' into eliza/local-spawn
66f4a45 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
fix bad merge
40374f7 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
fix bad merge 2: the fixening
7260998 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
more cleanup
e40faca 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
rm empty file committed accidentally
9c805b6 
Signed-off-by: Eliza Weisman <[email protected]>
carllerche
carllerche reviewed Nov 26, 2019
View reviewed changes
hawkw added 2 commits November 26, 2019 15:30
@hawkw
move to rt-util feature
04b0189 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
panic if nested
0528537 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw hawkw merged commit 38e602f into master Nov 27, 2019
@carllerche carllerche deleted the eliza/local-spawn branch November 28, 2019 04:10
@ADD-SP ADD-SP mentioned this pull request Aug 29, 2025
Open
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonhoo jonhoo jonhoo approved these changes

@carllerche carllerche carllerche approved these changes

@taiki-e taiki-e Awaiting requested review from taiki-e

Assignees

@hawkw hawkw

Labels
C-enhancement Category: A PR with an enhancement or bugfix.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hawkw @jonhoo @carllerche