Windows TLS - Only register the atexit hook when cleanup can be unloaded

[ohadravid]

Followup to #148799 - One of the motivations was to remove the "Synchronization in thread-local destructors" limitation (see the docs). However, it is still possible to deadlock on Windows with a destructor that uses a join, but only during an application shutdown.

(Edited. The original PR leaked the dtors when a DLL was unloaded.)

This is because calling FlsFree from the atexit hook from the main executable can deadlock if other threads are also running FLS destructors.

This PR tweaks the implementation to only register the hook if cleanup can be unloaded, meaning we are loaded as a DLL.

This has an added benefit of not triggering FlsFree at all during normal application shutdown, which is the most common case and is now handled more naturally by the Windows runtime (which calls the FLS hooks without holding the load-lock).

Checking if the cleanup hook is unload-able is done by checking if it is in the same module as the main exe GetModuleHandleExW(FLAG_FROM_ADDRESS, cleanup) == GetModuleHandleW(ptr::null()).

Join-on-Drop on Process Exit Deadlock Example

use std::thread::JoinHandle;
use std::time::Duration;

struct JoinOnDrop(Option<JoinHandle<()>>);

impl Drop for JoinOnDrop {
    fn drop(&mut self) {
        println!("Joining thread... ({:?})", std::thread::current());
        self.0.take().unwrap().join().unwrap();
        println!("Thread joined");
    }
}

thread_local! {
    static HANDLE: JoinOnDrop = {
        let thread = std::thread::spawn(|| {   
            println!("Starting...");
            std::thread::sleep(Duration::from_secs(3));
            println!("Done ({:?})", std::thread::current());
        });

        JoinOnDrop(Some(thread))
    };
}


fn main() {
    let thread = std::thread::spawn(|| {
        HANDLE.with(|_| {
            println!("Some other thread ({:?})", std::thread::current());
        })
    });

    // ***Here***, because we do not join beforehand, the drops are called from atexit which is done while in loader-lock, so the the binary will deadlock.
    // thread.join().unwrap();
    std::thread::sleep(Duration::from_secs(1));

    println!("Done ({:?})", std::thread::current());
}

r? @ChrisDenton

[rustbot]

ChrisDenton is not on the review rotation at the moment.
They may take a while to respond.

[ChrisDenton]

I like avoiding atexit for the exe itself. I'm less certain about skipping TLS dtors entirely on DLL unload. In any case, let's first check what CI says.

@bors try jobs=msvc,mingw

[ChrisDenton]

Hm, looking at the current docs for LocalKey, it says:

Note that a “best effort” is made to ensure that destructors for types stored in thread local storage are run...

This suggests to me we should favour attempting to run dtors but document the caveats. A deadlock is not a memory safety issue and many types of dtors (e.g. freeing memory) likely won't deadlock. Unloading a DLL (outside of program termination) is a niche use case but not so niche that I think we can justify not running dtors at all if the API claims to bias towards running them.

[ohadravid]

I'm less certain about skipping TLS dtors entirely on DLL unload. In any case, let's first check what CI says.

Actually, seems like I was wrong about the cause of the deadlock: the issue is specifically calling FlsFree from the main exec atexit (and not the cleanup hook's content - we deadlock before we get there), so I think it's possible to have DLL-unload-dtors as well as not deadlocking in this case.

I'll need to investigate this a bit more and I'll update the impl once I have a better idea if the fix is legit.

[rust-bors]

☀️ Try build successful (CI)
Build commit: 6ae382d (6ae382dfd22a3b67ed22b0a695d5d1f95d3a7fde, parent: a9625940af29aa50d785aee438b90f3b272c156f)

[ohadravid]

@ChrisDenton changed to not-leak when DLL unload, kept the no-atexit-in-main-exec because that solves the FlsFree lock. I don't think the docs need any changes now because if you unload the DLL we say that the dtor will run so they must run in the loader lock.

[ChrisDenton]

Ok, I'm happier with this. Just one nit I've commented on.

View changes since this review

[ChrisDenton]

@bors r+

[rust-bors]

📌 Commit f5e5709 has been approved by ChrisDenton

It is now in the queue for this repository.