Skip to content
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

Make blocking! able to borrow #1

Closed
Ekleog opened this issue May 11, 2020 · 5 comments
Closed

Make blocking! able to borrow #1

Ekleog opened this issue May 11, 2020 · 5 comments

Comments

@Ekleog
Copy link

Ekleog commented May 11, 2020

As far as I understand, the blocking! macro is well-scoped. Meaning that it should probably be possible to make it able to borrow from the environment without requiring everything to be 'static like Blocking::new would.

This would be… in a way similar to crossbeam's scoped threads, I guess?

What do you think about this?

@ghost
Copy link

ghost commented May 11, 2020

It's not well-scoped and this is not easy to do for the same reason why scoped tasks are so difficult to implement in a sound way. Consider:

let task = Task::spawn(async {
    // Local variable that will be borrowed
    // inside the blocking threadpool.
    let mut x = 0;

    // Spawn a blocking task borrowing the variable.
    blocking! {
        loop {
            x += 1; // borrowed as &mut i32
        }
    }
});

// Wait until the async task starts running and the
// loop is running on the blocking threadpool.
Timer::after(Duration::from_sec(1)).await;

// Cancel the spawned task.
drop(task); 

// UNDEFINED BEHAVIOR!
// 
// The async task was canceled, which means
// variable `x` doesn't exist anymore.
//
// However, the blocking task is still borrowing
// and using `x`, i.e. use-after-free!

Now, it would in theory be possible for Task::spawn() to be smart and not drop the future until blocking!() completes, but the problem is only solved in this executor then. Also, one could do mem::forget() to pretend that the blocking task has completed. It's a really complicated problem to solve...

@Ekleog
Copy link
Author

Ekleog commented May 11, 2020

Oh. That's a problem of task not being well-scoped, I see… that's sad :(

Random idea: would it be possible to enforce that all task spawning be well-scoped, which would thus probably solve the issue by doing structured concurrency? That's probably a larger project, though, that I should maybe take to smol's repo?

I was thinking that recording task spawning as associated to the “spawner” might be a solution, but actually that's not enough, unless it's not possible to pass tasks across threads… but maybe mediating task-passing by the spawner would work, then, so that both spawners update their representation of which tasks must outlive which?

@ghost
Copy link

ghost commented May 11, 2020

To be quite honest, I thought about this a lot but in the end have given up - I'm skeptical that scoped tasks will ever pan out... :/ See also tokio-rs/tokio#1879

@Ekleog
Copy link
Author

Ekleog commented May 11, 2020

Hmm… Ok, I'm still in the phase where I think something is possible but don't have enough time on my hands to get to it, so… :)

FWIW, my current envisioned solution to your example would be to have Task not contain the actual task but a just a handle to the task in a tree structure, when one spawns a new task it always gets registered as a child task, and when dropping a task handle, it automatically registers a “parentless” task that cancels all the tasks by starting with the children and going up the tree, until reaching the dropped task.

I'm not sure I see how this could fail (as in result in UB, I'm not sure about the API it's supposed to give), but I'm not going to waste your time :)

Shall we close this issue because it's most likely not actionable, or keep as an ideal future feature that in a perfect world blocking would have?

@ghost
Copy link

ghost commented May 11, 2020

The tokio thread above is well worth reading if you're into this topic :) Anyways, I'd love to see this problem solved, it's still a big open-ended question.

I'm going to close this issue because there's nothing actionable right now. We can reopen in the feature if someone figures out a way to solve this.

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

No branches or pull requests

1 participant