Mark JoinHandle with #[must_use]

[ghost]

This PR adds a #[must_use] attribute to JoinHandle.

The change may introduce a considerable number of warnings in the Rust code, but that's a good thing! Note that warnings are still easy to get around by writing let _ = std::thread::spawn(...), but at least they make the user think twice about forgetting to join the thread.

Generally speaking, spawning a thread with no intention of ever joining should be discouraged since it may lead to very subtle bugs. See #48820 for an explanation.

Closes #48820

cc @matklad

[ghost]

An interesting comment I found in src/liballoc/arc.rs, which may be yet another reason for this change:

// Note that we **do not** run these tests here. The windows builders get super
// unhappy if a thread outlives the main thread and then exits at the same time
// (something deadlocks) so we just avoid this entirely by not running these
// tests.
/// ```no_run
/// use std::sync::Arc;
/// use std::thread;
///
/// let five = Arc::new(5);
///
/// for _ in 0..10 {
///     let five = Arc::clone(&five);
///
///     thread::spawn(move || {
///         println!("{:?}", five);
///     });
/// }
/// ```

[matklad]

It might be a good idea to add a short message, describing the reason for must_use, like

#[must_use = "it is better to explicitly join the spawned thread, otherwise it might be terminated abruptly, without running destructors"]

[ghost]

Done.

[ghost]

Written in 2014. I wish it were still true.

[pietroalbini]

Highfive failed to assign a reviewer, @Kimundi?

[matklad]

I think we might want to cc @rust-lang/libs here: there will be a lot of warnings because of this, so it's better to discuss this at the team level just in case I think?

[Kimundi]

Yeah, this seems like it might have a lot of impact.

[alexcrichton]

Thanks for the PR @stjepang!

I'd personally think, however, that we would not want to do this. I think it's a laudable effort to encourage authors to think about shutdown and how to join threads, but there's a few reasons that I think we won't want to take this strategy:

• This seems pretty likely to cause a lot of warnings on stable. In the sense that I'm sure lots of crates, tests, examples, etc, all use thread::spawn without worrying about the result.
• I'm personally not a fan of lints where they're targeted at "more than half the time you don't want this, but if you do do this extra thing to ignore it". In other words I think there are legitimate use cases for daemonizing the thread and not worrying about its result, but now all of them will be required to do something like let _ = ... or drop(...) to avoid getting this warning. That ergonomic hit is one I'm personally not a huge fan of, but I know others feel quite differently!

I wonder though if there's perhaps another route we can take for warning about this? Not much comes to mind though :(

[ghost]

@alexcrichton

I'd say the key issue here is that thread::spawn is the easiest way of spawning threads, but not the best one for casual use. Manually calling .join().unwrap() is a chore and easy to forget, which makes thread::spawn a potential footgun.

Maybe we don't need to "fix" thread::spawn - maybe we're just lacking an alternative? I actually like the old JoinGuard, which was removed during the leakpocalypse. Maybe we should bring it back, require F: 'static, and call the method guarded rather than scoped? An example of how it might be used:

let guard = thread::guarded(move || {
    // ...
});

// Join the thread.
// Panics if the thread ended with a panic.
drop(guard);

Also, I find the fact that the documentation says the following amusing:

//! When the main thread of a Rust program terminates, the entire program shuts
//! down, even if other threads are still running. However, this module provides
//! convenient facilities for automatically waiting for the termination of a
//! child thread (i.e., join).

We're being warned than daemonized threads will not be cleanly shut down, but fret not - we're covered by convenient facilities for automatic joining. Except that's a lie - there is nothing automatic about calling .join(). Clearly, something is missing here (the comment was written in the old days of JoinGuard). :)

[alexcrichton]

Having an alternative seems plausible to me!

[bors]

☔ The latest upstream changes (presumably #48691) made this pull request unmergeable. Please resolve the merge conflicts.

[shepmaster]

Where does this PR stand? Should it be closed in favor of a new API addition like the proposed thread::guarded? Does the libs team want to weigh in on the current PR and/or the proposed path before @stjepang starts additional work?

[shepmaster]

Does the libs team want to weigh in

Well, now you gotta talk about it at your meeting, so there 😝

[pietroalbini]

Ping from triage @rust-lang/libs! What's the decision on this?

[alexcrichton]

We'll have triage this wednesday and I'll make sure we discuss it there

[sfackler]

We discussed this in triage and like the approach of a separate function which has a #[must_use] guard that joins on drop rather than detaching.

[SimonSapin]

Isn’t #[must_use] mostly useful for types that don’t do anything on drop?

[sfackler]

Mostly but not entirely! In this case, code like

thread::spawn_attached(|| do_expensive_work());

Is totally broken - we're presumably doing this work in a separate thread because we want to do other stuff while it's running, but the handle is going to join when it's dropped immediately.

[pietroalbini]

If I understood this correctly the PR needs to change? Marking as waiting on author @stjepang.

[ghost]

Let's close this PR since it's definitely not the direction we're going to take.

The alternative is to add a new function for spawning threads that returns a join-on-drop guard.

[Veetaha]

std::thread::guarded/scoped() (with 'static closure lifetime limitations of course) is very much needed, would there be any interest in merging a PR with this one, @pietroalbini @alexcrichton?