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

Scoped threads in the standard library, take 2 #3151

Merged
merged 10 commits into from
Jan 22, 2022

Conversation

bstrie
Copy link
Contributor

@bstrie bstrie commented Jul 19, 2021

The successor to #2647 .

Add scoped threads to the standard library that allow spawning threads that borrow variables from the parent thread.

let var = String::from("foo");

thread::scope(|s| {
    s.spawn(|_| println!("borrowed from thread #1: {}", var));
    s.spawn(|_| println!("borrowed from thread #2: {}", var));
});

Rendered

@yaahc yaahc added A-threads Threading related proposals & ideas T-libs-api Relevant to the library API team, which will review and decide on the RFC. labels Jul 19, 2021
@danielhenrymantilla
Copy link

Note that as discussed in #2647 (comment), it is possible to offer an API that doesn't take a parameter on the sub-closures:

let var = &String::from("foo");
thread::scope(|s| {
    s.spawn(move || {
        println!("borrowed from thread #1: {}", var);
        s.spawn(|| println!("Another one"));
    });
    s.spawn(|| println!("borrowed from thread #2: {}", var));
});

although it does require (a copy of) s to be captured by move (aside – I wonder if in a future edition the semantics of "closure that captures a Copy (and non-Freeze) value" could be made so that it automatically captures it in an owned fashion rather than borrowed).

I personally believe that this tiny "detail" of having to add move will, in practice, be a very unergonomic hurdle which, unless palliated with move { s } || … style of closures, makes this design, sadly, worse than the classical |s| { … } one.

So whilst I believe that the current design is better, the ||-design deserves to be, at least, mentioned in the Alternatives section, if only for future reference for other similar designs which could benefit from that knowledge.

to everyone.

# Prior art
[prior-art]: #prior-art
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior art also exists in Swift's TaskGroup introduced in SE-0304. Instead of working directly on threads, it works with async "tasks" (and is closer to e.g. an async_std::task::Task). But the design still covers a lot of the same space as we do in Rust, and as such I think it's worth to include in the prior art section.

Can this concept be extended to async? Would there be any behavioral or API differences?

# Future possibilities
[future-possibilities]: #future-possibilities
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably reference "async scopes" #2647 (comment).

# Unresolved questions
[unresolved-questions]: #unresolved-questions

Can this concept be extended to async? Would there be any behavioral or API differences?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to read more on the reasoning why we're going for "scoped threads" rather than a "scoped thread pool" (see also: #2647 (comment)). I feel like these two concepts are closer to each other than one might intuitively assume, and it's important to cover the relationship between the two.

but they work on a different abstraction level - Rayon spawns tasks rather than
threads. Its API is the same as the one proposed in this RFC.

# Unresolved questions
Copy link
Member

@yoshuawuyts yoshuawuyts Jul 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to raise a point on naming here.

The ambiguity of "Scope"

I remember the first time I learned about "scoped threads" and being confused about what they do. I wondered whether it referred to:

  1. A "scope" as in "function scope"
  2. A "scope" as in "telemetry scope"
  3. Some other kind of "scope" specific to concurrent programming I was not yet aware of.

To this day I'm still not entirely clear on its origin. Though it seems likely that when this API was first introduced in 2014 (#461) the name is a reference to Boost C++ Scoped Threads, which roughly seems to use "scope" as an analog for "grouping of".

Alternative Naming

Instead I would prefer we follow Swift's example, using simpler naming, and go with ThreadGroup:

- struct Scope<'env> {}
+ struct ThreadGroup<'env> {}

- fn scope<'env, F, T>(f: F) -> T {}
+ impl ThreadGroup<'env> {
+     fn new<'env, F, T>(f: F) -> T {}
+ }

- struct ScopedJoinHandle<'scope, T> {}
+ struct GroupJoinHandle<'scope, T> {}

The name ThreadGroup implies a type containing threads which work as a group — which is exactly what this API does. Seeing ThreadGroup referenced inside code should also be immediately clear as to what it does. If we contrast the two APIs:

// Immediately clear that this refers to a group of threads.
struct Thing {
    group: ThreadGroup,
}

// `Scope` requires additional context to clarify what it does.
struct Thing {
    scope: Scope,
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scope means "lexical scope" (where "function scope" is one kind of lexical scope). They're named "scoped threads" because the API ensures the threads have exited before the scope ends.

A "thread group" is a much weaker concept - simply a set of threads that could be "joined" as one. A thread group need not be scoped.

@steveklabnik
Copy link
Member

Re-posting my comment from the last version of this RFC:

I would like to register strong support here. I can't count the number of examples and documentation that would make use of scoped threads, and be way better for it.

Furthermore, at least in the kind of code that I write, often, scoped threads are what I actually want, rather than thread::spawn. thread::spawn's signature is more general, but that means that it can be used in less situations, and I rarely need the power of thread::spawn.

@Kestrer
Copy link

Kestrer commented Jul 26, 2021

It's probably worth waiting to merge this until we've figured out how to support run-to-completion futures, given the possibility of a Leak trait being added which could obsolete this API.

@Diggsey
Copy link
Contributor

Diggsey commented Jul 26, 2021

@Kestrer could you elaborate on why that would obsolete this API?

This is a synchronous API which has existed in a crate for many years without needing to change once, and it's already very overdue in the standard library. A futures-based API might also be useful, but it would be unprecedented for an async API to live under std::thread, which deals exclusively with synchronous, threaded concurrency.

@bstrie
Copy link
Contributor Author

bstrie commented Jul 26, 2021

@Kestrer Is there a proposal or prior discussion for run-to-completion futures that includes a Leak trait?

@Kestrer
Copy link

Kestrer commented Jul 26, 2021

could you elaborate on why that would obsolete this API?

The existence of a Leak trait would allow for a different, simpler API to be added: a scoped spawn method that automatically joins the thread when the join handle is dropped, because !Leak types would have their destructors be guaranteed to run.

Is there a proposal or prior discussion for run-to-completion futures that includes a Leak trait?

Yes, it was proposed by @Dirbaio on zulip (archive). It does have problems (archive) but I still believe it to be an option.

@Diggsey
Copy link
Contributor

Diggsey commented Jul 26, 2021

From that proposal:

This depends on how the final AsyncDrop design looks. We'll asume we have found a way to enforce that all things with AsyncDrop are async-dropped instead of sync-dropped.

I mean... this seems fundamentally impossible to begin with. Sync code doesn't yield so the only way this can work is if you prevent types with AsyncDrop from being used from synchronous code in the first place. This is getting a bit off-topic though.

I think we may at some point get something like the Leak trait (although I think a more promising approach would be an Affine/Linear distinction, as this would actually help with the AsyncDrop problem...) but I think that would be a bad reason to delay adding this API:

  • I don't think such a massive change would happen any time soon.
  • We already have APIs which work in the same way as scope.
  • There is not a big cost to having both APIs.

@rpjohnst
Copy link

Notably, a Leak-based API was already considered and rejected long, long ago, as part of the resolution of the "leakpocalypse" mentioned in the RFC. Perhaps revisiting that question in the async context will give a different answer, but nothing's really changed for the sync case since then.

@SkiFire13
Copy link

SkiFire13 commented Jul 27, 2021

What if this was implemented with a macro?

The idea is that the macro would be responsible for safely creating the root scope, ensuring it doesn't get leaked by exploiting temporary lifetime extension. In practice this means that the macro will create the scope and immediately borrow it, thus creating an anonymous local. Since it's anonymous and the macro only exposes a shared reference the user can't leak it, because that requires ownership (maybe something could be done with mutable access, but that's avoided anyway). Thus the anonymous local will always be dropped by the compiler and the API should be sound.

You can find a proof of concept in this playground (most code is copied from crossbeam, the main focus should be the thread_scope! macro and ThreadScope struct).

The main pro of this approach would be avoiding the extra nesting layer for creating the scope, which in my opinion makes the ergonomics pretty bad.

@bjorn3
Copy link
Member

bjorn3 commented Jul 27, 2021

That is basically how pin_mut!() works I think.

@SkiFire13
Copy link

Kinda. Both exploit macros to make some value not accessible to the user. While pin_mut! achieve this by shadowing an already existing binding, my macro is also responsible for creating the value and makes it inaccessible by borrowing it. Another difference with pin_mut! is that thanks to exploiting temporary lifetime extension my macro expands to an expression rather than a statement. This means you can write let scope = thread_scope!(); rather than thread_scope!(scope);, which in my opinion feels much more natural.

@Dirbaio
Copy link

Dirbaio commented Jul 27, 2021

Perhaps revisiting that question in the async context will give a different answer, but nothing's really changed for the sync case since then.

In sync code you can enforce cleanup runs just fine with closure-based APIs like the one proposed here. It's impossible for the execution flow to "magically vanish", the user closure either returns normally or it panics, in either case cleanup is guaranteed to run.

In async code execution flow can indeed "magically vanish" at any await, by leaking the Future. There's absolutely no way to prevent that without Leak or affine types. I hope this tips the balance in adding Leak :)

macro will create the scope and immediately borrow it, thus creating an anonymous local. Since it's anonymous and the macro only exposes a shared reference the user can't leak it, because that requires ownership

This doesn't work in async context: You can create the scope, suspend the future in an .await and then then leak it. The scope is stored in the Future, thus leaked.

This is not a problem with pin_mut! because Pin mandates "Drop must run before the memory for self is reused for something else", not "drop must run". Locals stored in the future fulfill that fine because the future itself is pinned too.

@bstrie
Copy link
Contributor Author

bstrie commented Jul 27, 2021

I encourage proponents of Leak to open an RFC soon so that it may be properly discussed. In the meantime let's keep discussion here focused on this RFC. In the next revision I'll mention these proposals in the alternatives section.

@Diggsey
Copy link
Contributor

Diggsey commented Jul 27, 2021

This doesn't work in async context: You can create the scope, suspend the future in an .await and then then leak it. The scope is stored in the Future, thus leaked.

It's worth noting that leaking itself is not the problem for the scope API, it's leaking non-'static data which is the problem, since this allows the lifetime to expire before the destructor is called. In an async context, as long as you only borrow from local variables, the returned future is still 'static and so the API is sound even if the future is leaked. This is why pin_mut! is still sound in async contexts.

@Dirbaio
Copy link

Dirbaio commented Jul 27, 2021

In an async context, as long as you only borrow from local variables, the returned future is still 'static and so the API is sound even if the future is leaked

But you can have the scope borrow from outside the future:

async fn boom(from_outside: &'a mut Foo) {
   let scope = thread_scope!();
   scope.spawn(|| do_stuff(from_outside); // this borrows data from outside the Future
   whatever().await; // we suspend the future here and then leak it
   // scope would be dropped here
}

@comex
Copy link

comex commented Jul 28, 2021

Possible extension:

Generalize Scope to arbitrary callbacks. Instead of knowing anything about threads, it would just maintain a list of callbacks that need to be run when the scope ends. Then the scoped thread API would register a callback consisting of thread.join(). Other code could register its own callbacks, typically unsafe code which wants to use the destruction guarantee to provide a safe API.

Examples of other code that could use the same Scope type:

  • Scoped thread APIs from third-party crates, e.g. one that uses a thread pool, or a specialized thread API on a non-std target (since Scope itself could go in core).
  • Arenas: you could allocate something for the scope of the Scope, and stick the deallocation call onto the callback list.

Disadvantages:

  • The only real benefit to this is if someone wants to use a single Scope with multiple different APIs. If you're only using a single API at a time, that API may as well just define its own scope type; it's not hard. But why would you use a single Scope with multiple APIs? It seems logical that someone might want to use both scoped threads and arenas with a single scope, but I don't have any concrete examples.
  • Having to support arbitrary callbacks might make the implementation slightly less efficient.
  • Support for arbitrary callbacks could be added later as a backwards-compatible extension, so conservatism suggests not adding it now.

I think I just argued myself out of thinking this is a good idea, but perhaps someone has a better use case.

@Amanieu
Copy link
Member

Amanieu commented Jan 3, 2022

I think forgetting a join handle sounds like intentional misuse of the API. As punishment for that crime, your screams of panic shall be silenced.

@m-ou-se
Copy link
Member

m-ou-se commented Jan 4, 2022

One more interesting question:

If a scoped thread panics, should thread::scope() use resume_unwind to rethrow the panic payload from that (any) panicked thread? Or should it start a new panic like panic!("a thread panicked")?

// This invokes the panic handler twice. Once by panic!() on the spawned
// thread, and the second time by the panic in `unwrap()` on the main thread. The
// panic payload from the unwrap is the standard Result::unwrap message, not
// the payload from the first panic.
std::thread::spawn(|| panic!("!")).join().unwrap();
// But what about this? Does this also invoke the panic handler on both the
// spawned thread and the main thread? Or do we teleport the panic payload from
// the scoped thread to the main thread and continue unwinding the same panic
// there?
std::thread::scope(|s| s.spawn(|_| panic!("!")));

My current implementation uses resume_unwind and shows only one panic message. I started thinking about this when I was writing the doc comments and wrote "This function will panic when [..]", and figured that might imply a new panic, rather than resuming an already panic_handler'd panic. But I do like that you only see one panic message though. The second one is just additional noise in most cases.

However, it might take a significant amount of time before the panic is resumed, if other threads are still running. That might result in surprising timing of the panic message getting shown and the program terminating:

std::thread::scope(|s| {
    s.spawn(|_| panic!("!"));
    s.spawn(|_| sleep(Duration::from_secs(10)));
});

This program will immediately show thread '<unnamed>' panicked at '!', ..., keeps running, and then ten seconds later suddenly terminates with an error code without any more output. That can get confusing if the second thread also produces output unrelated to the panic. This makes me wonder if it might be better to have the main thread start a new panic!("a thread panicked").

Thoughts?

@m-ou-se
Copy link
Member

m-ou-se commented Jan 4, 2022

Thought about it some more, and I'm now convinced it should start a new panic:

thread '<unnamed>' panicked at '!', src/main.rs:25:21
thread 'main' panicked at 'a thread panicked', src/main.rs:24:5

Then each thread explains why that thread panicked. Otherwise the main thread panics without any thread 'main' panicked at .. message.

It also simplifies the implementation.

@m-ou-se
Copy link
Member

m-ou-se commented Jan 4, 2022

Implementation PR: rust-lang/rust#92555

It doesn't have any tests yet (other than doc tests). If anyone has the time and energy to help out with writing some useful tests, please do. :) (Comment on the PR first though, so we don't do double work.)

@m-ou-se
Copy link
Member

m-ou-se commented Jan 4, 2022

@bstrie I've updated the alternatives section, which I think was the only to-do left on this RFC. Hope you don't mind.

@m-ou-se
Copy link
Member

m-ou-se commented Jan 4, 2022

@rfcbot merge

@rfcbot
Copy link
Collaborator

rfcbot commented Jan 4, 2022

Team member @m-ou-se has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Jan 4, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Jan 12, 2022

image
@yaahc @BurntSushi @dtolnay 🎶 Do you want to click a cheeckboooxx? 🎵 🎶 ❄️

@BurntSushi
Copy link
Member

This is exciting. I'm looking forward to seeing this back in std!

@rfcbot rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Jan 12, 2022
@rfcbot
Copy link
Collaborator

rfcbot commented Jan 12, 2022

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

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this RFC. and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Jan 22, 2022
@rfcbot
Copy link
Collaborator

rfcbot commented Jan 22, 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.

@m-ou-se m-ou-se merged commit 4f235d3 into rust-lang:master Jan 22, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Jan 22, 2022

This RFC has been accepted and merged! 🎉

Tracking issue: rust-lang/rust#93203

Implementation: rust-lang/rust#92555

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 23, 2022
Implement RFC 3151: Scoped threads.

This implements rust-lang/rfcs#3151

r? `@Amanieu`
@pickfire
Copy link
Contributor

Rendered url is broken

@bjorn3
Copy link
Member

bjorn3 commented Jan 26, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-threads Threading related proposals & ideas disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC. to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.