-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[RFC] Thread spawn hook (inheriting thread locals) #3642
base: master
Are you sure you want to change the base?
Conversation
cc @epage - You said you wanted this for your new test framework. :) |
dfbcd39
to
de2c8b3
Compare
de2c8b3
to
2acdaeb
Compare
Implementation, including use in libtest: rust-lang/rust#125405 |
Demonstration: Code#![feature(thread_spawn_hook)]
use std::cell::Cell;
use std::thread;
thread_local! {
static ID: Cell<usize> = panic!("ID not set!");
}
fn main() {
ID.set(123);
thread::add_spawn_hook(|_| {
let id = ID.get();
Ok(move || ID.set(id))
});
thread::spawn(|| {
println!("1: {}", ID.get());
thread::spawn(|| {
println!("1.1: {}", ID.get());
thread::spawn(|| {
println!("1.1.1: {}", ID.get());
}).join().unwrap();
thread::spawn(|| {
println!("1.1.2: {}", ID.get());
}).join().unwrap();
}).join().unwrap();
thread::spawn(|| {
ID.set(456); // <-- change thread local `ID`
println!("1.2: {}", ID.get());
thread::spawn(|| {
println!("1.2.1: {}", ID.get());
}).join().unwrap();
thread::spawn(|| {
println!("1.2.2: {}", ID.get());
}).join().unwrap();
}).join().unwrap();
}).join().unwrap();
thread::spawn(|| {
println!("2: {}", ID.get());
}).join().unwrap();
} Output
|
Could this be made more controllable through |
|
||
However, the first (global) behavior is conceptually simpler and allows for more | ||
flexibility. Using a global hook, one can still implement the thread local | ||
behavior, but this is not possible the other way around. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm sure yes it is likely implementation-wise simpler but I don't think it's conceptually simpler.
In particular we already have the Command::pre_exec
which I think is the parallel for Process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how Command::pre_exec is related. That provides a closure to run for one specific Command, not a way to hook into all future Commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems theoretically possible to implement the global behavior via the thread-local behavior, using a thread-local hook that installs itself in the new thread. But I agree that that's not ideal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@m-ou-se If the local effect isn't thread::Builder::add_spawn_hook()
I'm not sure what is meant by "local effect" in this section then 🤔
That might make sense for the stack size, but not for the thread name. And those are (at least today) the only two settings that a I don't think it makes sense to allow 'modifying' the set of hooks. If you want to change/remove any hook, you need a way to identify them. Using a number or string as identifier has many issues, and using some kind of ThreadSpawnHookHandle for each of them seems a bit much, without clear use cases.. |
The default thread name would be empty, as it is today.
The current proposal provides |
Why would you want to fully I strongly believe this should just work like (Even if we were to allow some way to skip the hooks for a specific builder/thread, it doesn't make sense to me to have a global "clear" function to clear all the registered hooks (from the 'global builder' as you propose), because you can't really know which other hooks (of other crates) you might be removing.)
TypeId is not enough. Two hooks might have the same type but a different value (and different behaviour), e.g. with different captures. (Also if you register already boxed |
The RFC already states that threads can be in an unhooked state through pthread spawn. And I assume some weird linkage things (dlopening statically linked plugins?) you could end up with a lot more threads not executing hooks? Additionally I'm somewhat concerned about a tool intended for automatic state inheritance being a global. In the Enterprise Java world such kind of implicit behavior/magic automatisms occasionally get abused to do terribly bloated things which means it's great when that bloat can be limited to a smaller scope or at least be opted out of. In the context of a test framework I can imagine tests running in sets and then clearing global state (or at least enumerating it to print information what didn't get cleaned up) to decouple tests from each other.
"break" in the sense of not doing output capture, as it already is today? Yeah, sure, if the tests need a clean environment for whatever reason that might be a side-effect. |
Today, inheriting the output capturing is done unconditionally when I'm not aware of anyone doing anything specifically to avoid inheriting libtest's output capturing (such as using pthread_create directly for that purpose).
The destructors for e.g. I agree it'd be nice to have some sort of |
I do think we're likely to want deregistration as well, but I think we'd want to do that via a scoped function rather than naming. I don't think it has to be in the initial version in any case. EDIT: On second thought, I'm increasingly concerned about not having the ability to remove these hooks. |
Are there use cases other than testing frameworks in mind? Do they have needs that differ from the testing framework case? |
I think hooks are going to be quite prone to potential deadlocks. That's not a blocker for doing this, just something that'll need to be documented. |
I can think of a few other use cases:
|
TLS drop implementations could likely be used for that. |
thread afterwards, which will execute them one by one before continuing with its | ||
main function. | ||
|
||
# Downsides |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When this feature is used as intended, it will now be the case, where it wasn't before, that Rust threads inherit some characteristics from the thread which spawned them. This could result in unintended nondeterministic behavior when a thread is spawned lazily. For example, rayon
lazily spawns its global thread pool the first time a parallel operation is used.
Therefore, I think that
-
The above should be listed as a downside (it's one more piece of complexity that library authors may need to keep in mind).
-
There should be a way to opt out in
thread::Builder
, resulting in the new thread having the same state as if it was created first thing inmain()
.(In another design I have seen for tasks inheriting state, for a language other than Rust, there was no opt-out, but there was also no equivalent of
static
items, so it was always possible for the action that captured the local state (here, thread spawning) to be be done exactly at the time of construction of the thing that interacts with the spawned thread, rather than lazily later. Because Rust hasstatic
items but a thread can't be spawned at program loading time, I think we need this option.)
|
||
# Rationale and alternatives | ||
|
||
## Use of `io::Result`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A hazard here is that lazy library authors might write an insufficiently contextful error. In particular, they might use the ?
operator to return a real IO error from some kind of per-thread resource creation; such errors are already cryptic, and become more so when separated from their original context.
Therefore, it might make sense to ensure that, if thread::Builder::spawn()
fails due to a hook, the error it returns is a wrapped error whose to_string()
is something along the lines of "failed to execute thread spawn hook" and whose Error::source()
is the error the hook returned. (The message could even include the Location
of the set_hook()
call, so that when debugging, one knows which hook to blame.)
If that is done, then the error type can, and perhaps should, be something other than io::Error
, because there is no advantage to the hook producing an io::Error
specifically, and making it an IO error might be misleading. (Use Box<dyn Error + Send + Sync>
if nothing better comes to mind.)
Co-authored-by: Kevin Reid <[email protected]>
Rendered