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

Tracking Issue for scoped threads #93203

Closed
10 tasks done
m-ou-se opened this issue Jan 22, 2022 · 74 comments · Fixed by #98118
Closed
10 tasks done

Tracking Issue for scoped threads #93203

m-ou-se opened this issue Jan 22, 2022 · 74 comments · Fixed by #98118
Labels
A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. finished-final-comment-period The final comment period is finished for this PR / Issue. relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@m-ou-se
Copy link
Member

m-ou-se commented Jan 22, 2022

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

let local_var = vec![1, 2, 3];

thread::scope(|s| {
    s.spawn(|| println!("borrowed from thread #1: {:?}", local_var));
    s.spawn(|| println!("borrowed from thread #2: {:?}", local_var));
    println!("borrowed from the main thread: {:?}", local_var);
});

Public API

Documentation: https://doc.rust-lang.org/nightly/std/thread/fn.scope.html

// std::thread

pub fn scope<'env, F, T>(f: F) -> T
where
    F: for<'scope> FnOnce(&'scope Scope<'scope, 'env>) -> T;

pub struct Scope<'scope, 'env: 'scope> { ... }

impl<'scope, 'env> Scope<'scope, 'env> {
    pub fn spawn<F, T>(&'scope self, f: F) -> ScopedJoinHandle<'scope, T>
    where
        F: FnOnce() -> T + Send + 'scope,
        T: Send + 'scope;
}

impl Builder {
    pub fn spawn_scoped<'scope, 'env, F, T>(
        self,
        scope: &'scope Scope<'env>,
        f: F,
    ) -> io::Result<ScopedJoinHandle<'scope, T>>
    where
        F: FnOnce() -> T + Send + 'scope,
        T: Send + 'scope;
}

pub struct ScopedJoinHandle<'scope, T> { ... }

impl<'scope, T> ScopedJoinHandle<'scope, T> {
    pub fn join(self) -> Result<T>;
    pub fn thread(&self) -> &Thread;
    pub fn is_finshed(&self) -> bool;
}

Steps / History

Unresolved Questions

@m-ou-se m-ou-se added A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Jan 22, 2022
@m-ou-se
Copy link
Member Author

m-ou-se commented Jan 22, 2022

Nominating this for @rust-lang/lang team discussion for the unresolved question.

@m-ou-se m-ou-se added the I-lang-nominated Nominated for discussion during a lang team meeting. label Jan 22, 2022
@danielhenrymantilla
Copy link
Contributor

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 (Copy) type definitions with a #[favor_capture_by_move] annotation on them or something, so that, in a non-generic context, if one has let x = instance_of_that type; || uses(x) || uses(&x), x is nonetheless captured by move.
It sounds like a kind of straight-forward thing to implement? I'd be willing to try to tackle the implementation 🙂.
With it, we'd then replace &'scope Scope<'env> with ScopeRef<'scope, 'env> (probably still named Scope, but I'm using the Ref suffix here to clarify the comparison with the current implementation). The reason for that would be to have that first 'scope parameter be non-covariant (≠ the (co)variance of 'scope in &'scope Scope<'env>), so that the requirement for the scoped joined thread closure would become F : 'scope rather than F : 'env, so that F would be allowed to capture the ScopeRef.

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 .spawn(|| …) functions), but would also avoid the rather hard to grasp error messages that somebody gets when they update their scoped threads usage with a new nested thread.

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 nightly, the following error message:

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 #[favor_capture_by_move], for instance.


These have been my two cents on the topic, thanks for reading 🙏, and won't insist anymore on this subject, I promise 😄

@nikomatsakis
Copy link
Contributor

Hmm, regarding the capture of references, I think that the RFC 2229 logic might be relevant here. i.e. we could capture *scope by reference instead of capturing scope by reference. In a way, I'm always surprised we don't.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Feb 15, 2022

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 spawn calls don't need to get a scope? In other words, instead of this:

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.

Question

Similar 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 s necessary? Couldn't we just do s.spawn(|| { .. }) and capture s?

Why || { s } doesn't work, take 1

If you try to setup an API like the one above, you will find that it doesn't work. The problem is that the inner || s closure captures s from the outer frame by reference. This turns out to be a general thing: any time that you have reads of a copy type, a non-move closure like || s will always capture it by reference. The reason is that non-move closures take the "least permissions" that they can: they prefer to capture by & ref, then by &mut ref, and finally to take ownership ("by value").

The downside of capturing by reference in this case is that it means the lifetime of the inner closure is linked to the outer closure, and we need them to be independent. We want the outer closure to return and then the inner closure is still running in a parallel thread, so it can't have a link to the outer closure.

Shouldn't we just always capture copy types by value?

So why DON'T we always capture copy types by value? It's debatable if the current behavior is right, but it wasn't an accident. For fun, here is an internals thread where we discussed this very question in 2015.

The intuition for the current rules is that a || (non-move) closure is "part of the enclosing stack frame". The problem is that this is not the behavior we want here, we actually want something that is detached from its immediate parent, but attached to the parent of the scope itself. We don't have a way to express this (we'll return to this theme).

This is not something we can "just change", it would change the semantics of existing programs in various ways. Consider this example:

let mut i = 0;
let c = || println!("{}", i);
i += 1;
c(); 

Today, this code fails to compile, because c grabs a shared reference to i, preventing the mutation i += 1. If you change c to a move closure, the code compiles and prints 0 (not 1, because it now has its own independent copy of i).

I suspect there are examples of code that runs in both versions but does something different, though at the moment I'm not sure what that is (if Cell were copy, it'd be easy to create such programs, but it's not, partly for this reason).

What if users use move closures by hand?

OK, so if by-reference closures aren't quite what we want (because we want something detached from the caller), why not have people use a move closure? i.e., why not write s.spawn(move || ....)? That would work fine for copy types like s, but not so well for things that are not copy.

The problem is that the intuition for a move closure is that it is detached from the surrounding stack entirely. But that's not what we want here. As we said, we want something that remains attached to the parent stack, but not the immediate parent. We want it to be attached to the scope's parent.

If you use a move closure, patterns like this don't work, and that's annoying:

let v = vec![];
scope(|s| {
    s.spawn(move || {
        for e in &v {
        }
    });
    s.spawn(move || {
        for e in &v {
        }
    });
})

Here, the two move closures would both try to take ownership of v, which means this code won't compile. We'd have to write let v = &v before spawning for this to work.

Wait, what about RFC 2229?

So, the above is all accurate for Rust 2018, but as of Rust 2021 the situation is actually somewhat different. In particular, we can now capture places and not just variables. Actually, in Rust 2021, it turns out that we don't capture s -- we capture *s. You can see this if you use some internal rustc annotations. Analyzing this program gives output that includes:

...
note: Min Capture s[Deref] -> ImmBorrow
  --> src/main.rs:23:25
   |
23 |                         s.spawn(
   |                         ^
...

The s[Deref] means that the closure is borrowing &*s, and not &s. This is important because it means that we don't actually capture data from the intermediate stack frame, which is going to terminate. We capture the Scope object directly from its owner, which is the scope function, which (simplified) looks like this:

fn scope<'env>(c: impl FnOnce(&Scope<'env>) + 'env) {
    let s = Scope::new();
    c(&s)
}

So why don't things work?

The role of the 'env lifetime

The problem is this 'env lifetime. It represents "the lifetime of the closure's captured data", and it can basically be anything, but it has to be larger than the call to scope itself. Therefore, so long as scope doesn't return, that data remains valid (which is what we want for this API). The spawn API requires that each thread's closure outlives 'env:

impl<'env> Scope<'env> {
    pub fn spawn<'scope, F, T>(&'scope self, f: F) -> ScopedJoinHandle<'scope, T>
    where
        F: FnOnce(&Scope<'env>) -> T + Send + 'env,
        T: Send + 'env;                    // ^^^^ here                        
}

Putting pieces together

So, let's put it together:

  • We have a lifetime 'env that lives "sometime longer then the call to scope()"
  • Threads can only captaure things that live for 'env
  • We have a s variable that threads need to capture which is a local variable within scope()

You see the problem? Threads cannot capture s because it doesn't live for 'env. We can't quite express what we need to express, which is something like "some lifetime that is internal to 'scope but longer than the call to this closure". It's possible we'll grow ways to say that or tweak the system in the future, but I don't think it'll be soon. =)

Other possible language additions

I could imagine us trying to tweak the closure capture rules here, but I'm not sure exactly how. In some ideal-ish world we've have a way for the compiler to know "how detached" the closure ought to be from the parent stack frame (e.g., based the lifetime bound that the spawn API requires).

I've advocated for a more explicit set of capture clauses, too, but that doesn't really solve anything here, since you wouldn't want to use those on a common basis.

I think that the context-capabilities proposal that @tmandry recently blogged about is relevant here (I view the "scope" as a kind of capability), but that's obviously far out.

In any case, any change here will require a fair bit of research and coordination.

@m-ou-se m-ou-se removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Feb 15, 2022
@danielhenrymantilla
Copy link
Contributor

danielhenrymantilla commented Feb 15, 2022

Thanks for the detailed write-up, @nikomatsakis 🙏

It turns our that that 2021-edition-captures tangent about a &T being captured by "value" rather than by ref (&&T on older editions) is actually a very interesting escape hatch. It basically provides one case of my advocated #[favor_capture_by_move], and one case is all I needed:

&'scope Scope<'scope, 'env>

scope parameter that would be captured "by value", and with the inner 'scope, made interiorly invariant, so as to be able to use F : 'scope rather than F : 'env.

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 😅

  • More seriously: if hidden behind a type alias, the API docs could just show scope: ScopeRef<'scope, 'env>, so it's not that bad.

@nikomatsakis
Copy link
Contributor

@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 for<'a where 'a: 'b> (not a syntax I would recommend, but hopefully you get the idea) then effectively what we want to be able to say is something like

fn scope<'env>(f: impl for<'scope where 'env: 'scope> FnOnce(&'scope Scope<'scope>)

i.e., each of the spawned closures must outlive this 'scope, but they don't quite know what 'scope is, only that it is outlived by 'env. So if they can prove they outlive 'env, that's good enough.

You can indeed often simulate where clauses like this by leveraging implied bounds, e.g., struct Foo<'a, 'b> where 'a: 'b, which is kind of the trick here, but it does make the signature more complex of course (as you noted).

@m-ou-se
Copy link
Member Author

m-ou-se commented Feb 16, 2022

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 for<.. where ..> explanation helps a bit here.

(Not sure if that type alias helps much, since the definition of such an alias is public and part of the stable api anyway.)

@danielhenrymantilla
Copy link
Contributor

danielhenrymantilla commented Feb 16, 2022

The gist regarding the lifetimes involved here, would be:

  • for<'scope where 'env ⊇ 'scope> F : UsableAtLeastWithin<'scope>

    F : for<'scope where 'env ⊇ 'scope> UsableAtLeastWithin<'scope>

    F : UsableAtLeastWithin<'env>

    • Where I'm using the trait UsableAtLeastWithin<'region> = 'region; alias for better readability, and 'a ⊇ 'b notation for 'a : 'b.

    So we can replace the F : 'env bounds with F : 'scope bounds, provided 'scope be invariant1
    That way we can have the closures capture the …Scope… itself.

  • We achieve invariance of 'scope by having it inside the Scope type, since otherwise, with the &'scope part alone, 'scope would be covariant (the region 'scope could be shrunk by users).

  • We need to have a &'scope Scope<'scope> rather than just a ScopeRef<'scope> because of this capture-by-move thing, where &'scope Scope<'scope> is captured as such, whereas a ScopeRef<'scope> would be captured by reference, yielding a &'short_lived ScopeRef<'scope>, which would not meet the : 'scope requirement.

  • To get a for<'scope where 'env : 'scope> quantification in current Rust, we carry an implicit 'env : 'scope bound inside the Scope type itself, so that for<'scope> quantifications that "see" the Scope<'scope, 'env> type end up correctly constrained. Hence why Scope also needs to be infected with 'env2.

  • As to what does 'env represent, it's a caller-chosen upper-bound on this quantification, and the caller will get maximal flexibility by picking a minimal 'env. In Rust, the minimal 'lifetime fed to a generic function call necessarily spans beyond the return of that call. In this instance, thread::scope(…):

    thread::scope(…)
    //             |
    //             | 'env cannot end before this call, and has no reason to end later.
    
    
    thread::scope(|scope| {
        …
    }) ---------
    // 'env cannot end before this call, and has no reason to end later.
    

    These lines I've drawn thus represent the limit after which stuff borrowed by the threads may die / be dropped.

Footnotes

  1. if we were to let the caller "shrink 'scope", then they'd end up with F : UsableWithin<'small_enough_region>, which would always be met, resulting in a useless bound

  2. Technically an extra closure parameter could be the one mentioning 'env: it doesn't necessarily have to be tied to Scope itself (e.g., f : impl for<'scope> FnOnce(&'scope Scope<'scope>, [&'scope &'env (); 0])). But we can agree that it wouldn't improve the readability 😅.

@joshtriplett
Copy link
Member

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.

@joshtriplett
Copy link
Member

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?

@m-ou-se
Copy link
Member Author

m-ou-se commented Mar 3, 2022

I think we should go forward with removing that parameter. I implemented @danielhenrymantilla's suggestion here: #94559

@m-ou-se
Copy link
Member Author

m-ou-se commented Mar 5, 2022

Found a soundness bug in the implementation. Here's the fix: #94644

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 8, 2022
…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.)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Mar 8, 2022
…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.)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 8, 2022
…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.)
@m-ou-se
Copy link
Member Author

m-ou-se commented Mar 9, 2022

Adding documentation about the lifetimes here: #94763

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 9, 2022
…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``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 9, 2022
…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```
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Mar 9, 2022
…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````
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 9, 2022
…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`````
@leoleoasd
Copy link

Should we also update the example of JoinGuard in The Rustonomicron book?

thread::scoped::JoinGuard

Note: This API has already been removed from std, for more information you may refer issue #24292.
We still remain this chapter here because we think this example is still important, regardless of if it is still in std.

@danielhenrymantilla
Copy link
Contributor

danielhenrymantilla commented May 18, 2022

are we certain the issue will be rare

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:

  • either feeding that lifetime name to scope():

      #![feature(scoped_threads)]
    
      pub fn foo<'r>(x: &'r u8) {
          ::std::thread::scope::<'r>(|s| {
    +                           ^^^^
              s.spawn(move || drop(x));
    +                 ^^^^
          });
      }
  • or by making an even more nested non-copy borrow:

      #![feature(scoped_threads)]
    
      pub fn foo(x: &u8) {
    +     let local_borrow = &mut { x };
          ::std::thread::scope(|s| {
              s.spawn(|| {
    +             let x = &**local_borrow; // strict reborrow of the original `x`
                  drop(x)
              });
          });
      }
    • (the reason for the latter is that thanks to the 2021 edition, only a local-to-the-function reborrow of x is captured by the closure, thereby removing 'r from the equation).

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 🙏

@steffahn
Copy link
Member

The fact that the support for nested usage of s in non-move closures relies on edition-2021 capturing behavior means that someone who would try to do something like

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

  • (given the current documentation) have a hard time knowing in the first place that this is something that just works on edition 2021
  • have to come up on their own with the solution of putting a reference to x into a variable in order to be able to work with move closures, provided they stay on the older edition; something as follows:
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 ... Scope<...> value is possible without move on edition 2021.


In a perfect world, we could quickly implement both HRTBs with lifetime bounds (in some sort of where clause), as well as a way to mark some Copy type as "always capture by value in closures when in case it would otherwise be captured by immutable borrow", and then the API could become significantly more simple by replacing &'scope Scope<'scope, 'env> by a simple Scope<'scope>, and all the &'scope self receiver types by self. This would then also solve edition-dependence.

In particular as someone who regularly advises newcomers to generally avoid most or all occurrences of &'a Foo<'a> where a value is borrowed with the same lifetime as a lifetime argument (though the rule is even more important only for &'a mut Foo<'a>), and -- what amounts to the same thing -- to avoid &'a self (or even more importantly &'a mut self) methods on a type with lifetime argument 'a, I perceive this type here as considerably ugly. And that's not surprising since the whole thing is nothing but a hack in the first place, around the lack of a feature to mark capture-by-value-instead-of-immutable-reference types.

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 Scope<'scope, 'env> instead of &'scope Scope<'scope, 'env>, and the edition-dependence is lifted. And the signature of thread::scope() is shorter this way even compared to an ultimate Scope<'scope> solution, because it can use lifetime elision. Compare

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 F: FnOnce(Scope<'_, 'env>) -> T, or the more explicit F: for<'scope> FnOnce(Scope<'scope, 'env>) -> T would be preferable. And for<'scope> F: FnOnce(Scope<'scope, 'env>) -> T is yet another option.)

Even without any language changes, there's the option of introducing a type synonym for ScopeRef<'scope, 'env> for &'scope Scope<'scope, 'env>. (Naming could also be changed so the synonym would be called Scope.) This allows the option of lifetime elision in the FnOnce bound, too, as well as giving a shorter name to that type. On the other hand, the scope's methods would still be necessary to be placed on the type itself, since we cannot do a impl TypeSynonym {} block for a type synonym that's defined to be a &.. type, so thread::scope would probably be the only function signature in the standard library that could use the synonym.

@m-ou-se
Copy link
Member Author

m-ou-se commented May 31, 2022

NLL is about to be stabilized: #95565 (comment)

@rfcbot resolve borrow function arg ref example

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels May 31, 2022
@rfcbot
Copy link

rfcbot commented May 31, 2022

🔔 This is now entering its final comment period, as per the review above. 🔔

@m-ou-se
Copy link
Member Author

m-ou-se commented Jun 9, 2022

NLL has been stabilized/merged. The examples that broke before now compile fine on the latest nightly. :)

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jun 10, 2022
@rfcbot
Copy link

rfcbot commented Jun 10, 2022

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.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Jun 10, 2022
@m-ou-se m-ou-se added the relnotes Marks issues that should be documented in the release notes of the next release. label Jun 11, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Jun 12, 2022
…=joshtriplett

Stabilize scoped threads.

Tracking issue: rust-lang#93203

FCP finished here: rust-lang#93203 (comment)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Jun 12, 2022
…=joshtriplett

Stabilize scoped threads.

Tracking issue: rust-lang#93203

FCP finished here: rust-lang#93203 (comment)
@m-ou-se
Copy link
Member Author

m-ou-se commented Jun 14, 2022

Merged and stabilized in 1.63!

@m-ou-se m-ou-se closed this as completed Jun 14, 2022
@steffahn
Copy link
Member

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.

@jackh726
Copy link
Member

Reopening for now. Can be closed with a test case added.

@jackh726 jackh726 reopened this Jun 14, 2022
@compiler-errors compiler-errors added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Jun 15, 2022
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jun 16, 2022
@bors bors closed this as completed in 1f3023c Jun 17, 2022
@jackh726
Copy link
Member

Err I guess I should have reopened the other issue. Whoops. Oh well.

@JanetJDavis123

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. finished-final-comment-period The final comment period is finished for this PR / Issue. relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.