From 878cd5d88fc38fe0131455c84a6793c562238f70 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Mon, 20 Jun 2016 11:57:00 +0200 Subject: Fix GC background thread start in OOM (#5840) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This change fixes a problem that happened when the `gc_heap::create_bgc_thread` calls SetupUnstartedThread and it fails to allocate a Thread object and throws. The GC doesn't expect that and so when the stack is unwound, the `gc_heap::gc_started` stays set. Now we try to allocate memory for the Throwable for the exception, so we go to GC heap and since there is not enough space, we end up calling `gc_heap::try_allocate_more_space`, which calls `gc_heap::wait_for_gc_done`. And that’s the end of it, since the `gc_started` is still set and we wait forever. The fix is to catch exception from the SetupUnstartedThread. I have also fixed couple of places where this method was being called and the exception was not expected. As an additional cleanup, I have moved the thread creation in GC to the GCToEEInterface. --- src/vm/assembly.cpp | 2 -- src/vm/ceemain.cpp | 3 +-- src/vm/finalizerthread.cpp | 25 ++++++++----------------- src/vm/finalizerthread.h | 2 +- src/vm/gcenv.ee.cpp | 36 ++++++++++++++++++++++++++++++++++++ src/vm/threads.cpp | 9 +++------ 6 files changed, 49 insertions(+), 28 deletions(-) (limited to 'src/vm') diff --git a/src/vm/assembly.cpp b/src/vm/assembly.cpp index 3de92206d4..06effd7240 100644 --- a/src/vm/assembly.cpp +++ b/src/vm/assembly.cpp @@ -2343,8 +2343,6 @@ static void Stress_Thread_Start (LPVOID lpParameter) for (n = 0; n < dwThreads-1; n ++) { threads[n] = SetupUnstartedThread(); - if (threads[n] == NULL) - COMPlusThrowOM(); threads[n]->m_stressThreadCount = dwThreads/2; Stress_Thread_Param *param = ((Stress_Thread_Param*)lpParameter)->Clone(); diff --git a/src/vm/ceemain.cpp b/src/vm/ceemain.cpp index 9a54adf7b7..f3e8c3b28d 100644 --- a/src/vm/ceemain.cpp +++ b/src/vm/ceemain.cpp @@ -3727,8 +3727,7 @@ void InitializeGarbageCollector() IfFailThrow(hr); // Thread for running finalizers... - if (FinalizerThread::FinalizerThreadCreate() != 1) - ThrowOutOfMemory(); + FinalizerThread::FinalizerThreadCreate(); // Now we really have fully initialized the garbage collector SetGarbageCollectorFullyInitialized(); diff --git a/src/vm/finalizerthread.cpp b/src/vm/finalizerthread.cpp index 3960719c65..5d51d33cfb 100644 --- a/src/vm/finalizerthread.cpp +++ b/src/vm/finalizerthread.cpp @@ -994,13 +994,13 @@ DWORD __stdcall FinalizerThread::FinalizerThreadStart(void *args) return 0; } -DWORD FinalizerThread::FinalizerThreadCreate() +void FinalizerThread::FinalizerThreadCreate() { - DWORD dwRet = 0; - - // TODO: The following line should be removed after contract violation is fixed. - // See bug 27409 - SCAN_IGNORE_THROW; + CONTRACTL{ + THROWS; + GC_TRIGGERS; + MODE_ANY; + } CONTRACTL_END; #ifndef FEATURE_PAL if (!CLRMemoryHosted()) @@ -1021,9 +1021,6 @@ DWORD FinalizerThread::FinalizerThreadCreate() _ASSERTE(g_pFinalizerThread == 0); g_pFinalizerThread = SetupUnstartedThread(); - if (g_pFinalizerThread == 0) { - return 0; - } // We don't want the thread block disappearing under us -- even if the // actual thread terminates. @@ -1031,7 +1028,7 @@ DWORD FinalizerThread::FinalizerThreadCreate() if (GetFinalizerThread()->CreateNewThread(0, &FinalizerThreadStart, NULL)) { - dwRet = GetFinalizerThread()->StartThread(); + DWORD dwRet = GetFinalizerThread()->StartThread(); // When running under a user mode native debugger there is a race // between the moment we've created the thread (in CreateNewThread) and @@ -1046,10 +1043,6 @@ DWORD FinalizerThread::FinalizerThreadCreate() // debugger may have been detached between the time it got the notification // and the moment we execute the test below. _ASSERTE(dwRet == 1 || dwRet == 2); - if (dwRet == 2) - { - dwRet = 1; - } // workaround wwl: make sure finalizer is ready. This avoids OOM problem on finalizer // thread startup. @@ -1057,12 +1050,10 @@ DWORD FinalizerThread::FinalizerThreadCreate() FinalizerThreadWait(INFINITE); if (!s_FinalizerThreadOK) { - dwRet = 0; + ThrowOutOfMemory(); } } } - - return dwRet; } void FinalizerThread::SignalFinalizationDone(BOOL fFinalizer) diff --git a/src/vm/finalizerthread.h b/src/vm/finalizerthread.h index 42a9e9f250..90dd3e9ec9 100644 --- a/src/vm/finalizerthread.h +++ b/src/vm/finalizerthread.h @@ -91,7 +91,7 @@ public: static void FinalizeObjectsOnShutdown(LPVOID args); static DWORD __stdcall FinalizerThreadStart(void *args); - static DWORD FinalizerThreadCreate(); + static void FinalizerThreadCreate(); static BOOL FinalizerThreadWatchDog(); }; diff --git a/src/vm/gcenv.ee.cpp b/src/vm/gcenv.ee.cpp index 7cce1716ae..1bb4ee815b 100644 --- a/src/vm/gcenv.ee.cpp +++ b/src/vm/gcenv.ee.cpp @@ -746,3 +746,39 @@ void GCToEEInterface::DisablePreemptiveGC(Thread * pThread) WRAPPER_NO_CONTRACT; pThread->DisablePreemptiveGC(); } + +bool GCToEEInterface::CreateBackgroundThread(Thread** thread, GCBackgroundThreadFunction threadStart, void* arg) +{ + CONTRACTL + { + NOTHROW; + GC_TRIGGERS; + } + CONTRACTL_END; + + Thread* newThread = NULL; + EX_TRY + { + newThread = SetupUnstartedThread(FALSE); + *thread = newThread; + } + EX_CATCH + { + } + EX_END_CATCH(SwallowAllExceptions); + + if ((newThread != NULL) && newThread->CreateNewThread(0, (LPTHREAD_START_ROUTINE)threadStart, arg)) + { + newThread->SetBackground (TRUE, FALSE); + + // wait for the thread to be in its main loop, this is to detect the situation + // where someone triggers a GC during dll loading where the loader lock is + // held. + newThread->StartThread(); + + return true; + } + + *thread = NULL; + return false; +} diff --git a/src/vm/threads.cpp b/src/vm/threads.cpp index 9e1a528370..5624bb1026 100644 --- a/src/vm/threads.cpp +++ b/src/vm/threads.cpp @@ -1118,13 +1118,10 @@ Thread* SetupUnstartedThread(BOOL bRequiresTSL) _ASSERTE(ThreadInited()); Thread* pThread = new Thread(); - if (pThread) - { - FastInterlockOr((ULONG *) &pThread->m_State, - (Thread::TS_Unstarted | Thread::TS_WeOwn)); + FastInterlockOr((ULONG *) &pThread->m_State, + (Thread::TS_Unstarted | Thread::TS_WeOwn)); - ThreadStore::AddThread(pThread, bRequiresTSL); - } + ThreadStore::AddThread(pThread, bRequiresTSL); return pThread; } -- cgit v1.2.3