From 495c998508039764b07a64303ae2c9461ec86a7b Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 24 Apr 2017 11:34:16 -0700 Subject: std: Avoid locks during TLS destruction on Windows Gecko recently had a bug reported [1] with a deadlock in the Rust TLS implementation for Windows. TLS destructors are implemented in a sort of ad-hoc fashion on Windows as it doesn't natively support destructors for TLS keys. To work around this the runtime manages a list of TLS destructors and registers a hook to get run whenever a thread exits. When a thread exits it takes a look at the list and runs all destructors. Unfortunately it turns out that there's a lock which is held when our "at thread exit" callback is run. The callback then attempts to acquire a lock protecting the list of TLS destructors. Elsewhere in the codebase while we hold a lock over the TLS destructors we try to acquire the same lock held first before our special callback is run. And as a result, deadlock! This commit sidesteps the issue with a few small refactorings: * Removed support for destroying a TLS key on Windows. We don't actually ever exercise this as a public-facing API, and it's only used during `lazy_init` during racy situations. To handle that we just synchronize `lazy_init` globally on Windows so we never have to call `destroy`. * With no need to support removal the global synchronized `Vec` was tranformed to a lock-free linked list. With the removal of locks this means that iteration no long requires a lock and as such we won't run into the deadlock problem mentioned above. Note that it's still a general problem that you have to be extra super careful in TLS destructors. For example no code which runs a TLS destructor on Windows can call back into the Windows API to do a dynamic library lookup. Unfortunately I don't know of a great way around that, but this at least fixes the immediate problem that Gecko was seeing which is that with "well behaved" destructors the system would still deadlock! [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1358151 --- src/libstd/sys_common/thread_local.rs | 37 +++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 15 deletions(-) (limited to 'src/libstd/sys_common') diff --git a/src/libstd/sys_common/thread_local.rs b/src/libstd/sys_common/thread_local.rs index 25a9d5720d9..0ade90e64c3 100644 --- a/src/libstd/sys_common/thread_local.rs +++ b/src/libstd/sys_common/thread_local.rs @@ -61,6 +61,7 @@ use sync::atomic::{self, AtomicUsize, Ordering}; use sys::thread_local as imp; +use sys_common::mutex::Mutex; /// A type for TLS keys that are statically allocated. /// @@ -145,20 +146,6 @@ impl StaticKey { #[inline] pub unsafe fn set(&self, val: *mut u8) { imp::set(self.key(), val) } - /// Deallocates this OS TLS key. - /// - /// This function is unsafe as there is no guarantee that the key is not - /// currently in use by other threads or will not ever be used again. - /// - /// Note that this does *not* run the user-provided destructor if one was - /// specified at definition time. Doing so must be done manually. - pub unsafe fn destroy(&self) { - match self.key.swap(0, Ordering::SeqCst) { - 0 => {} - n => { imp::destroy(n as imp::Key) } - } - } - #[inline] unsafe fn key(&self) -> imp::Key { match self.key.load(Ordering::Relaxed) { @@ -168,6 +155,24 @@ impl StaticKey { } unsafe fn lazy_init(&self) -> usize { + // Currently the Windows implementation of TLS is pretty hairy, and + // it greatly simplifies creation if we just synchronize everything. + // + // Additionally a 0-index of a tls key hasn't been seen on windows, so + // we just simplify the whole branch. + if imp::requires_synchronized_create() { + static INIT_LOCK: Mutex = Mutex::new(); + INIT_LOCK.lock(); + let mut key = self.key.load(Ordering::SeqCst); + if key == 0 { + key = imp::create(self.dtor) as usize; + self.key.store(key, Ordering::SeqCst); + } + INIT_LOCK.unlock(); + assert!(key != 0); + return key + } + // POSIX allows the key created here to be 0, but the compare_and_swap // below relies on using 0 as a sentinel value to check who won the // race to set the shared TLS key. As far as I know, there is no @@ -227,7 +232,9 @@ impl Key { impl Drop for Key { fn drop(&mut self) { - unsafe { imp::destroy(self.key) } + // Right now Windows doesn't support TLS key destruction, but this also + // isn't used anywhere other than tests, so just leak the TLS key. + // unsafe { imp::destroy(self.key) } } } -- cgit 1.4.1-3-g733a5