-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Tracking Issue for scoped threads #93203
Comments
Nominating this for @rust-lang/lang team discussion for the unresolved question. |
Awesome API, thanks again @m-ou-se for taking care of driving this 👍 To expand on my captures suggestion / "thingy"I think it would generally make sense to be able to mark ( With all that, the API would become: thread::scope(|scope| {
…
scope.spawn(|| {
…
scope.spawn(|| { … });
});
…
}); which is not only neater (there is one scoped / scope-defining API, at the outermost closure layer, and then good old Indeed, consider somebody updating some code and doing: fn main ()
{
::crossbeam::thread::scope(|scope| {
scope.spawn(|_| {
// …
});
scope.spawn(|_| {
+ scope.spawn(|_| { /* … */ })
// …
});
})
.expect("Some thread panicked");
} They'd get, as of the current error[E0521]: borrowed data escapes outside of closure
--> src/main.rs:7:9
|
3 | ::crossbeam::thread::scope(|scope| {
| -----
| |
| `scope` declared here, outside of the closure body
| `scope` is a reference that is only valid in the closure body
...
7 | / scope.spawn(|_| {
8 | | scope.spawn(|_| { /* … */ }) // <- ADDED!
9 | | // …
10 | | });
| |__________^ `scope` escapes the closure body here
|
= note: requirement occurs because of the type Scope<'_>, which makes the generic argument '_ invariant
= note: the struct Scope<'env> is invariant over the parameter 'env
= help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance Now, I personally do love the quality of this error message, but I think we have to admit it can still be quite too "technical / advanced" for some Rust programmers out there (Alan yadda yadda): they shouldn't have to know about variance to know how to use a scoped API. Granted, another option would be to invest some effort in improving the error message when using a scoped API / hard-coding this very situation, but I'd rather see that effort invested on These have been my two cents on the topic, thanks for reading 🙏, and won't insist anymore on this subject, I promise 😄 |
Hmm, regarding the capture of references, I think that the RFC 2229 logic might be relevant here. i.e. we could capture |
In the @rust-lang/lang meeting last week, I agreed to do a writeup about the question of whether a more ergonomic API is possible. I see that @danielhenrymantilla has a nice write-up here as well. What was the question?Is there any way to make it so that scope(|s| {
s.spawn(|s| {
s.spawn(|s| { ... });
})
}) can we have this? scope(|s| {
s.spawn(|| {
// ^^ look ma, no `s`
s.spawn(|| { ... });
// ^^ look ma, no `s`
})
}) What is the answer?No. No? There's no way?OK, fine: maybe someday. But not for a while. Can't you be more specific?If you insist, but remember that you asked for it.I spent a while deep-diving here because the situation wasn't quite what I expected. To explain, though, I think we gotta go back to basics. QuestionSimilar to Rayon and others, the current scoped API gives each task a reference to the scope that contains the task: scope(|s| {
// s here has type `&'a Scope` for some fresh lifetime 'a that is confined to this closure
let s0 = s;
s.spawn(|s| {
// here, we shadow `s`; you get back a `&'b Scope` for some fresh `'b` lifetime specific to *this* closure
// in reality, though, this `s` is the same as the outer one -- i.e., same pointer:
assert_eq!(s as usize, s0 as usize);
});
}); The question is, why is this inner Why
|
Thanks for the detailed write-up, @nikomatsakis 🙏 It turns our that that 2021-edition-captures tangent about a &'scope Scope<'scope, 'env> scope parameter that would be captured "by value", and with the inner Here is a Playground, a bit dirty, tweaked from the original "capture by move" idea Now to decide if such a complex type is warranted 😅
|
@danielhenrymantilla Heh, thanks for proving my conclusion a bit hasty! I think indeed that this scheme can work, though whether the type is too complex is a good question. To elaborate a bit around this point, if we had the ability to write fn scope<'env>(f: impl for<'scope where 'env: 'scope> FnOnce(&'scope Scope<'scope>) i.e., each of the spawned closures must outlive this You can indeed often simulate where clauses like this by leveraging implied bounds, e.g., |
Thanks @nikomatsakis and @danielhenrymantilla! @danielhenrymantilla, your suggestion is interesting! The type is a bit complex, but users don't have to write it out as they just get it as argument to their closure. It's definitely worth considering. However, I'd really like to have some understandable explanation of the lifetimes, which is already quite tricky with the version that's currently implemented. If a user asks how the lifetimes work here, I'd like to be able to explain it without causing more confusion. @nikomatsakis's (Not sure if that type alias helps much, since the definition of such an alias is public and part of the stable api anyway.) |
The gist regarding the lifetimes involved here, would be:
Footnotes
|
I tried the current nightly API in some test code I was writing, and it worked well. Served the purpose I wanted it for, borrowing some local values, without any issues. |
What's the next step here? We have an existence proof that it's possible to eliminate the scope parameter. It might be possible to do so in a simpler way, but we don't know that yet. Can we hide some of the internals and the multiple lifetimes from the public API to allow for future evolution, perhaps by keeping the Scope type unstable for longer than the scope function? |
I think we should go forward with removing that parameter. I implemented @danielhenrymantilla's suggestion here: #94559 |
Found a soundness bug in the implementation. Here's the fix: #94644 |
…without-arg, r=Mark-Simulacrum Remove argument from closure in thread::Scope::spawn. This implements `@danielhenrymantilla's` [suggestion](rust-lang#93203 (comment)) for improving the scoped threads interface. Summary: The `Scope` type gets an extra lifetime argument, which represents basically its own lifetime that will be used in `&'scope Scope<'scope, 'env>`: ```diff - pub struct Scope<'env> { .. }; + pub struct Scope<'scope, 'env: 'scope> { .. } pub fn scope<'env, F, T>(f: F) -> T where - F: FnOnce(&Scope<'env>) -> T; + F: for<'scope> FnOnce(&'scope Scope<'scope, 'env>) -> T; ``` This simplifies the `spawn` function, which now no longer passes an argument to the closure you give it, and now uses the `'scope` lifetime for everything: ```diff - pub fn spawn<'scope, F, T>(&'scope self, f: F) -> ScopedJoinHandle<'scope, T> + pub fn spawn<F, T>(&'scope self, f: F) -> ScopedJoinHandle<'scope, T> where - F: FnOnce(&Scope<'env>) -> T + Send + 'env, + F: FnOnce() -> T + Send + 'scope, - T: Send + 'env; + T: Send + 'scope; ``` The only difference the user will notice, is that their closure now takes no arguments anymore, even when spawning threads from spawned threads: ```diff thread::scope(|s| { - s.spawn(|_| { + s.spawn(|| { ... }); - s.spawn(|s| { + s.spawn(|| { ... - s.spawn(|_| ...); + s.spawn(|| ...); }); }); ``` <details><summary>And, as a bonus, errors get <em>slightly</em> better because now any lifetime issues point to the outermost <code>s</code> (since there is only one <code>s</code>), rather than the innermost <code>s</code>, making it clear that the lifetime lasts for the entire <code>thread::scope</code>. </summary> ```diff error[E0373]: closure may outlive the current function, but it borrows `a`, which is owned by the current function --> src/main.rs:9:21 | - 7 | s.spawn(|s| { - | - has type `&Scope<'1>` + 6 | thread::scope(|s| { + | - lifetime `'1` appears in the type of `s` 9 | s.spawn(|| println!("{:?}", a)); // might run after `a` is dropped | ^^ - `a` is borrowed here | | | may outlive borrowed value `a` | note: function requires argument type to outlive `'1` --> src/main.rs:9:13 | 9 | s.spawn(|| println!("{:?}", a)); // might run after `a` is dropped | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: to force the closure to take ownership of `a` (and any other referenced variables), use the `move` keyword | 9 | s.spawn(move || println!("{:?}", a)); // might run after `a` is dropped | ++++ " ``` </details> The downside is that the signature of `scope` and `Scope` gets slightly more complex, but in most cases the user wouldn't need to write those, as they just use the argument provided by `thread::scope` without having to name its type. Another downside is that this does not work nicely in Rust 2015 and Rust 2018, since in those editions, `s` would be captured by reference and not by copy. In those editions, the user would need to use `move ||` to capture `s` by copy. (Which is what the compiler suggests in the error.)
…without-arg, r=Mark-Simulacrum Remove argument from closure in thread::Scope::spawn. This implements ``@danielhenrymantilla's`` [suggestion](rust-lang#93203 (comment)) for improving the scoped threads interface. Summary: The `Scope` type gets an extra lifetime argument, which represents basically its own lifetime that will be used in `&'scope Scope<'scope, 'env>`: ```diff - pub struct Scope<'env> { .. }; + pub struct Scope<'scope, 'env: 'scope> { .. } pub fn scope<'env, F, T>(f: F) -> T where - F: FnOnce(&Scope<'env>) -> T; + F: for<'scope> FnOnce(&'scope Scope<'scope, 'env>) -> T; ``` This simplifies the `spawn` function, which now no longer passes an argument to the closure you give it, and now uses the `'scope` lifetime for everything: ```diff - pub fn spawn<'scope, F, T>(&'scope self, f: F) -> ScopedJoinHandle<'scope, T> + pub fn spawn<F, T>(&'scope self, f: F) -> ScopedJoinHandle<'scope, T> where - F: FnOnce(&Scope<'env>) -> T + Send + 'env, + F: FnOnce() -> T + Send + 'scope, - T: Send + 'env; + T: Send + 'scope; ``` The only difference the user will notice, is that their closure now takes no arguments anymore, even when spawning threads from spawned threads: ```diff thread::scope(|s| { - s.spawn(|_| { + s.spawn(|| { ... }); - s.spawn(|s| { + s.spawn(|| { ... - s.spawn(|_| ...); + s.spawn(|| ...); }); }); ``` <details><summary>And, as a bonus, errors get <em>slightly</em> better because now any lifetime issues point to the outermost <code>s</code> (since there is only one <code>s</code>), rather than the innermost <code>s</code>, making it clear that the lifetime lasts for the entire <code>thread::scope</code>. </summary> ```diff error[E0373]: closure may outlive the current function, but it borrows `a`, which is owned by the current function --> src/main.rs:9:21 | - 7 | s.spawn(|s| { - | - has type `&Scope<'1>` + 6 | thread::scope(|s| { + | - lifetime `'1` appears in the type of `s` 9 | s.spawn(|| println!("{:?}", a)); // might run after `a` is dropped | ^^ - `a` is borrowed here | | | may outlive borrowed value `a` | note: function requires argument type to outlive `'1` --> src/main.rs:9:13 | 9 | s.spawn(|| println!("{:?}", a)); // might run after `a` is dropped | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: to force the closure to take ownership of `a` (and any other referenced variables), use the `move` keyword | 9 | s.spawn(move || println!("{:?}", a)); // might run after `a` is dropped | ++++ " ``` </details> The downside is that the signature of `scope` and `Scope` gets slightly more complex, but in most cases the user wouldn't need to write those, as they just use the argument provided by `thread::scope` without having to name its type. Another downside is that this does not work nicely in Rust 2015 and Rust 2018, since in those editions, `s` would be captured by reference and not by copy. In those editions, the user would need to use `move ||` to capture `s` by copy. (Which is what the compiler suggests in the error.)
…without-arg, r=Mark-Simulacrum Remove argument from closure in thread::Scope::spawn. This implements ```@danielhenrymantilla's``` [suggestion](rust-lang#93203 (comment)) for improving the scoped threads interface. Summary: The `Scope` type gets an extra lifetime argument, which represents basically its own lifetime that will be used in `&'scope Scope<'scope, 'env>`: ```diff - pub struct Scope<'env> { .. }; + pub struct Scope<'scope, 'env: 'scope> { .. } pub fn scope<'env, F, T>(f: F) -> T where - F: FnOnce(&Scope<'env>) -> T; + F: for<'scope> FnOnce(&'scope Scope<'scope, 'env>) -> T; ``` This simplifies the `spawn` function, which now no longer passes an argument to the closure you give it, and now uses the `'scope` lifetime for everything: ```diff - pub fn spawn<'scope, F, T>(&'scope self, f: F) -> ScopedJoinHandle<'scope, T> + pub fn spawn<F, T>(&'scope self, f: F) -> ScopedJoinHandle<'scope, T> where - F: FnOnce(&Scope<'env>) -> T + Send + 'env, + F: FnOnce() -> T + Send + 'scope, - T: Send + 'env; + T: Send + 'scope; ``` The only difference the user will notice, is that their closure now takes no arguments anymore, even when spawning threads from spawned threads: ```diff thread::scope(|s| { - s.spawn(|_| { + s.spawn(|| { ... }); - s.spawn(|s| { + s.spawn(|| { ... - s.spawn(|_| ...); + s.spawn(|| ...); }); }); ``` <details><summary>And, as a bonus, errors get <em>slightly</em> better because now any lifetime issues point to the outermost <code>s</code> (since there is only one <code>s</code>), rather than the innermost <code>s</code>, making it clear that the lifetime lasts for the entire <code>thread::scope</code>. </summary> ```diff error[E0373]: closure may outlive the current function, but it borrows `a`, which is owned by the current function --> src/main.rs:9:21 | - 7 | s.spawn(|s| { - | - has type `&Scope<'1>` + 6 | thread::scope(|s| { + | - lifetime `'1` appears in the type of `s` 9 | s.spawn(|| println!("{:?}", a)); // might run after `a` is dropped | ^^ - `a` is borrowed here | | | may outlive borrowed value `a` | note: function requires argument type to outlive `'1` --> src/main.rs:9:13 | 9 | s.spawn(|| println!("{:?}", a)); // might run after `a` is dropped | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: to force the closure to take ownership of `a` (and any other referenced variables), use the `move` keyword | 9 | s.spawn(move || println!("{:?}", a)); // might run after `a` is dropped | ++++ " ``` </details> The downside is that the signature of `scope` and `Scope` gets slightly more complex, but in most cases the user wouldn't need to write those, as they just use the argument provided by `thread::scope` without having to name its type. Another downside is that this does not work nicely in Rust 2015 and Rust 2018, since in those editions, `s` would be captured by reference and not by copy. In those editions, the user would need to use `move ||` to capture `s` by copy. (Which is what the compiler suggests in the error.)
Adding documentation about the lifetimes here: #94763 |
…s, r=Mark-Simulacrum Add documentation about lifetimes to thread::scope. This resolves the last unresolved question of rust-lang#93203 This was brought up in rust-lang#94559 (comment) r? ``@Mark-Simulacrum``
…s, r=Mark-Simulacrum Add documentation about lifetimes to thread::scope. This resolves the last unresolved question of rust-lang#93203 This was brought up in rust-lang#94559 (comment) r? ```@Mark-Simulacrum```
…s, r=Mark-Simulacrum Add documentation about lifetimes to thread::scope. This resolves the last unresolved question of rust-lang#93203 This was brought up in rust-lang#94559 (comment) r? ````@Mark-Simulacrum````
…s, r=Mark-Simulacrum Add documentation about lifetimes to thread::scope. This resolves the last unresolved question of rust-lang#93203 This was brought up in rust-lang#94559 (comment) r? `````@Mark-Simulacrum`````
Should we also update the example of
|
My bad, I missed a imho qualifier in it, since that wasn't an objective statement. To detail: I suspect the cases where this issue will be fully blocking one's usage to be rare. Indeed, while the cases where we may run into this issue ("lifetime comes from outside the current function") can happen, they can be worked around by:
so this leads to a rough edge in the API rather than a blocking thing; nothing some documentation can't palliate while waiting for nll to be stabilized. But if nll is to stabilized that fast, then there obviously isn't even a question to begin with, so let's wait for that nominated discussion 🙏 |
The fact that the support for nested usage of fn main() {
let x: String = "Hello World".to_owned();
thread::scope(|s| {
s.spawn(|| {
s.spawn(|| {
println!("{x}");
});
});
});
println!("{x}");
} on edition 2018 or 2015 would
fn main() {
let x: String = "Hello World".to_owned();
let x_ref = &x;
thread::scope(|s| {
s.spawn(move || {
s.spawn(move || {
println!("{x_ref}");
});
});
});
println!("{x}");
} I think at a minimum, the documentation should mention the fact that nested usage of the In a perfect world, we could quickly implement both HRTBs with lifetime bounds (in some sort of In particular as someone who regularly advises newcomers to generally avoid most or all occurrences of Given the fact that closure captures were worked on somewhat "recently" (for the 2021 release) anyway, I'm wondering how hard it would be to, perhaps, at least get the "capture-by-value-not-immutable-reference" marker implemented. With this, it at least becomes fn scope<'env, F, T>(f: F) -> T
where
F: FnOnce(Scope<'_, 'env>) -> T,
{
// ...
} vs. fn scope<'env, F, T>(f: F) -> T
where
F: for<'scope, where 'env: 'scope> FnOnce(Scope<'scope>) -> T,
{
// ...
} (Although, I'm not 100% sure whether Even without any language changes, there's the option of introducing a type synonym for |
NLL is about to be stabilized: #95565 (comment) @rfcbot resolve borrow function arg ref example |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
NLL has been stabilized/merged. The examples that broke before now compile fine on the latest nightly. :) |
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
…=joshtriplett Stabilize scoped threads. Tracking issue: rust-lang#93203 FCP finished here: rust-lang#93203 (comment)
…=joshtriplett Stabilize scoped threads. Tracking issue: rust-lang#93203 FCP finished here: rust-lang#93203 (comment)
Merged and stabilized in 1.63! |
Do we have a test case for the “borrow function arg ref example” code? I’m asking because apparently there’s additional work on NLL happening, and we probably wouldn’t want that test case to break before scoped threads are released, or otherwise (in case of a full revert on NLL) reconsider whether or not scoped threads should be already stabilized without it regardless, or later. In any case having a test case seems important, I couldn’t find any so far, but also I don’t exactly know everywhere I’d have to look for it. |
Reopening for now. Can be closed with a test case added. |
Err I guess I should have reopened the other issue. Whoops. Oh well. |
…lett Stabilize scoped threads. Tracking issue: rust-lang/rust#93203 FCP finished here: rust-lang/rust#93203 (comment)
Feature gate:
#![feature(scoped_threads)]
This is a tracking issue for scoped threads.
RFC: https://rust-lang.github.io/rfcs/3151-scoped-threads.html
Example usage
Public API
Documentation: https://doc.rust-lang.org/nightly/std/thread/fn.scope.html
Steps / History
Unresolved Questions
&Scope
argument to the functions given to.spawn()
? That is,scope.spawn(|| ..)
rather thanscope.spawn(|_| ..)
.move || ..
instead, but that's not great. Maybe the language could be subtly changed to capture references or certainCopy
types by value rather than by reference(-to-reference).'env
and'scope
lifetimes clearly without scaring people away.The text was updated successfully, but these errors were encountered: