Remove argument from closure in thread::Scope::spawn.

[m-ou-se]

This implements @danielhenrymantilla's suggestion 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>:

- 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:

-     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:

  thread::scope(|s| {
-     s.spawn(|_| {
+     s.spawn(|| {
          ...
      });
-     s.spawn(|s| {
+     s.spawn(|| {
          ...
-         s.spawn(|_| ...);
+         s.spawn(|| ...);
      });
  });

And, as a bonus, errors get slightly better because now any lifetime issues point to the outermost s (since there is only one s), rather than the innermost s, making it clear that the lifetime lasts for the entire thread::scope.

  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
    |                     ++++
"

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.)

[rust-highfive]

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

[m-ou-se]

cc @nikomatsakis because you put a lot of thought into this issue.

[danielhenrymantilla]

Yay, death to the tie fighters! 😄

[danielhenrymantilla]

It shouldn't be needed for the return value to be allowed to be as small as 'scope

Suggested change

	T: Send + 'scope,
	T: Send + 'env,

mainly, I'd find it less problematic if s.spawn(|| s) were denied.

[m-ou-se]

s.spawn(|| s) seems to be denied in both cases.

Do you have an example of any difference that this change makes?

[danielhenrymantilla]

Hmm, that was unexpected; it looks like the edition-2021 magic fails when the capture is used "by value" but with fully inferred types (the *&& -> & place flattening does not seem to occur if the *&& place starts off inferred). This includes a move in return position.

Thus, using move || s or || -> &_ { s } are both examples that do pass when T is only restricted to be : 'scope.

• Playground

[m-ou-se]

Is it a problem if s.spawn(move || s) works? I can't really think of a good use case, but I'd prefer to not unnecessarily add more restrictions to the return type than necessary. I also think the signature is easier to follow if every lifetime is 'scope without having to wonder why F is 'scope but T is 'env.

[danielhenrymantilla]

🤔 It all boils down to what does the "scoped runtime" do with the returned value when not joined / and potentially when leaked —I suspect the latter is the only actual area of problems, and the current implementation leaks stuff in that case, so at first glance it looks like it's gonna be fine.

• If s can be returned, then scopeguard::guard(s, |s| s.spawn(…)) could also be the value of type T, and then it could be problematic to have such drop logic run within the runtime's auto-join cleanup etc.. Basically by using 'scope we are giving more flexibility to the caller, who oughtn't realistically need it, and which could impair a bit future changes to the implementation so as not to cause unsoundness within that contrived use case.

• So, conservatively, using 'env is the more cautious and future-proof approach; we could always loosen it afterwards. But I agree I haven't delved too much into "all potential cleanup implementations", and that it is thus also possible that there be no problem whatsoever with that extra flexibility.

To recap:

• there could be future-proofing, soundness-wise, issues with the more lenient : 'scope bound (but it may also be fine);
• the signature is a bit more complex with F : 'scope on one end, and T : 'env on the other.

So, one thing that can be done is sticking to T : 'scope for now, but adding a mention to this to the tracking issue so that we remember to think about it before stabilization 🙂

[m-ou-se]

I spent some more time thinking about this, and concluded that using 'scope is actually unsound with the current implementation, since the active thread counter is decremented before dropping an ignored return value:

#![feature(scoped_threads)]

use std::{
    thread::{scope, sleep},
    time::Duration,
};

fn main() {
    let mut a = "asdf".to_string();
    let r = &a;
    scope(|s| {
        s.spawn(move || {
            scopeguard::guard(s, move |s| {
                sleep(Duration::from_millis(100));
                s.spawn(move || dbg!(r));
            })
        });
    });
    drop(a);
    sleep(Duration::from_millis(200));
}

With std from this PR:

[src/main.rs:15] r = "�K\u{f}\\"

Eep!

However! After thinking a bit more, I concluded that using 'env doesn't fix things. It is still unsound with 'env, and in fact even the code before the change in this PR is unsound:

fn main() {
    let mut a = "asdf".to_string();
    let r = &a;
    scope(|s| {
        s.spawn(move || {
            scopeguard::guard(r, move |r| {
                sleep(Duration::from_millis(100));
                dbg!(r);
            })
        });
    });
    drop(a);
    sleep(Duration::from_millis(200));
}

On Rust rustc 1.61.0-nightly (8769f4e 2022-03-02):

[src/main.rs:15] r = "!\u{1a}\u{7f}c"

This makes me think that it's unlikely that an implementation would be unsound with the 'scope bound, but not with the 'env bound, since it needs to handle this last example anyway.

(The fix is to drop the return value before marking a thread as joined. So, an implementation issue, not an API issue. I'll send a PR for that.)

[danielhenrymantilla]

Good catch! 🙂

---

So, I've been thinking about this, to fully spell out what my hunch / feeling of uneasyness was.

Let's start with the following new names, for the remainder of this post:

• : 'env is : CanReferToSuffOutsideTheScopeButNotTheSpawner:
  • renamed 'cannot_capture_inside_scope in what follows
• 'scope is CanAlsoSpawnScopedThreads
  • renamed 'cannot_capture_inside_scope_except_spawner in what follows (together with Scope becoming ScopedSpawner).

So from a start point of F and T both being CanReferToSuffOutsideTheScopeButNotTheSpawner, we loosen it down to CanAlsoSpawnScopedThreads capability to F, since we are removing the raw spawner handle it used to receive as a parameter.

- F : 'cannot_capture_inside_scope + FnOnce(ScopedSpawner...) -> T,
+ F : 'cannot_capture_inside_scope_except_spawner + FnOnce() -> T, // equivalent API

• (This change is actually a very good summary of this whole PR, btw 😄)

Now, you are also giving the CanAlsoSpawnScopedThreads capability to the return value of the closure.

- T : 'cannot_capture_inside_scope,
+ T : 'cannot_capture_inside_scope_except_spawner, // new capability

• If such value is obtained through an explicit .join(), then that capability becomes pointless since the spawner handle itself is also available.

• Else, the return value is no longer accessible to the user, and thus its sole API being called, if any, will be its drop glue.

  • If the value is leaked, then such capability becomes pointless as well, since not called.
  • Else, the value is dropped around the join-point (but before it, once your PR fixing the soundness bug is merged).

So what granting it the CanAlsoSpawnScopedThreads capability achieves is allowing this implicit drop, right before the join point cleanup, to spawn extra scoped threads, leading to thread count changes etc.

• This is an extra feature of the API, so it can be beneficial if we deem it to be a useful feature to have;

• It's also a maintenance burden on the scoped threads API, since any time the join logic is touched, implementors will need to be mindful of that rough edge:

  impl<'spawner, T> Drop for Packet<'spawner, T> {
      fn drop(&mut self) {
          // ...
          {
              // Drop the closure's return value: THIS MAY SPAWN EXTRA THREADS
              *self.result.get_mut() = None;
          // ...
          self.spawner.decrement_num_running_threads(...)

With the caveat that mistakes here endanger soundness, and that removing this CanAlsoSpawnScopedThreads capability would be a breaking change.

---

• So, it's not, granted, that much of extra cognitive burden when dealing with the implementation, but it is some.
• However, the obtained flexibility does feel incredibly niche, imho (moreover, even if I were wrong and that capability were useful, then we could always add it later on: it wouldn't be a breaking change).
• Remains the argument of "symmetry in the lifetime bounds, for a simpler function signature", which to me is outweighed by the "caution, extra threads may be spawned during drop glue" subtlety for implementors.

[m-ou-se]

// Drop the closure's return value: THIS MAY SPAWN EXTRA THREADS

I wonder if there's any reasonable implementation where this requires special attention. The point I was trying to make towards the end of my last comment is that implementations need to take care of handling Drop implementations that borrow from 'env anyway, which means the drop needs to happen before any kind of 'joining' happens, which also covers the case where someone spawns more threads from the Drop impl.

I don't think there is really any difference between spawning threads near the end of F, versus doing things in Ts Drop impl, from the perspective of what needs to be taken into account for soundness/correctness. In both cases it's just a thread spawning more threads before it gets joined.

[danielhenrymantilla]

The point I was trying to make

Yeah I guess I didn't get it 100% the first time 😅; I remained convinced it would lead to a more subtle approach, but as you've patiently mentioned, the subtlety is already there even with 'env, as in "the true F is actually F + drop(T)".

Thanks for having put up with my initial skepticism! 🙏

_{^{Silly GH won't let me mark my own remark as resolved 🤦}}

[m-ou-se]

"the true F is actually F + drop(T)"

Yeah, that's a great way of putting it.

[Mark-Simulacrum]

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.)

I think this is ok -- by the time this API stabilizes, it'll be 2021 edition on stable for a good bit of time (at least 4ish releases, I imagine).

Do you think it's worth including a short paragraph about this in the documentation for the function, though? Something like:

The examples for the standard library use current Rust idioms and are written in the 2021 edition. If you're using an earlier edition, you may need to add move modifiers to the closures passed to Scope::spawn.

[joshtriplett]

If you're using an earlier edition, you may need to add move modifiers to the closures passed to Scope::spawn.

I don't think, in general, that we should treat "using an earlier edition" as a consideration that affects codebases not in maintenance mode. We need to make sure older code keeps working, and can get patched as needed, and is capable of using new features whenever possible. But we don't need to write new documentation for how to do new things in older editions.

Many of our examples use new language features, and will continue to do so; I'd prefer not to set the precedent of having to explicitly note that we're using new language features. And even if we do want to do that, I certainly don't think we should document any more than "These examples depend on features of Rust 2021.". (Even that has the problem of suggesting that we should add the same note to other examples.)

[m-ou-se]

I think for now I'll add a note about the edition. We have nothing currently that tells a user to upgrade or even know about a new edition. If they are working on an existing (2018) crate and just update through rustup and want to use this new feature, there is nothing that points them towards the existence of Rust 2021. They don't get any warning that they're using an old edition, and the error they'd get in this case doesn't explain that it wouldn't have happened in the new edition.

[m-ou-se]

Oh actually, we don't have any examples where threads spawned in a scope spawn more threads, so all examples work fine on all editions. Never mind. :)

[m-ou-se]

@Mark-Simulacrum This is ready for review/merging now!

The lifetime discussion has been resolved, and a note about move closures in older editions would only apply code in which spawn is used within spawned threads, which doesn't occur in the examples.

[Mark-Simulacrum]

Some scattered comments, other than the small nit not actual blockers for this PR (r=me) -- maybe good to add to the tracking issue.

[Mark-Simulacrum]

Unrelated to this PR, but it seems worrying that if park() panics we get unsoundness -- it seems like we should be using a drop guard or similar rather than catch_unwind?

(On non-futex or windows platforms, the generic parker implementation has some clear panic points, though they should not happen in theory).

But maybe that kind of defensiveness isn't worthwhile -- anyway, just a passing thought as I wrap my head around the impl here.

[m-ou-se]

We should probably just change those implementations and guarantee that park() never panics.

[Mark-Simulacrum]

I would like to see the documentation expanded here as to the meaning of 'scope and 'env, likely drawing from the commentary on #93203 (comment) and this PR.

Part of me wants to say that we should also add some kind of user-visible note about 'scope/'env -- it seems like this is an API that we would like new users to Rust to go "cool!" about, and I think the current signature will make them afraid that all work in Rust with threads requires complicated lifetime signatures that look pretty opaque -- even to experienced Rustaceans, I suspect :) I'm not sure if putting the note on this type and then linking to it from the other APIs which use Scope is right -- that would be my first instinct, but the more complicated signature is on those APIs (with for<'scope>... and all).

[m-ou-se]

Yeah, we probably should, but I'm not sure how yet. I've added it to the tracking issue.

[danielhenrymantilla]

FWIW, I have an idea of renaming stuff that could improve stuff, but I was gonna officially suggest it in a follow-up PR, but the gist of it would be:

Current	Suggested	Notes
thread::scope()	thread::scoped() or thread::with_scope()	debatable nit
'env	'scope	I don't feel strongly about this one either
'scope	'spawner	but I do for this one
Scope	ScopedSpawner or just Spawner	and thus for this one as well

fn scoped<'scope, R> (
    // `'spawner` is a higher-order lifetime parameter, thus it may not escape the scope
    // of the closure's body: the spawner is effectively "caged" within this function call.
    f: impl for<'spawner> FnOnce(&'spawner ScopedSpawner<'spawner, 'scope>) -> R
) -> R

impl<'spawner, '_> ScopedSpawner<'spawner, '_> {
    fn spawn<R, ThreadBody>(
        &'spawner self,
        thread_body: ThreadBody,
    ) -> JoinHandle<'spawner, T>
    where
        // ThreadBody represents logic / a computation that produces some `R`.
        ThreadBody : FnOnce() -> R,
        // Such computation may (and will!) be run in another thread...
        ThreadBody : Send,
        // ... and thus it may be used / be running for as long as the `ScopedSpawner` is.
        ThreadBody : 'spawner,
        // The computed value needs to be able to come back to our thread.
        R : Send,
        // And its implicit drop glue may act as a computation of its own, that may thus
        // be run while a `ScopedSpawner` is around.
        R : 'spawner,

Funnily enough, by phrasing the F : 'spawner bound as the straightforward "it may be used / may be running while the ScopedSpawner is alive"¹, it turns out we don't need to think about the universal quantification of 'scope : 'env and all those more complicated considerations. We just have a thread that has to be able to be run(ning) / used while the ScopedSpawner, the handle that ensures the thread is joined, is itself dropped.

It is really the main key point.

Then, and only then, you may consider going further in the reasoning:

We can see that:

impl<'spawner> ScopedSpawner<'spawner, '_> {
    fn spawn(
       self: &'_ Self,
       thread: impl 'spawner + Send + FnOnce()...,

is not that different from the more natural:

impl<'spawner> ScopedSpawner<'spawner> {
    fn spawn(
        self: &'_ Self,
        thread: impl 'spawner + Send + FnOnce()...,

Why "natural"

Indeed, if we stare at the latter, it's actually the pre-leakpocalypse API.

And the bound / logic itself, modulo Send, is actually the same as imagining a collection of drop glues / of defers, since joining a thread on drop is not that different from running that thread's logic directly on drop:

struct MultiDefer<'multi_defer> /* = */ (
    Vec<Box<dyn 'multi_defer+ FnOnce()>>,
);
impl Drop for MultiDefer<'_> { fn drop(&mut self) {
    for f in mem::take(self.0) { f(); }
}}

/// Look, same bounds as the pre-leakpocalypse API (but for Send)
impl<'multi_defer> MultiDefer<'multi_defer> {
    fn add(&mut self, f: impl 'multi_defer + FnOnce())
    {
       self.0.push(Box::new(f))

We "just" have to add that extra "upper bound" on 'spawner for convenience, since a post-leakpocalypse API needs a higher-order lifetime, and a higher-order lifetime, by default, not only expresses a local/caged lifetime, but it also covers the range of 'static or other big lifetimes (our spawner won't be living for that long!). Hence the need to opt-out of that "range of big lifetimes", and use a for<'spawner where 'spawner : 'scope> quantification. That where clause can't be written there and then in current Rust, so it is written inside ScopedSpawner's definition, which thus needs to carry that otherwise useless extra lifetime parameter.

Footnotes

1. which is actually almost exactly what happens –just a bit too conservative, since there is a tiny instant between the moment where all the threads are joined and their non-collected computed values are dropped and the moment the ScopedSpawner itself finally dies ↩

[m-ou-se]

I don't think we should rename Scope to Spawner, because I imagine Scope might be able to do more than just spawning threads in the future. For example, I can imagine something like scope.is_panicked() to check if any of the threads panicked. Or scope.num_running_threads() to see how many threads are currently still running in this scope. Etc.

[m-ou-se]

But let's have that discussion on the tracking issue. ^^

[m-ou-se]

@bors r=Mark-Simulacrum

[bors]

📌 Commit a3d269e has been approved by Mark-Simulacrum