Add JoinHandle::into_join_future().

[kpreid]

This allows spawned threads to be incorporated into Future-based concurrency control without needing to add separate result-reporting channels and an additional layer of catch_unwind() to the thread functions. I believe this will be useful to async/blocking interop and for various applications which want to manage parallel tasks in a lightweight way.

There is a small additional cost which is paid even if the mechanism is unused: the algorithm built in to the shutdown of a spawned thread must obtain and invoke a Waker, and the Packet internal struct is larger by one Mutex<Waker>. In the future, this Mutex should be replaced by something equivalent to futures::task::AtomicWaker, which will be more efficient and eliminate deadlock and blocking hazards, but std doesn't contain one of those yet. I am unsure whether the added cost is significant relative to the overall cost of thread creation and destruction.

This is not an impl IntoFuture for JoinHandle so that it can avoid being insta-stable; particularly because during the design discussion, concerns were raised that a proper implementation should obey structured concurrency via an AsyncDrop that forces waiting for the thread. I personally think that would be a mistake, and structured spawning should be its own thing, but this choice of API permits either option in the future by keeping everything unstable, where a trait implementation would not.

Previous discussion, including why this is worth doing:

• Zulip: "impl Future for std::thread::JoinHandle"
• wg-async: "Design meeting 2024-02-22: impl IntoFuture for JoinHandle"

@rustbot label: +T-libs-api -T-libs

r? libs-api

[kpreid]

@rustbot label -T-libs

[bors]

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

[bors]

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

[Amanieu]

The implementation looks fine, however it suffers from a similar problem as #116237: this isn't a proper "join" operation since the thread may still be executed when the future indicates readiness. This can be a problem since TLS destructors may still be executing in that thread.

[Amanieu]

I don't see a good solution except to document this as a caveat (and do the same for Thread::scope).

We could try to register a TLS destructor for Packet before any other code runs, but TLS destruction order is unspecified in both pthread and C++ so it's not something we can rely on.

[kpreid]

I don't see a good solution except to document this as a caveat (and do the same for Thread::scope).

I’ve added a mention of this consideration to the documentation that this PR adds (i.e. not including Thread::scope). I wasn’t sure how best to insert it into the documentation without too many miscellaneous paragraphs on scattered topics; I decided to create a list of # Behavior details, but I’d like to know if there's something that would be more in line with existing std documentation idiom.

/// * Unlike [`JoinHandle::join()`], the thread may still exist when the future resolves.
///   In particular, it may still be executing destructors for thread-local values.

I also note that this API design prevents one possible solution: run the JoinFuture, then after it completes, call blocking JoinHandle::join() on the assumption that it will complete quickly enough. But, that result can still be achieved by using an explicit channel instead of JoinFuture (the status quo), so this is fine as long as we see into_join_future() as a convenience feature rather than “should be able to be used for everything”.

Another approach, not reasonable in current std, would be to poll for thread completion on a timer, perhaps informed by a TLS destructor hook of its own, to avoid blocking — hm, actually, Thread::is_finished() has the exact same considerations, and it does have some documentation:

/// This might return `true` for a brief moment after the thread's main
/// function has returned, but before the thread itself has stopped running.
/// However, once this returns `true`, [`join`][Self::join] can be expected
/// to return quickly, without blocking for any significant amount of time.

This doesn’t mention TLS specifically, and it makes the implicit claim that TLS destruction will complete quickly, which isn’t guaranteed. Should this be changed too? Regardless, it’s some additional precedent for not worrying too much about TLS destruction.

[joboet]

This is not correct. Since JoinInner::join is called in take_result, the thread will be properly joined, resulting in blocking during the execution of the TLS destructors. IMHO that's a very reasonable behaviour as TLS destructors shouldn't be doing anything except freeing resources anyway, but it's not what's described here.

[Amanieu]

That's a good point: take_result will actually block here if the thread is marked as finished but still executing TLS destructors. This is arguably the more correct behavior, but maybe not what async users are expecting?

[kpreid]

I've now updated the documentation to say that it currently blocks on TLS destruction, but is not guaranteed to continue doing that (because that seems an appropriately conservative starting point).

[ogoffart]

I was just randomly looking at this patch and found a potential issue.

Don't you have a race here?
What if the Future is polled right now?
We already took the waker which was still the noop when we cloned it and released the Mutex on the waker.
Now the Future sets the waker and then check again that it is not finished. But we as we haven't decremented the ref-count yet (we do that only in the next line) so the Future will return Pending and nobody is ever going to wake it.

Since you are using a Mutex for the synchronization of the Waker, I suggest to also use this lock to communicate to the future that we are finished and that the result can be taken out even if the ref-count is still 2.