From cbda2450bd5cd9acd14ec4c9aa4f663065f19848 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Tue, 10 Dec 2024 10:52:35 -0800 Subject: [PATCH 01/20] Use FLS detach as thread termination notification on windows. --- src/coreclr/nativeaot/Runtime/threadstore.cpp | 2 +- src/coreclr/vm/ceemain.cpp | 157 ++++++++++++++---- src/coreclr/vm/ceemain.h | 4 + src/coreclr/vm/threads.cpp | 17 ++ 4 files changed, 149 insertions(+), 31 deletions(-) diff --git a/src/coreclr/nativeaot/Runtime/threadstore.cpp b/src/coreclr/nativeaot/Runtime/threadstore.cpp index 33d882f340fea0..6e32929ddb6b55 100644 --- a/src/coreclr/nativeaot/Runtime/threadstore.cpp +++ b/src/coreclr/nativeaot/Runtime/threadstore.cpp @@ -157,7 +157,7 @@ void ThreadStore::DetachCurrentThread() } // Unregister from OS notifications - // This can return false if detach notification is spurious and does not belong to this thread. + // This can return false if a thread did not register for OS notification. if (!PalDetachThread(pDetachingThread)) { return; diff --git a/src/coreclr/vm/ceemain.cpp b/src/coreclr/vm/ceemain.cpp index 58f1544146f9e4..6cb702f449364c 100644 --- a/src/coreclr/vm/ceemain.cpp +++ b/src/coreclr/vm/ceemain.cpp @@ -1684,6 +1684,130 @@ BOOL STDMETHODCALLTYPE EEDllMain( // TRUE on success, FALSE on error. #endif // !defined(CORECLR_EMBEDDED) +static void RuntimeThreadShutdown(void* thread) +{ + Thread* pThread = (Thread*)thread; + _ASSERTE(pThread == GetThreadNULLOk()); + + if (pThread) + { +#ifdef FEATURE_COMINTEROP + // reset the CoInitialize state + // so we don't call CoUninitialize during thread detach + pThread->ResetCoInitialized(); +#endif // FEATURE_COMINTEROP + // For case where thread calls ExitThread directly, we need to reset the + // frame pointer. Otherwise stackwalk would AV. We need to do it in cooperative mode. + // We need to set m_GCOnTransitionsOK so this thread won't trigger GC when toggle GC mode + if (pThread->m_pFrame != FRAME_TOP) + { +#ifdef _DEBUG + pThread->m_GCOnTransitionsOK = FALSE; +#endif + GCX_COOP_NO_DTOR(); + pThread->m_pFrame = FRAME_TOP; + GCX_COOP_NO_DTOR_END(); + } + + pThread->DetachThread(TRUE); + } + else + { + // Since we don't actually cleanup the TLS data along this path, verify that it is already cleaned up + AssertThreadStaticDataFreed(); + } + + ThreadDetaching(); +} + +#ifdef TARGET_WINDOWS + +// Index for the fiber local storage of the attached thread pointer +static uint32_t g_flsIndex = FLS_OUT_OF_INDEXES; + +// This is called when each *fiber* is destroyed. When the home fiber of a thread is destroyed, +// it means that the thread itself is destroyed. +// Since we receive that notification outside of the Loader Lock, it allows us to safely acquire +// the ThreadStore lock in the RuntimeThreadShutdown. +static void __stdcall FiberDetachCallback(void* lpFlsData) +{ + ASSERT(g_flsIndex != FLS_OUT_OF_INDEXES); + ASSERT(lpFlsData == FlsGetValue(g_flsIndex)); + + if (lpFlsData != NULL) + { + // The current fiber is the home fiber of a thread, so the thread is shutting down + RuntimeThreadShutdown(lpFlsData); + } +} + +bool InitFlsSlot() +{ + // We use fiber detach callbacks to run our thread shutdown code because the fiber detach + // callback is made without the OS loader lock + g_flsIndex = FlsAlloc(FiberDetachCallback); + if (g_flsIndex == FLS_OUT_OF_INDEXES) + { + return false; + } + + return true; +} + +// Register the thread with OS to be notified when thread is about to be destroyed +// It fails fast if a different thread was already registered with the current fiber. +// Parameters: +// thread - thread to attach +static void OsAttachThread(void* thread) +{ + void* threadFromCurrentFiber = FlsGetValue(g_flsIndex); + + if (threadFromCurrentFiber != NULL) + { + _ASSERTE(!"Multiple threads encountered from a single fiber"); + COMPlusThrowWin32(); + } + + // Associate the current fiber with the current thread. This makes the current fiber the thread's "home" + // fiber. This fiber is the only fiber allowed to execute managed code on this thread. When this fiber + // is destroyed, we consider the thread to be destroyed. + FlsSetValue(g_flsIndex, thread); +} + +// Detach thread from OS notifications. +// It fails fast if some other thread value was attached to the current fiber. +// Parameters: +// thread - thread to detach +// Return: +// true if the thread was detached, false if there was no attached thread +bool OsDetachThread(void* thread) +{ + ASSERT(g_flsIndex != FLS_OUT_OF_INDEXES); + void* threadFromCurrentFiber = FlsGetValue(g_flsIndex); + + if (threadFromCurrentFiber == NULL) + { + // we've seen this thread, but not this fiber. It must be a "foreign" fiber that was + // borrowing this thread. + return false; + } + + if (threadFromCurrentFiber != thread) + { + _ASSERTE(!"Detaching a thread from the wrong fiber"); + COMPlusThrowWin32(); + } + + FlsSetValue(g_flsIndex, NULL); + return true; +} + +void EnsureTlsDestructionMonitor() +{ + OsAttachThread(GetThread()); +} + +#else struct TlsDestructionMonitor { bool m_activated = false; @@ -1697,36 +1821,7 @@ struct TlsDestructionMonitor { if (m_activated) { - Thread* thread = GetThreadNULLOk(); - if (thread) - { -#ifdef FEATURE_COMINTEROP - // reset the CoInitialize state - // so we don't call CoUninitialize during thread detach - thread->ResetCoInitialized(); -#endif // FEATURE_COMINTEROP - // For case where thread calls ExitThread directly, we need to reset the - // frame pointer. Otherwise stackwalk would AV. We need to do it in cooperative mode. - // We need to set m_GCOnTransitionsOK so this thread won't trigger GC when toggle GC mode - if (thread->m_pFrame != FRAME_TOP) - { -#ifdef _DEBUG - thread->m_GCOnTransitionsOK = FALSE; -#endif - GCX_COOP_NO_DTOR(); - thread->m_pFrame = FRAME_TOP; - GCX_COOP_NO_DTOR_END(); - } - - thread->DetachThread(TRUE); - } - else - { - // Since we don't actually cleanup the TLS data along this path, verify that it is already cleaned up - AssertThreadStaticDataFreed(); - } - - ThreadDetaching(); + RuntimeThreadShutdown(GetThreadNULLOk()); } } }; @@ -1740,6 +1835,8 @@ void EnsureTlsDestructionMonitor() tls_destructionMonitor.Activate(); } +#endif + #ifdef DEBUGGING_SUPPORTED // // InitializeDebugger initialized the Runtime-side CLR Debugging Services diff --git a/src/coreclr/vm/ceemain.h b/src/coreclr/vm/ceemain.h index 1404a5a04237ff..fec164ea3920b7 100644 --- a/src/coreclr/vm/ceemain.h +++ b/src/coreclr/vm/ceemain.h @@ -46,6 +46,10 @@ void ForceEEShutdown(ShutdownCompleteAction sca = SCA_ExitProcessWhenShutdownCom void ThreadDetaching(); void EnsureTlsDestructionMonitor(); +#ifdef TARGET_WINDOWS +bool InitFlsSlot(); +bool OsDetachThread(void* thread); +#endif void SetLatchedExitCode (INT32 code); INT32 GetLatchedExitCode (void); diff --git a/src/coreclr/vm/threads.cpp b/src/coreclr/vm/threads.cpp index cb40472d664620..69240a6ffccda5 100644 --- a/src/coreclr/vm/threads.cpp +++ b/src/coreclr/vm/threads.cpp @@ -353,6 +353,7 @@ void SetThread(Thread* t) { LIMITED_METHOD_CONTRACT + Thread* origThread = gCurrentThreadInfo.m_pThread; gCurrentThreadInfo.m_pThread = t; if (t != NULL) { @@ -360,6 +361,14 @@ void SetThread(Thread* t) EnsureTlsDestructionMonitor(); t->InitRuntimeThreadLocals(); } +#ifdef TARGET_WINDOWS + else if (origThread != NULL) + { + // Unregister from OS notifications + // This can return false if a thread did not register for OS notification. + OsDetachThread(origThread); + } +#endif // Clear or set the app domain to the one domain based on if the thread is being nulled out or set gCurrentThreadInfo.m_pAppDomain = t == NULL ? NULL : AppDomain::GetCurrentDomain(); @@ -1039,6 +1048,14 @@ void InitThreadManager() } CONTRACTL_END; +#ifdef TARGET_WINDOWS + if (!InitFlsSlot()) + { + _ASSERTE(!"Initialization of a FLS slot failed."); + COMPlusThrowWin32(); + } +#endif + // All patched helpers should fit into one page. // If you hit this assert on retail build, there is most likely problem with BBT script. _ASSERTE_ALL_BUILDS((BYTE*)JIT_PatchedCodeLast - (BYTE*)JIT_PatchedCodeStart > (ptrdiff_t)0); From 7a846ec2cd448b9c77dfc22060df703822a150f8 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Tue, 10 Dec 2024 15:33:09 -0800 Subject: [PATCH 02/20] use _ASSERTE_ALL_BUILDS --- src/coreclr/vm/ceemain.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/coreclr/vm/ceemain.cpp b/src/coreclr/vm/ceemain.cpp index 6cb702f449364c..6ffd21e80096d3 100644 --- a/src/coreclr/vm/ceemain.cpp +++ b/src/coreclr/vm/ceemain.cpp @@ -1764,8 +1764,7 @@ static void OsAttachThread(void* thread) if (threadFromCurrentFiber != NULL) { - _ASSERTE(!"Multiple threads encountered from a single fiber"); - COMPlusThrowWin32(); + _ASSERTE_ALL_BUILDS(!"Multiple threads encountered from a single fiber"); } // Associate the current fiber with the current thread. This makes the current fiber the thread's "home" @@ -1794,8 +1793,7 @@ bool OsDetachThread(void* thread) if (threadFromCurrentFiber != thread) { - _ASSERTE(!"Detaching a thread from the wrong fiber"); - COMPlusThrowWin32(); + _ASSERTE_ALL_BUILDS(!"Detaching a thread from the wrong fiber"); } FlsSetValue(g_flsIndex, NULL); From f4cac2d6ba0d0c41a7a3562a6dfd56a42e68e366 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Tue, 10 Dec 2024 15:34:46 -0800 Subject: [PATCH 03/20] one more case --- src/coreclr/vm/threads.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/coreclr/vm/threads.cpp b/src/coreclr/vm/threads.cpp index 69240a6ffccda5..e9ebbf98db135f 100644 --- a/src/coreclr/vm/threads.cpp +++ b/src/coreclr/vm/threads.cpp @@ -1051,8 +1051,7 @@ void InitThreadManager() #ifdef TARGET_WINDOWS if (!InitFlsSlot()) { - _ASSERTE(!"Initialization of a FLS slot failed."); - COMPlusThrowWin32(); + _ASSERTE_ALL_BUILDS(!"Initialization of a FLS slot failed."); } #endif From c2b13dd5e861c7eb2cc5a5e1fd5b795ae8d86214 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Tue, 10 Dec 2024 16:51:07 -0800 Subject: [PATCH 04/20] InitFlsSlot throws per convention used in threading initialization --- src/coreclr/vm/ceemain.cpp | 7 +++---- src/coreclr/vm/ceemain.h | 2 +- src/coreclr/vm/threads.cpp | 5 +---- 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/src/coreclr/vm/ceemain.cpp b/src/coreclr/vm/ceemain.cpp index 6ffd21e80096d3..ac1bd17de7f8ec 100644 --- a/src/coreclr/vm/ceemain.cpp +++ b/src/coreclr/vm/ceemain.cpp @@ -1741,17 +1741,16 @@ static void __stdcall FiberDetachCallback(void* lpFlsData) } } -bool InitFlsSlot() +void InitFlsSlot() { // We use fiber detach callbacks to run our thread shutdown code because the fiber detach // callback is made without the OS loader lock g_flsIndex = FlsAlloc(FiberDetachCallback); if (g_flsIndex == FLS_OUT_OF_INDEXES) { - return false; + _ASSERTE(!"Initialization of an FLS slot failed."); + COMPlusThrowWin32(); } - - return true; } // Register the thread with OS to be notified when thread is about to be destroyed diff --git a/src/coreclr/vm/ceemain.h b/src/coreclr/vm/ceemain.h index fec164ea3920b7..69ba92d692d6d2 100644 --- a/src/coreclr/vm/ceemain.h +++ b/src/coreclr/vm/ceemain.h @@ -47,7 +47,7 @@ void ThreadDetaching(); void EnsureTlsDestructionMonitor(); #ifdef TARGET_WINDOWS -bool InitFlsSlot(); +void InitFlsSlot(); bool OsDetachThread(void* thread); #endif diff --git a/src/coreclr/vm/threads.cpp b/src/coreclr/vm/threads.cpp index e9ebbf98db135f..227ca953895715 100644 --- a/src/coreclr/vm/threads.cpp +++ b/src/coreclr/vm/threads.cpp @@ -1049,10 +1049,7 @@ void InitThreadManager() CONTRACTL_END; #ifdef TARGET_WINDOWS - if (!InitFlsSlot()) - { - _ASSERTE_ALL_BUILDS(!"Initialization of a FLS slot failed."); - } + InitFlsSlot(); #endif // All patched helpers should fit into one page. From 90559bbff2f229688e9b6969afb801558fea01a2 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Tue, 10 Dec 2024 20:12:05 -0800 Subject: [PATCH 05/20] OsDetachThread could be void --- src/coreclr/vm/ceemain.cpp | 5 ++--- src/coreclr/vm/ceemain.h | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/coreclr/vm/ceemain.cpp b/src/coreclr/vm/ceemain.cpp index ac1bd17de7f8ec..d47bdd91675425 100644 --- a/src/coreclr/vm/ceemain.cpp +++ b/src/coreclr/vm/ceemain.cpp @@ -1778,7 +1778,7 @@ static void OsAttachThread(void* thread) // thread - thread to detach // Return: // true if the thread was detached, false if there was no attached thread -bool OsDetachThread(void* thread) +void OsDetachThread(void* thread) { ASSERT(g_flsIndex != FLS_OUT_OF_INDEXES); void* threadFromCurrentFiber = FlsGetValue(g_flsIndex); @@ -1787,7 +1787,7 @@ bool OsDetachThread(void* thread) { // we've seen this thread, but not this fiber. It must be a "foreign" fiber that was // borrowing this thread. - return false; + return; } if (threadFromCurrentFiber != thread) @@ -1796,7 +1796,6 @@ bool OsDetachThread(void* thread) } FlsSetValue(g_flsIndex, NULL); - return true; } void EnsureTlsDestructionMonitor() diff --git a/src/coreclr/vm/ceemain.h b/src/coreclr/vm/ceemain.h index 69ba92d692d6d2..120082d30572ca 100644 --- a/src/coreclr/vm/ceemain.h +++ b/src/coreclr/vm/ceemain.h @@ -48,7 +48,7 @@ void ThreadDetaching(); void EnsureTlsDestructionMonitor(); #ifdef TARGET_WINDOWS void InitFlsSlot(); -bool OsDetachThread(void* thread); +void OsDetachThread(void* thread); #endif void SetLatchedExitCode (INT32 code); From d62a1fc52c15b2cd8b4277fa5126ea8589a845e5 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Tue, 10 Dec 2024 21:24:36 -0800 Subject: [PATCH 06/20] Update src/coreclr/vm/ceemain.cpp --- src/coreclr/vm/ceemain.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/coreclr/vm/ceemain.cpp b/src/coreclr/vm/ceemain.cpp index d47bdd91675425..b073eca133c8fc 100644 --- a/src/coreclr/vm/ceemain.cpp +++ b/src/coreclr/vm/ceemain.cpp @@ -1748,7 +1748,6 @@ void InitFlsSlot() g_flsIndex = FlsAlloc(FiberDetachCallback); if (g_flsIndex == FLS_OUT_OF_INDEXES) { - _ASSERTE(!"Initialization of an FLS slot failed."); COMPlusThrowWin32(); } } From 98639a1121e0055f2b804a09d1d63f81ee350226 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Thu, 20 Feb 2025 10:21:19 -0800 Subject: [PATCH 07/20] ensure CoInitialize --- src/coreclr/vm/ceemain.cpp | 18 ++++++++++++++---- src/coreclr/vm/finalizerthread.cpp | 19 +++++++++++++++++++ src/coreclr/vm/finalizerthread.h | 2 ++ src/coreclr/vm/interoputil.cpp | 12 +++++++----- src/coreclr/vm/threads.cpp | 4 ---- 5 files changed, 42 insertions(+), 13 deletions(-) diff --git a/src/coreclr/vm/ceemain.cpp b/src/coreclr/vm/ceemain.cpp index b073eca133c8fc..08cc9379cc56f9 100644 --- a/src/coreclr/vm/ceemain.cpp +++ b/src/coreclr/vm/ceemain.cpp @@ -872,6 +872,10 @@ void EEStartupHelper() } #endif + // This isn't done as part of InitializeGarbageCollector() above because + // debugger must be initialized before creating EE thread objects + FinalizerThread::FinalizerThreadCreate(); + InitPreStubManager(); #ifdef FEATURE_COMINTEROP @@ -909,10 +913,6 @@ void EEStartupHelper() #endif // FEATURE_PERFTRACING GenAnalysis::Initialize(); - // This isn't done as part of InitializeGarbageCollector() above because thread - // creation requires AppDomains to have been set up. - FinalizerThread::FinalizerThreadCreate(); - // Now we really have fully initialized the garbage collector SetGarbageCollectorFullyInitialized(); @@ -964,6 +964,16 @@ void EEStartupHelper() g_MiniMetaDataBuffMaxSize, MEM_COMMIT, PAGE_READWRITE); #endif // FEATURE_MINIMETADATA_IN_TRIAGEDUMPS +#ifdef TARGET_WINDOWS +#ifdef FEATURE_COMINTEROP + // By now finalizer thread should have initialized COM.Make sure that it did. + // COM initialization will register its FLS slot. + // We need that it happened before we register ours so that COM cleanup callback + // run before ours (since COM cleanup may indirectly call managed code) + FinalizerThread::WaitForFinalizerThreadStart(); +#endif + InitFlsSlot(); +#endif g_fEEStarted = TRUE; g_EEStartupStatus = S_OK; hr = S_OK; diff --git a/src/coreclr/vm/finalizerthread.cpp b/src/coreclr/vm/finalizerthread.cpp index 08608e64b5ece9..8e5aa84fb5f7c3 100644 --- a/src/coreclr/vm/finalizerthread.cpp +++ b/src/coreclr/vm/finalizerthread.cpp @@ -385,6 +385,12 @@ DWORD WINAPI FinalizerThread::FinalizerThreadStart(void *args) _ASSERTE(s_FinalizerThreadOK); _ASSERTE(GetThread() == GetFinalizerThread()); +#if defined(TARGET_WINDOWS) && defined (FEATURE_COMINTEROP) + // handshake with EE initializating FLS slot, which should be happening after + // HasStarted called CoInitializeEx + hEventFinalizerDone->Set(); +#endif + // finalizer should always park in default domain if (s_FinalizerThreadOK) @@ -457,6 +463,10 @@ void FinalizerThread::FinalizerThreadCreate() _ASSERTE(g_pFinalizerThread == 0); g_pFinalizerThread = SetupUnstartedThread(); +#ifdef FEATURE_COMINTEROP + g_pFinalizerThread->SetApartmentOfUnstartedThread(Thread::AS_InMTA); +#endif + // We don't want the thread block disappearing under us -- even if the // actual thread terminates. GetFinalizerThread()->IncExternalCount(); @@ -491,6 +501,15 @@ void FinalizerThread::SignalFinalizationDone(int observedFullGcCount) hEventFinalizerDone->Set(); } +void FinalizerThread::WaitForFinalizerThreadStart() +{ + // this should be only called during EE startup + _ASSERTE(!g_fEEStarted); + + hEventFinalizerDone->Wait(INFINITE,FALSE); + hEventFinalizerDone->Reset(); +} + // Wait for the finalizer thread to complete one pass. void FinalizerThread::FinalizerThreadWait() { diff --git a/src/coreclr/vm/finalizerthread.h b/src/coreclr/vm/finalizerthread.h index 03aae7b4e9cf6d..eda53d2e2d7a69 100644 --- a/src/coreclr/vm/finalizerthread.h +++ b/src/coreclr/vm/finalizerthread.h @@ -65,6 +65,8 @@ class FinalizerThread } } + static void WaitForFinalizerThreadStart(); + static void FinalizerThreadWait(); static void SignalFinalizationDone(int observedFullGcCount); diff --git a/src/coreclr/vm/interoputil.cpp b/src/coreclr/vm/interoputil.cpp index ee12cd717164e3..eb7e39013a54e3 100644 --- a/src/coreclr/vm/interoputil.cpp +++ b/src/coreclr/vm/interoputil.cpp @@ -1400,14 +1400,12 @@ VOID EnsureComStarted(BOOL fCoInitCurrentThread) GC_TRIGGERS; MODE_ANY; PRECONDITION(GetThreadNULLOk() || !fCoInitCurrentThread); - PRECONDITION(g_fEEStarted); + PRECONDITION(g_fEEStarted || IsFinalizerThread()); } CONTRACTL_END; if (g_fComStarted == FALSE) { - FinalizerThread::GetFinalizerThread()->SetRequiresCoInitialize(); - // Attempt to set the thread's apartment model (to MTA by default). May not // succeed (if someone beat us to the punch). That doesn't matter (since // CLR objects are now apartment agile), we only care that a CoInitializeEx @@ -1415,8 +1413,12 @@ VOID EnsureComStarted(BOOL fCoInitCurrentThread) if (fCoInitCurrentThread) GetThread()->SetApartment(Thread::AS_InMTA); - // set the finalizer event - FinalizerThread::EnableFinalization(); + if (!IsFinalizerThread()) + { + FinalizerThread::GetFinalizerThread()->SetRequiresCoInitialize(); + // set the finalizer event + FinalizerThread::EnableFinalization(); + } g_fComStarted = TRUE; } diff --git a/src/coreclr/vm/threads.cpp b/src/coreclr/vm/threads.cpp index 227ca953895715..3924aced3b0016 100644 --- a/src/coreclr/vm/threads.cpp +++ b/src/coreclr/vm/threads.cpp @@ -1048,10 +1048,6 @@ void InitThreadManager() } CONTRACTL_END; -#ifdef TARGET_WINDOWS - InitFlsSlot(); -#endif - // All patched helpers should fit into one page. // If you hit this assert on retail build, there is most likely problem with BBT script. _ASSERTE_ALL_BUILDS((BYTE*)JIT_PatchedCodeLast - (BYTE*)JIT_PatchedCodeStart > (ptrdiff_t)0); From 092e6f6d056b875e2e5d2bcb08710560a69e30fa Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Fri, 21 Feb 2025 17:30:24 -0800 Subject: [PATCH 08/20] Asserts to fail deterministically. --- src/coreclr/vm/ceemain.cpp | 21 +++++++++++++++++++-- src/coreclr/vm/finalizerthread.cpp | 19 +++++++++++++------ src/coreclr/vm/threads.cpp | 1 + 3 files changed, 33 insertions(+), 8 deletions(-) diff --git a/src/coreclr/vm/ceemain.cpp b/src/coreclr/vm/ceemain.cpp index 08cc9379cc56f9..055512a4c79e17 100644 --- a/src/coreclr/vm/ceemain.cpp +++ b/src/coreclr/vm/ceemain.cpp @@ -972,7 +972,6 @@ void EEStartupHelper() // run before ours (since COM cleanup may indirectly call managed code) FinalizerThread::WaitForFinalizerThreadStart(); #endif - InitFlsSlot(); #endif g_fEEStarted = TRUE; g_EEStartupStatus = S_OK; @@ -1681,6 +1680,24 @@ BOOL STDMETHODCALLTYPE EEDllMain( // TRUE on success, FALSE on error. } break; } + + case DLL_THREAD_DETACH: + { + // Make sure that we do not have an attached Thread object. + // We can end up here with live Thread if some kind of thread termination callback reinitializes + // the Thread by entering managed code or by calling APIs that set up Thread, after we have already + // seen the OS premortem callback for the thread. + // The callback runs only once. There will be no thurther clean up at this point and we will likely crash in GC + // once OS clears the native TLS. + // + // NB: It is ok for the Thread to be reinitialized before the premortem OS callback runs. + // That historically may happen in rare cases when a thread main returns and cleans, but then FlsDataCleanup for COM + // ends up calling release APIs in the runtime that require Thread, or sends messages to the thread's + // managed message loop. That is ok, as long as reinitialization happens prior to the final thread termination + // callback from OS. We guarantee that by ensuring our FlsSlot is initializad after COM has already been + // initialized. + _ASSERTE_ALL_BUILDS(!GetThreadNULLOk() && "Thread reinitialized after final clean up?"); + } } } @@ -1719,7 +1736,7 @@ static void RuntimeThreadShutdown(void* thread) GCX_COOP_NO_DTOR_END(); } - pThread->DetachThread(TRUE); + pThread->DetachThread(FALSE); } else { diff --git a/src/coreclr/vm/finalizerthread.cpp b/src/coreclr/vm/finalizerthread.cpp index 8e5aa84fb5f7c3..81280d4b535f6e 100644 --- a/src/coreclr/vm/finalizerthread.cpp +++ b/src/coreclr/vm/finalizerthread.cpp @@ -380,17 +380,24 @@ DWORD WINAPI FinalizerThread::FinalizerThreadStart(void *args) LOG((LF_GC, LL_INFO10, "Finalizer thread starting...\n")); - s_FinalizerThreadOK = GetFinalizerThread()->HasStarted(); +#ifdef TARGET_WINDOWS +#ifdef FEATURE_COMINTEROP + // Making finalizer thread MTA early ensures that COM is initialized before we initialize our thread + // termination callback. + ::CoInitializeEx(NULL, COINIT_MULTITHREADED); +#endif - _ASSERTE(s_FinalizerThreadOK); - _ASSERTE(GetThread() == GetFinalizerThread()); + InitFlsSlot(); -#if defined(TARGET_WINDOWS) && defined (FEATURE_COMINTEROP) - // handshake with EE initializating FLS slot, which should be happening after - // HasStarted called CoInitializeEx + // handshake with EE initialization, as now we can attach Thread objects to native threads. hEventFinalizerDone->Set(); #endif + s_FinalizerThreadOK = GetFinalizerThread()->HasStarted(); + + _ASSERTE(s_FinalizerThreadOK); + _ASSERTE(GetThread() == GetFinalizerThread()); + // finalizer should always park in default domain if (s_FinalizerThreadOK) diff --git a/src/coreclr/vm/threads.cpp b/src/coreclr/vm/threads.cpp index 3924aced3b0016..51ee78fb7815f2 100644 --- a/src/coreclr/vm/threads.cpp +++ b/src/coreclr/vm/threads.cpp @@ -357,6 +357,7 @@ void SetThread(Thread* t) gCurrentThreadInfo.m_pThread = t; if (t != NULL) { + _ASSERTE(origThread == NULL); InitializeCurrentThreadsStaticData(t); EnsureTlsDestructionMonitor(); t->InitRuntimeThreadLocals(); From d952ae4d39bd69d7148d7c5eafd16208f35ed06e Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Fri, 21 Feb 2025 17:51:21 -0800 Subject: [PATCH 09/20] comments --- src/coreclr/vm/ceemain.cpp | 19 ++++++++----------- src/coreclr/vm/threads.cpp | 6 +++--- src/coreclr/vm/threads.h | 2 +- 3 files changed, 12 insertions(+), 15 deletions(-) diff --git a/src/coreclr/vm/ceemain.cpp b/src/coreclr/vm/ceemain.cpp index 055512a4c79e17..1c9bf9ab6601ed 100644 --- a/src/coreclr/vm/ceemain.cpp +++ b/src/coreclr/vm/ceemain.cpp @@ -965,13 +965,10 @@ void EEStartupHelper() #endif // FEATURE_MINIMETADATA_IN_TRIAGEDUMPS #ifdef TARGET_WINDOWS -#ifdef FEATURE_COMINTEROP - // By now finalizer thread should have initialized COM.Make sure that it did. - // COM initialization will register its FLS slot. - // We need that it happened before we register ours so that COM cleanup callback - // run before ours (since COM cleanup may indirectly call managed code) + // By now finalizer thread should have initialized FLS slot for thread cleanup notifications. + // And ensured that COM is initialized (must happen before allocating FLS slot). + // Make sure that this was done. FinalizerThread::WaitForFinalizerThreadStart(); -#endif #endif g_fEEStarted = TRUE; g_EEStartupStatus = S_OK; @@ -1687,13 +1684,13 @@ BOOL STDMETHODCALLTYPE EEDllMain( // TRUE on success, FALSE on error. // We can end up here with live Thread if some kind of thread termination callback reinitializes // the Thread by entering managed code or by calling APIs that set up Thread, after we have already // seen the OS premortem callback for the thread. - // The callback runs only once. There will be no thurther clean up at this point and we will likely crash in GC + // The callback runs only once. There will be no further clean up and we will likely crash in GC // once OS clears the native TLS. // // NB: It is ok for the Thread to be reinitialized before the premortem OS callback runs. - // That historically may happen in rare cases when a thread main returns and cleans, but then FlsDataCleanup for COM - // ends up calling release APIs in the runtime that require Thread, or sends messages to the thread's - // managed message loop. That is ok, as long as reinitialization happens prior to the final thread termination + // That historically may happen in rare cases when a thread cleans upon returning, but then FlsDataCleanup for COM + // ends up releasing objects and calling APIs in the runtime that require Thread, or sends messages to the thread's + // managed message loop. That is ok, as long as reinitialization happens prior to our thread termination // callback from OS. We guarantee that by ensuring our FlsSlot is initializad after COM has already been // initialized. _ASSERTE_ALL_BUILDS(!GetThreadNULLOk() && "Thread reinitialized after final clean up?"); @@ -1736,7 +1733,7 @@ static void RuntimeThreadShutdown(void* thread) GCX_COOP_NO_DTOR_END(); } - pThread->DetachThread(FALSE); + pThread->DetachThread(TRUE); } else { diff --git a/src/coreclr/vm/threads.cpp b/src/coreclr/vm/threads.cpp index 51ee78fb7815f2..98a287f28aa420 100644 --- a/src/coreclr/vm/threads.cpp +++ b/src/coreclr/vm/threads.cpp @@ -875,7 +875,7 @@ void DestroyThread(Thread *th) // Public function: DetachThread() // Marks the thread as needing to be destroyed, but doesn't destroy it yet. //------------------------------------------------------------------------- -HRESULT Thread::DetachThread(BOOL fDLLThreadDetach) +HRESULT Thread::DetachThread(BOOL inTerminationCallback) { // !!! Can not use contract here. // !!! Contract depends on Thread object for GC_TRIGGERS. @@ -900,9 +900,9 @@ HRESULT Thread::DetachThread(BOOL fDLLThreadDetach) pErrorInfo->Release(); } - // Revoke our IInitializeSpy registration only if we are not in DLL_THREAD_DETACH + // Revoke our IInitializeSpy registration only if we are not in a thread termination callback // (COM will do it or may have already done it automatically in that case). - if (!fDLLThreadDetach) + if (!inTerminationCallback) { RevokeApartmentSpy(); } diff --git a/src/coreclr/vm/threads.h b/src/coreclr/vm/threads.h index 848712141e3632..48580a13278432 100644 --- a/src/coreclr/vm/threads.h +++ b/src/coreclr/vm/threads.h @@ -678,7 +678,7 @@ class Thread }; public: - HRESULT DetachThread(BOOL fDLLThreadDetach); + HRESULT DetachThread(BOOL inTerminationCallback); void SetThreadState(ThreadState ts) { From d29fb5b2f54b6c09353c8aeb8492f9bc5828cc1b Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Sat, 22 Feb 2025 11:32:05 -0800 Subject: [PATCH 10/20] scope the failfast to Windows and tweak the comments a bit. --- src/coreclr/vm/ceemain.cpp | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/coreclr/vm/ceemain.cpp b/src/coreclr/vm/ceemain.cpp index 1c9bf9ab6601ed..16a09b7b6bb4d3 100644 --- a/src/coreclr/vm/ceemain.cpp +++ b/src/coreclr/vm/ceemain.cpp @@ -1680,20 +1680,22 @@ BOOL STDMETHODCALLTYPE EEDllMain( // TRUE on success, FALSE on error. case DLL_THREAD_DETACH: { +#ifdef TARGET_WINDOWS // Make sure that we do not have an attached Thread object. - // We can end up here with live Thread if some kind of thread termination callback reinitializes - // the Thread by entering managed code or by calling APIs that set up Thread, after we have already - // seen the OS premortem callback for the thread. - // The callback runs only once. There will be no further clean up and we will likely crash in GC - // once OS clears the native TLS. + // We can end up here with live Thread if some kind of thread termination callback initializes + // the Thread by entering managed code or by calling APIs that set up Thread, after the point where + // the OS premortem callback for the thread could have run. + // There will be no further clean up at this point and we will likely crash in GC once OS clears the native TLS. // // NB: It is ok for the Thread to be reinitialized before the premortem OS callback runs. // That historically may happen in rare cases when a thread cleans upon returning, but then FlsDataCleanup for COM // ends up releasing objects and calling APIs in the runtime that require Thread, or sends messages to the thread's - // managed message loop. That is ok, as long as reinitialization happens prior to our thread termination + // managed message loop. That is ok, as long as initialization happens prior to our thread termination // callback from OS. We guarantee that by ensuring our FlsSlot is initializad after COM has already been // initialized. - _ASSERTE_ALL_BUILDS(!GetThreadNULLOk() && "Thread reinitialized after final clean up?"); + _ASSERTE_ALL_BUILDS(!GetThreadNULLOk() && "Thread initialized after final clean up could have run."); +#endif + break; } } From 322dadc34e47ba57795fe14327ba1ae0e4742681 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Sat, 22 Feb 2025 19:42:45 -0800 Subject: [PATCH 11/20] better assert --- src/coreclr/vm/ceemain.cpp | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/src/coreclr/vm/ceemain.cpp b/src/coreclr/vm/ceemain.cpp index 16a09b7b6bb4d3..dea6b84e8c28af 100644 --- a/src/coreclr/vm/ceemain.cpp +++ b/src/coreclr/vm/ceemain.cpp @@ -1751,20 +1751,27 @@ static void RuntimeThreadShutdown(void* thread) // Index for the fiber local storage of the attached thread pointer static uint32_t g_flsIndex = FLS_OUT_OF_INDEXES; +#define FLS_STATE_CLEAR 0 +#define FLS_STATE_ARMED 1 +#define FLS_STATE_INVOKED 2 + +static __declspec(thread) int flsState; + // This is called when each *fiber* is destroyed. When the home fiber of a thread is destroyed, // it means that the thread itself is destroyed. // Since we receive that notification outside of the Loader Lock, it allows us to safely acquire // the ThreadStore lock in the RuntimeThreadShutdown. static void __stdcall FiberDetachCallback(void* lpFlsData) { - ASSERT(g_flsIndex != FLS_OUT_OF_INDEXES); - ASSERT(lpFlsData == FlsGetValue(g_flsIndex)); + _ASSERTE(g_flsIndex != FLS_OUT_OF_INDEXES); + _ASSERTE(lpFlsData); - if (lpFlsData != NULL) + if (flsState == FLS_STATE_ARMED) { - // The current fiber is the home fiber of a thread, so the thread is shutting down RuntimeThreadShutdown(lpFlsData); } + + flsState = FLS_STATE_INVOKED; } void InitFlsSlot() @@ -1784,12 +1791,9 @@ void InitFlsSlot() // thread - thread to attach static void OsAttachThread(void* thread) { - void* threadFromCurrentFiber = FlsGetValue(g_flsIndex); + _ASSERTE_ALL_BUILDS((flsState != FLS_STATE_INVOKED) && "Initializing thread after termination callback has run."); - if (threadFromCurrentFiber != NULL) - { - _ASSERTE_ALL_BUILDS(!"Multiple threads encountered from a single fiber"); - } + flsState = FLS_STATE_ARMED; // Associate the current fiber with the current thread. This makes the current fiber the thread's "home" // fiber. This fiber is the only fiber allowed to execute managed code on this thread. When this fiber @@ -1808,19 +1812,12 @@ void OsDetachThread(void* thread) ASSERT(g_flsIndex != FLS_OUT_OF_INDEXES); void* threadFromCurrentFiber = FlsGetValue(g_flsIndex); - if (threadFromCurrentFiber == NULL) - { - // we've seen this thread, but not this fiber. It must be a "foreign" fiber that was - // borrowing this thread. - return; - } - if (threadFromCurrentFiber != thread) { _ASSERTE_ALL_BUILDS(!"Detaching a thread from the wrong fiber"); } - FlsSetValue(g_flsIndex, NULL); + flsState = FLS_STATE_CLEAR; } void EnsureTlsDestructionMonitor() From 2593989a354192a86f6ecfeefb63e57f5abde575 Mon Sep 17 00:00:00 2001 From: Vladimir Sadov Date: Sun, 23 Feb 2025 09:20:44 -0800 Subject: [PATCH 12/20] Apply suggestions from code review Co-authored-by: Jan Kotas --- src/coreclr/vm/ceemain.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/ceemain.cpp b/src/coreclr/vm/ceemain.cpp index dea6b84e8c28af..0d6e61137a0bf5 100644 --- a/src/coreclr/vm/ceemain.cpp +++ b/src/coreclr/vm/ceemain.cpp @@ -1755,7 +1755,7 @@ static uint32_t g_flsIndex = FLS_OUT_OF_INDEXES; #define FLS_STATE_ARMED 1 #define FLS_STATE_INVOKED 2 -static __declspec(thread) int flsState; +static __declspec(thread) byte flsState; // This is called when each *fiber* is destroyed. When the home fiber of a thread is destroyed, // it means that the thread itself is destroyed. @@ -1791,7 +1791,10 @@ void InitFlsSlot() // thread - thread to attach static void OsAttachThread(void* thread) { - _ASSERTE_ALL_BUILDS((flsState != FLS_STATE_INVOKED) && "Initializing thread after termination callback has run."); + if (flsState != FLS_STATE_INVOKED) + { + _ASSERTE_ALL_BUILDS(!"Attempt to execute managed code after the .NET runtime thread state has been destroyed."); + } flsState = FLS_STATE_ARMED; From 1612a8e9b77d150d3392c76be06f540f71e970ac Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Sun, 23 Feb 2025 09:55:55 -0800 Subject: [PATCH 13/20] Undo unnecessary finalizer thread changes --- src/coreclr/vm/finalizerthread.cpp | 4 ---- src/coreclr/vm/interoputil.cpp | 12 +++++------- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/src/coreclr/vm/finalizerthread.cpp b/src/coreclr/vm/finalizerthread.cpp index 81280d4b535f6e..0b20d77d2ae1b3 100644 --- a/src/coreclr/vm/finalizerthread.cpp +++ b/src/coreclr/vm/finalizerthread.cpp @@ -470,10 +470,6 @@ void FinalizerThread::FinalizerThreadCreate() _ASSERTE(g_pFinalizerThread == 0); g_pFinalizerThread = SetupUnstartedThread(); -#ifdef FEATURE_COMINTEROP - g_pFinalizerThread->SetApartmentOfUnstartedThread(Thread::AS_InMTA); -#endif - // We don't want the thread block disappearing under us -- even if the // actual thread terminates. GetFinalizerThread()->IncExternalCount(); diff --git a/src/coreclr/vm/interoputil.cpp b/src/coreclr/vm/interoputil.cpp index eb7e39013a54e3..ee12cd717164e3 100644 --- a/src/coreclr/vm/interoputil.cpp +++ b/src/coreclr/vm/interoputil.cpp @@ -1400,12 +1400,14 @@ VOID EnsureComStarted(BOOL fCoInitCurrentThread) GC_TRIGGERS; MODE_ANY; PRECONDITION(GetThreadNULLOk() || !fCoInitCurrentThread); - PRECONDITION(g_fEEStarted || IsFinalizerThread()); + PRECONDITION(g_fEEStarted); } CONTRACTL_END; if (g_fComStarted == FALSE) { + FinalizerThread::GetFinalizerThread()->SetRequiresCoInitialize(); + // Attempt to set the thread's apartment model (to MTA by default). May not // succeed (if someone beat us to the punch). That doesn't matter (since // CLR objects are now apartment agile), we only care that a CoInitializeEx @@ -1413,12 +1415,8 @@ VOID EnsureComStarted(BOOL fCoInitCurrentThread) if (fCoInitCurrentThread) GetThread()->SetApartment(Thread::AS_InMTA); - if (!IsFinalizerThread()) - { - FinalizerThread::GetFinalizerThread()->SetRequiresCoInitialize(); - // set the finalizer event - FinalizerThread::EnableFinalization(); - } + // set the finalizer event + FinalizerThread::EnableFinalization(); g_fComStarted = TRUE; } From 2d6c736811eadf8f61f3536432d565e8dfecb496 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Sun, 23 Feb 2025 09:59:49 -0800 Subject: [PATCH 14/20] reverse the failfast condition --- src/coreclr/vm/ceemain.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/ceemain.cpp b/src/coreclr/vm/ceemain.cpp index 0d6e61137a0bf5..9879b6a418c65b 100644 --- a/src/coreclr/vm/ceemain.cpp +++ b/src/coreclr/vm/ceemain.cpp @@ -1791,7 +1791,7 @@ void InitFlsSlot() // thread - thread to attach static void OsAttachThread(void* thread) { - if (flsState != FLS_STATE_INVOKED) + if (flsState == FLS_STATE_INVOKED) { _ASSERTE_ALL_BUILDS(!"Attempt to execute managed code after the .NET runtime thread state has been destroyed."); } From a1f374a98fb4cc477500cd51036fe7695e9ce9ac Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Sun, 23 Feb 2025 15:12:45 -0800 Subject: [PATCH 15/20] handle destruction of unattached threads --- src/coreclr/vm/ceemain.cpp | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/coreclr/vm/ceemain.cpp b/src/coreclr/vm/ceemain.cpp index 9879b6a418c65b..f9135e6f9bac79 100644 --- a/src/coreclr/vm/ceemain.cpp +++ b/src/coreclr/vm/ceemain.cpp @@ -1801,6 +1801,7 @@ static void OsAttachThread(void* thread) // Associate the current fiber with the current thread. This makes the current fiber the thread's "home" // fiber. This fiber is the only fiber allowed to execute managed code on this thread. When this fiber // is destroyed, we consider the thread to be destroyed. + _ASSERTE(thread != NULL); FlsSetValue(g_flsIndex, thread); } @@ -1815,11 +1816,22 @@ void OsDetachThread(void* thread) ASSERT(g_flsIndex != FLS_OUT_OF_INDEXES); void* threadFromCurrentFiber = FlsGetValue(g_flsIndex); + if (threadFromCurrentFiber == NULL) + { + // Thread is not attached. + // This could come from DestroyThread called when refcount reaches 0 + // and the thread may have already been detached or never attached. + // We leave flsState as-is to keep track whether our callback has been called. + return; + } + if (threadFromCurrentFiber != thread) { _ASSERTE_ALL_BUILDS(!"Detaching a thread from the wrong fiber"); } + // Leave the existing FLS value, to keep the callback "armed" so that we could observe the termination callback. + // After that we will not allow to attach as we will no longer be able to clean up. flsState = FLS_STATE_CLEAR; } From 2aa70a519d8087cea1a3b1ae2477b6ef45e80dbf Mon Sep 17 00:00:00 2001 From: Vladimir Sadov Date: Mon, 24 Feb 2025 16:47:48 -0800 Subject: [PATCH 16/20] PR feedback Co-authored-by: Jan Kotas --- src/coreclr/vm/ceemain.cpp | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/src/coreclr/vm/ceemain.cpp b/src/coreclr/vm/ceemain.cpp index f9135e6f9bac79..29422d6c9d9658 100644 --- a/src/coreclr/vm/ceemain.cpp +++ b/src/coreclr/vm/ceemain.cpp @@ -1677,26 +1677,6 @@ BOOL STDMETHODCALLTYPE EEDllMain( // TRUE on success, FALSE on error. } break; } - - case DLL_THREAD_DETACH: - { -#ifdef TARGET_WINDOWS - // Make sure that we do not have an attached Thread object. - // We can end up here with live Thread if some kind of thread termination callback initializes - // the Thread by entering managed code or by calling APIs that set up Thread, after the point where - // the OS premortem callback for the thread could have run. - // There will be no further clean up at this point and we will likely crash in GC once OS clears the native TLS. - // - // NB: It is ok for the Thread to be reinitialized before the premortem OS callback runs. - // That historically may happen in rare cases when a thread cleans upon returning, but then FlsDataCleanup for COM - // ends up releasing objects and calling APIs in the runtime that require Thread, or sends messages to the thread's - // managed message loop. That is ok, as long as initialization happens prior to our thread termination - // callback from OS. We guarantee that by ensuring our FlsSlot is initializad after COM has already been - // initialized. - _ASSERTE_ALL_BUILDS(!GetThreadNULLOk() && "Thread initialized after final clean up could have run."); -#endif - break; - } } } From 82f3b5572e0846823807dc80b9282ed928944e40 Mon Sep 17 00:00:00 2001 From: Vladimir Sadov Date: Mon, 24 Feb 2025 19:55:35 -0800 Subject: [PATCH 17/20] Update src/coreclr/vm/ceemain.cpp --- src/coreclr/vm/ceemain.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/coreclr/vm/ceemain.cpp b/src/coreclr/vm/ceemain.cpp index 29422d6c9d9658..bf2f778ffc0285 100644 --- a/src/coreclr/vm/ceemain.cpp +++ b/src/coreclr/vm/ceemain.cpp @@ -1789,8 +1789,6 @@ static void OsAttachThread(void* thread) // It fails fast if some other thread value was attached to the current fiber. // Parameters: // thread - thread to detach -// Return: -// true if the thread was detached, false if there was no attached thread void OsDetachThread(void* thread) { ASSERT(g_flsIndex != FLS_OUT_OF_INDEXES); From 9bfcada8a3e7c2c5b4e779e07c4b80a52134e65f Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Mon, 24 Feb 2025 21:12:44 -0800 Subject: [PATCH 18/20] set g_fComStarted as soon as we call CoInitialize --- src/coreclr/vm/finalizerthread.cpp | 1 + src/coreclr/vm/interoputil.cpp | 29 ++++------------------------- 2 files changed, 5 insertions(+), 25 deletions(-) diff --git a/src/coreclr/vm/finalizerthread.cpp b/src/coreclr/vm/finalizerthread.cpp index 0b20d77d2ae1b3..4194804bc18e21 100644 --- a/src/coreclr/vm/finalizerthread.cpp +++ b/src/coreclr/vm/finalizerthread.cpp @@ -385,6 +385,7 @@ DWORD WINAPI FinalizerThread::FinalizerThreadStart(void *args) // Making finalizer thread MTA early ensures that COM is initialized before we initialize our thread // termination callback. ::CoInitializeEx(NULL, COINIT_MULTITHREADED); + g_fComStarted = true; #endif InitFlsSlot(); diff --git a/src/coreclr/vm/interoputil.cpp b/src/coreclr/vm/interoputil.cpp index ee12cd717164e3..571da19c40b5ef 100644 --- a/src/coreclr/vm/interoputil.cpp +++ b/src/coreclr/vm/interoputil.cpp @@ -1404,22 +1404,8 @@ VOID EnsureComStarted(BOOL fCoInitCurrentThread) } CONTRACTL_END; - if (g_fComStarted == FALSE) - { - FinalizerThread::GetFinalizerThread()->SetRequiresCoInitialize(); - - // Attempt to set the thread's apartment model (to MTA by default). May not - // succeed (if someone beat us to the punch). That doesn't matter (since - // CLR objects are now apartment agile), we only care that a CoInitializeEx - // has been performed on this thread by us. - if (fCoInitCurrentThread) - GetThread()->SetApartment(Thread::AS_InMTA); - - // set the finalizer event - FinalizerThread::EnableFinalization(); - - g_fComStarted = TRUE; - } + // COM should have been stated as soon as we start finalizer thread. + _ASSERTE(g_fComStarted); } HRESULT EnsureComStartedNoThrow(BOOL fCoInitCurrentThread) @@ -1436,15 +1422,8 @@ HRESULT EnsureComStartedNoThrow(BOOL fCoInitCurrentThread) HRESULT hr = S_OK; - if (!g_fComStarted) - { - GCX_COOP(); - EX_TRY - { - EnsureComStarted(fCoInitCurrentThread); - } - EX_CATCH_HRESULT(hr); - } + // COM should have been stated as soon as we start finalizer thread. + _ASSERTE(g_fComStarted); return hr; } From 65eb637379c717b52ece4a674f0daf7e1844afe0 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Mon, 24 Feb 2025 21:20:43 -0800 Subject: [PATCH 19/20] Naming nit --- src/coreclr/vm/ceemain.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/coreclr/vm/ceemain.cpp b/src/coreclr/vm/ceemain.cpp index bf2f778ffc0285..95001c07cc001e 100644 --- a/src/coreclr/vm/ceemain.cpp +++ b/src/coreclr/vm/ceemain.cpp @@ -1735,7 +1735,7 @@ static uint32_t g_flsIndex = FLS_OUT_OF_INDEXES; #define FLS_STATE_ARMED 1 #define FLS_STATE_INVOKED 2 -static __declspec(thread) byte flsState; +static __declspec(thread) byte t_flsState; // This is called when each *fiber* is destroyed. When the home fiber of a thread is destroyed, // it means that the thread itself is destroyed. @@ -1746,12 +1746,12 @@ static void __stdcall FiberDetachCallback(void* lpFlsData) _ASSERTE(g_flsIndex != FLS_OUT_OF_INDEXES); _ASSERTE(lpFlsData); - if (flsState == FLS_STATE_ARMED) + if (t_flsState == FLS_STATE_ARMED) { RuntimeThreadShutdown(lpFlsData); } - flsState = FLS_STATE_INVOKED; + t_flsState = FLS_STATE_INVOKED; } void InitFlsSlot() @@ -1771,12 +1771,12 @@ void InitFlsSlot() // thread - thread to attach static void OsAttachThread(void* thread) { - if (flsState == FLS_STATE_INVOKED) + if (t_flsState == FLS_STATE_INVOKED) { _ASSERTE_ALL_BUILDS(!"Attempt to execute managed code after the .NET runtime thread state has been destroyed."); } - flsState = FLS_STATE_ARMED; + t_flsState = FLS_STATE_ARMED; // Associate the current fiber with the current thread. This makes the current fiber the thread's "home" // fiber. This fiber is the only fiber allowed to execute managed code on this thread. When this fiber @@ -1799,7 +1799,7 @@ void OsDetachThread(void* thread) // Thread is not attached. // This could come from DestroyThread called when refcount reaches 0 // and the thread may have already been detached or never attached. - // We leave flsState as-is to keep track whether our callback has been called. + // We leave t_flsState as-is to keep track whether our callback has been called. return; } @@ -1810,7 +1810,7 @@ void OsDetachThread(void* thread) // Leave the existing FLS value, to keep the callback "armed" so that we could observe the termination callback. // After that we will not allow to attach as we will no longer be able to clean up. - flsState = FLS_STATE_CLEAR; + t_flsState = FLS_STATE_CLEAR; } void EnsureTlsDestructionMonitor() From 45fcc478fabc865bd33a54461973b280dcc09f7e Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Mon, 24 Feb 2025 21:27:34 -0800 Subject: [PATCH 20/20] fix a typpo + better comment --- src/coreclr/vm/interoputil.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/interoputil.cpp b/src/coreclr/vm/interoputil.cpp index 571da19c40b5ef..858d8d55776aa0 100644 --- a/src/coreclr/vm/interoputil.cpp +++ b/src/coreclr/vm/interoputil.cpp @@ -1404,7 +1404,7 @@ VOID EnsureComStarted(BOOL fCoInitCurrentThread) } CONTRACTL_END; - // COM should have been stated as soon as we start finalizer thread. + // COM is expected to be started on finalizer thread during startup _ASSERTE(g_fComStarted); } @@ -1422,7 +1422,7 @@ HRESULT EnsureComStartedNoThrow(BOOL fCoInitCurrentThread) HRESULT hr = S_OK; - // COM should have been stated as soon as we start finalizer thread. + // COM is expected to be started on finalizer thread during startup _ASSERTE(g_fComStarted); return hr;