Support for !Send tasks #1216
Support for !Send tasks #1216
Conversation
Moxinilian
added
A-Core
| // until the tasks that are spawned by this scope() call complete. (If the caller of scope() | ||
| // happens to be a thread in this thread pool, and we only have one thread in the pool, then | ||
| // simply calling future::block_on(spawned) would deadlock.) | ||
| let mut spawned = local_executor.spawn(fut); |
There was a problem hiding this comment.
It looks like we're running all of the tasks on the local executor, regardless of the context they were spawned with. Maybe we should just have two scopes for simplicity / to avoid potential overhead when only one context is needed? Ex: pool.scope() and pool.local_scope()
There was a problem hiding this comment.
Ah wait. We're awaiting (and i assume that means polling) using the local executor only. Which might be fine? I'm not sure! Separate scopes would probably still be safest.
There was a problem hiding this comment.
Yep. The tasks run on the executor where they were spawned and are only awaited on the local executor. Doing it that way makes the code significantly cleaner because otherwise you have to basically duplicate every line because the executors have different types with no unifying trait.
I see that the tests I've added don't actually prove that this is running the tasks on the right threads. Let me add a test that demonstrates that. If you're still uncomfortable with it I can move to awaiting two groups of futures since I agree that is more obvious.
There was a problem hiding this comment.
I'm less worried about the parallelism (spawning the task with the proper executor is proof enough for me). I'm more worried about using the local executor to poll tasks that it didn't spawn. That seems a bit weird / might produce undefined behavior. I'm not sure how that's implemented (and even if the current implementation works, can we be sure that will always be the case?)
There was a problem hiding this comment.
That's a valid concern. Let me go read more in async-executor/std::future and see what the guarantees actually are.
There was a problem hiding this comment.
Ok, I am convinced this is a reasonable thing to do because of the way async-task is designed. When you spawn a future via their API they produce two structs, a Task and a Runnable. The Task, which is the only part we interact with directly, is used only to check for completion and retrieve the result, it doesn't participate in polling the underlying future at all. The Runnable, which is retained by the Executor/LocalExecutor is what actually polls the future when try_tick is called. So using the Task in a composite future that waits for results is exactly what it is intended for. Turning the crank on the executor to actually run the main futures is a completely separate process.
There was a problem hiding this comment.
Alrighty I'm decently convinced at this point. Lets get this merged!
|
Looks like |
|
Bleh, you're right. I'll do a quick PR for that. Since the single threaded task pool is a trivial case for this I didn't spend much time with it, too little obviously. |
Support for !Send tasks
This adds thread local LocalExectuors to each TaskPool so that they can
spawn_localtasks which are !Send. This is required functionality for #708