Skip to content

Implement scoped tasks #40

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.

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

Closed
wants to merge 4 commits into from
Closed

Implement scoped tasks #40

wants to merge 4 commits into from

Conversation

notgull
Copy link
Member

@notgull notgull commented Dec 3, 2022

This PR implements "scoped tasks", similar to scoped threads in libstd and crossbeam. It provides a safe abstraction for borrowing variables from the local stack in tasks. The goal is to enable scoped tasks in blocking.

For the time being, this relies on async-channel, which uses libstd for the time being. However, once we move it to no_std, this should not be an issue.

This PR probably needs a review for soundness. MIRI doesn't raise any red flags but it's 100% possible that I missed something. Just saw something I missed, let me take care of that.

@notgull notgull closed this Dec 3, 2022
@ethe
Copy link

ethe commented Dec 3, 2022

I have simply thought about the soundness of "scoped-task", there are four kinds of condition just similar as "scoped-threads":

  1. task is awaited in its scope, await would own the ownership of task and drop it after it is complete, so it must be safe;
  2. task is not used in scope, because the lifetime of task is captured in scope, it would be dropped and set to cancel when it leaves this scope, once it is canceled, executor could not run it never more, so I think it is safe;
  3. task is set to detach, this should be unsafe because Task::detach requires task and all captured variables 'static, therefore "scoped-task" must not be detached.
  4. task moves out of the scope, it is impossible because task captures the lifetime of its scope;

@notgull
Copy link
Member Author

notgull commented Dec 3, 2022

task is set to detach, this should be unsafe because Task::detach requires task and all captured variables 'static, therefore "scoped-task" must not be detached.

If we keep a handle to the task stored in the Scope structure (as I do in this PR) this doesn't turn out to be an issue. Task is just a frontend to the inner structure that the handle captures.


The real issue is the fact that the outer scope future can be dropped at will. Consider this sequence of events:

  1. User calls scope() and uses it to spawn a handful of scoped tasks and capture variables from outside of the scope.
  2. User polls scope until it's done with the closure provided the user, and starts waiting for the remaining tasks to finish.
  3. User stops polling scope and drops the scope future.
  4. Since scope is dropped, user now has access to all of the variables that scope borrowed, even if there may still be tasks that simultaneously have access to these variables.
  5. This is undefined behavior.

The only real option would be to have some kind of guard inside of the scope() function that aborts the program if there are any tasks left over when the user drops the future. We can't panic in this state, since a panic can be recovered from and the stacked borrowed rules are being flagrantly violated here, so the only option would be aborting. I'd like to avoid this if at all possible. Aborts are reserved for situations when the program is truly broken, like an overflowed reference counter or an operation that leaves memory in an invalid state. Futures are meant to be dropped often, and aborts are hard to debug. Imagine if this was in an enterprise codebase, running inside of another task that was accidentally cancelled, and it caused an abort that brought down the entire system.

I'd love to be proven wrong here, but I think that the only way to have a sound implementation of scoped tasks would be to have it be a block_on() function or variant thereof. While futures can be dropped at any point, a function execution cannot, which would ensure that the issue of interrupting the wait for the tasks to close could never happen (without the presence of other unsafe code, at least). I guess it would look like this:

pub fn block_on_scoped<'env>(f: impl FnOnce(&Scope<'env>) -> impl Future) {
    ...
}

let mut i = 1;
block_on_scoped(|scope| {
    let i = &mut i;
    let (runnable, task) = scope.spawn(async move { *i += 1; });
    
    async move {
        ...
    }
});

However, that would involve quite a few caveats. For instance, for large programs, you would have to pass the Scope across many function boundaries and threads. This would be very annoying. In addition, we'd probably have to parameterize it across multiple blocking strategies, which would require a substantial amount of effort to design an API for.

All in all, it's probably a job best suited for a crate outside of this one, and would require a careful hand to ensure that everything is sound.

@ethe
Copy link

ethe commented Dec 3, 2022

Yes I think your are right, considering the Scope implementation of std::threads::scoped, it always wait all threads completed, I think scoped-task is might able to use the similar behavior: it should block the current thread until all scoped-task is finished.

As you said, maybe it is not the best way to embed this feature into async-task and blocking crates, however, it could not be implemented without unchecked methods. blocking::unblock_unchecked might be helpful to other user make "scoped-blocking" above blocking crate. Please consider add this method into it.

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

Successfully merging this pull request may close these issues.

2 participants