std::thread::JoinHandle should be marked with #[must_use]

[ghost]

Here's a problem that caught my attention after reading @matklad's blog post titled Stopping a Rust worker. While the presented thread stopping mechanism is indeed very nice and convenient, I think a serious flaw is being overlooked: the spawned thread is never joined.

I've also noticed that spawning threads without ever joining has become a common pattern in Rust code. As a good example, take a peek at IndraDB's codebase.

Seeing something like the following should give everyone a pause and make them think twice:

thread::spawn(|| {
    // ...
});

Not only this thread is not joined, but the JoinHandle returned by thread::spawn is completely ignored. This is a pattern that should be discouraged.

So what's all the fuss about - why is not joining dangerous? Consider this:

use std::sync::mpsc;
use std::thread;

struct Foo;

impl Drop for Foo {
    fn drop(&mut self) {
        println!("Important cleanup work...");
    }
}

fn main() {
    let (s, r) = mpsc::channel::<()>();
    
    thread::spawn(move || {
        // Initialize some kind of resource.
        let _foo = Foo;
        
        // This loop ends when the channel becomes closed.
        for msg in r {
            // Process the message.
        }
    });
    
    // Close the channel.
    drop(s);
}

At the end of the main function, the channel is closed and the worker thread is signalled that no more messages will be sent. This automatically breaks the loop, which is nice.

However, the thread will not have enough time to actually take notice and drop all its variables. Instead, the thread is abruptly killed. That means it doesn't have a chance to do any kind of final cleanup work, like flushing buffers to disk, closing files, deallocating memory, or anything like that.

Even worse - the exit code is 0, Rust doesn't give any warnings, and everything looks good, while it really isn't!

I believe we should mark JoinHandle with #[must_use]. And if the user really doesn't want to join the thread (which should be very rare), they should acknowledge the possibility of the thread silently getting killed and write let _handle = thread::spawn(...) instead.

[matklad]

I think a serious flaw is being overlooked: the spawned thread is never joined.

Mea culpa! I'd say it's even worse, because that was deliberate decision not to join the thread on my part :(

I wonder if #[must_use] is the end-game here though... It looks like most of the time, when you want to use a thread, a scoped one makes more sense, because you can share data without Arcs and because you don't need to explicitly call join. Does it make sense, long-term, to make some scoped API the default for creating threads in Rust?

It's also not clear how to deal with panics: if you explicitly call .join, then the thread won't be joined during a panic. If you join the thread on panic (for example, in Drop, or via the scope API), you risk deadlock...

[ghost]

Mea culpa! I'd say it's even worse, because that was deliberate decision not to join the thread on my part :(

It's okay, I'm blaming the compiler for not showing any warnings. :)

Does it make sense, long-term, to make some scoped API the default for creating threads in Rust?

Probably. There was a thread on this topic in Rust Internals: https://internals.rust-lang.org/t/scoped-threads-in-the-nursery-maybe-with-rayon/4942
cc @steveklabnik

The thing is, we have a very diverse set of options for managing threads: tokio/futures, rayon, crossbeam, various thread pool crates, std::thread::spawn, and so on. They all have their own uses, and we aren't really sure yet what is and what isn't appropriate for the standard library.

It's also not clear how to deal with panics: if you explicitly call .join, then the thread won't be joined during a panic. If you join the thread on panic (for example, in Drop, or via the scope API), you risk deadlock...

I agree, perhaps we should revive JoinGuard from the leakpocalypse but keep the 'static bound.

[varkor]

It was decided that JoinHandle should not be #[must_use] here. Is there any reason to keep this issue open?

[ghost]

Nope. Closing!

[ckaran]

However, the thread will not have enough time to actually take notice and drop all its variables. Instead, the thread is abruptly killed. That means it doesn't have a chance to do any kind of final cleanup work, like flushing buffers to disk, closing files, deallocating memory, or anything like that.

Even worse - the exit code is 0, Rust doesn't give any warnings, and everything looks good, while it really isn't!

ARGHHHHH!!!!! This actually just caught me in some code I'm working on; I have a separate logging thread whose only job is to listen to a channel, and log the incoming contents. Since there are a large number of producers and only one consumer, it made sense to me to use an unbounded channel and join the logging thread at the very end. Unfortunately, something in the standard library has decided to not join, and in some cases my output logs are corrupted. Forcing something better would make life much easier...