From de83f003fc9bac17170a8d7a0b0deeb95e080a7b Mon Sep 17 00:00:00 2001 From: David Mason Date: Thu, 25 Feb 2021 00:13:12 -0800 Subject: [PATCH 1/4] Fix race condition in ReleaseOnDetach test --- .../releaseondetach/releaseondetach.cpp | 27 ++++++++++++++++--- .../native/releaseondetach/releaseondetach.h | 9 +++---- .../profiler/unittest/releaseondetach.cs | 19 ++++++------- 3 files changed, 34 insertions(+), 21 deletions(-) diff --git a/src/tests/profiler/native/releaseondetach/releaseondetach.cpp b/src/tests/profiler/native/releaseondetach/releaseondetach.cpp index 8f731d36f36ee3..417c302fd68491 100644 --- a/src/tests/profiler/native/releaseondetach/releaseondetach.cpp +++ b/src/tests/profiler/native/releaseondetach/releaseondetach.cpp @@ -10,6 +10,8 @@ #include #include #include +#include +#include using std::string; using std::ifstream; @@ -51,6 +53,17 @@ ReleaseOnDetach::~ReleaseOnDetach() fflush(stdout); + + for (int i = 0; i < 50000; ++i) + { + if (_doneFlag != NULL) + { + break; + } + + std::this_thread::sleep_for(std::chrono::milliseconds(1)); + } + if (_doneFlag != NULL) { *_doneFlag = true; @@ -113,7 +126,7 @@ HRESULT ReleaseOnDetach::ProfilerAttachComplete() return S_OK; } -HRESULT STDMETHODCALLTYPE ReleaseOnDetach::ProfilerDetachSucceeded() +HRESULT ReleaseOnDetach::ProfilerDetachSucceeded() { SHUTDOWNGUARD(); @@ -122,7 +135,13 @@ HRESULT STDMETHODCALLTYPE ReleaseOnDetach::ProfilerDetachSucceeded() return S_OK; } -extern "C" EXPORT void STDMETHODCALLTYPE PassBoolToProfiler(void *boolPtr) +void ReleaseOnDetach::SetBoolPtr(bool *done) { - ReleaseOnDetach::Instance->SetBoolPtr(boolPtr); -} \ No newline at end of file + assert(done != NULL); + _doneFlag = reinterpret_cast(done); +} + +extern "C" EXPORT void STDMETHODCALLTYPE PassBoolToProfiler(bool *done) +{ + ReleaseOnDetach::Instance->SetBoolPtr(done); +} diff --git a/src/tests/profiler/native/releaseondetach/releaseondetach.h b/src/tests/profiler/native/releaseondetach/releaseondetach.h index f545feddb53533..5baba46b440a31 100644 --- a/src/tests/profiler/native/releaseondetach/releaseondetach.h +++ b/src/tests/profiler/native/releaseondetach/releaseondetach.h @@ -6,6 +6,7 @@ #include "../profiler.h" #include +#include typedef HRESULT (*GetDispenserFunc) (const CLSID &pClsid, const IID &pIid, void **ppv); @@ -32,11 +33,7 @@ class ReleaseOnDetach : public Profiler virtual HRESULT STDMETHODCALLTYPE ProfilerAttachComplete(); virtual HRESULT STDMETHODCALLTYPE ProfilerDetachSucceeded(); - void SetBoolPtr(void *ptr) - { - assert(ptr != NULL); - _doneFlag = reinterpret_cast(ptr); - } + void SetBoolPtr(bool *done); private: @@ -45,5 +42,5 @@ class ReleaseOnDetach : public Profiler IMetaDataDispenserEx* _dispenser; std::atomic _failures; bool _detachSucceeded; - volatile bool *_doneFlag; + std::atomic _doneFlag; }; diff --git a/src/tests/profiler/unittest/releaseondetach.cs b/src/tests/profiler/unittest/releaseondetach.cs index 064a6a8e380f49..54d5d360829216 100644 --- a/src/tests/profiler/unittest/releaseondetach.cs +++ b/src/tests/profiler/unittest/releaseondetach.cs @@ -18,30 +18,27 @@ class ReleaseOnShutdown static volatile bool _profilerDone = false; [DllImport("Profiler")] - private static extern void PassBoolToProfiler(IntPtr boolPtr); + private static extern unsafe void PassBoolToProfiler(bool *done); [MethodImpl(MethodImplOptions.NoInlining)] - public static void WasteTime() + public static void WaitForDetach() { // Give time for the profiler to detach Console.WriteLine("Waiting for profiler to detach..."); bool profilerSetFlag = false; - for (int i = 0; i < 100_000; ++i) + for (int i = 0; i < 50_000; ++i) { Thread.Sleep(TimeSpan.FromMilliseconds(1)); if (_profilerDone) { profilerSetFlag = true; - break; + return; } } - if (!profilerSetFlag) - { - Console.WriteLine("Warning: test will fail because the profiler never had its destructor called."); - } + Console.WriteLine("Warning: test will fail because the profiler never had its destructor called."); } - + public unsafe static int RunTest(string[] args) { string profilerName; @@ -70,9 +67,9 @@ public unsafe static int RunTest(string[] args) #pragma warning disable CS0420 fixed (bool *boolPtr = &_profilerDone) { - PassBoolToProfiler(new IntPtr(boolPtr)); + PassBoolToProfiler(boolPtr); - WasteTime(); + WaitForDetach(); } return 100; From 0d23f9648f1e13593215ada8e36a06814c33ef89 Mon Sep 17 00:00:00 2001 From: David Mason Date: Thu, 25 Feb 2021 12:10:45 -0800 Subject: [PATCH 2/4] Get rid of sleeps --- src/tests/profiler/native/profiler.def | 2 +- .../releaseondetach/releaseondetach.cpp | 37 +++++++++--------- .../native/releaseondetach/releaseondetach.h | 7 ++-- .../profiler/unittest/releaseondetach.cs | 38 +++---------------- 4 files changed, 31 insertions(+), 53 deletions(-) diff --git a/src/tests/profiler/native/profiler.def b/src/tests/profiler/native/profiler.def index 38cdf6c0f5af16..37a59858dbfe0a 100644 --- a/src/tests/profiler/native/profiler.def +++ b/src/tests/profiler/native/profiler.def @@ -3,6 +3,6 @@ LIBRARY Profiler.dll EXPORTS DllGetClassObject PRIVATE DllCanUnloadNow PRIVATE - PassBoolToProfiler PRIVATE + PassCallbackToProfiler PRIVATE DoPInvoke PRIVATE diff --git a/src/tests/profiler/native/releaseondetach/releaseondetach.cpp b/src/tests/profiler/native/releaseondetach/releaseondetach.cpp index 417c302fd68491..9b8c2589a502f5 100644 --- a/src/tests/profiler/native/releaseondetach/releaseondetach.cpp +++ b/src/tests/profiler/native/releaseondetach/releaseondetach.cpp @@ -22,13 +22,16 @@ using std::getline; #endif // __APPLE__ #endif // WIN32 +using std::thread; + ReleaseOnDetach *ReleaseOnDetach::Instance; ReleaseOnDetach::ReleaseOnDetach() : _dispenser(NULL), _failures(0), _detachSucceeded(false), - _doneFlag(NULL) + _callback(NULL), + _callbackSet() { ReleaseOnDetach::Instance = this; } @@ -54,20 +57,19 @@ ReleaseOnDetach::~ReleaseOnDetach() fflush(stdout); - for (int i = 0; i < 50000; ++i) - { - if (_doneFlag != NULL) - { - break; - } + _callbackSet.Wait(); - std::this_thread::sleep_for(std::chrono::milliseconds(1)); - } - if (_doneFlag != NULL) + thread callbackThread([&]() { - *_doneFlag = true; - } + // The destructor will be called from the profiler detach thread, which causes + // some crst order asserts if we call back in to managed code. Spin up + // a new thread to avoid that. + pCorProfilerInfo->InitializeCurrentThread(); + _callback(); + }); + + callbackThread.join(); } GUID ReleaseOnDetach::GetClsid() @@ -135,13 +137,14 @@ HRESULT ReleaseOnDetach::ProfilerDetachSucceeded() return S_OK; } -void ReleaseOnDetach::SetBoolPtr(bool *done) +void ReleaseOnDetach::SetCallback(ProfilerCallback callback) { - assert(done != NULL); - _doneFlag = reinterpret_cast(done); + assert(callback != NULL); + _callback = callback; + _callbackSet.Signal(); } -extern "C" EXPORT void STDMETHODCALLTYPE PassBoolToProfiler(bool *done) +extern "C" EXPORT void STDMETHODCALLTYPE PassCallbackToProfiler(ProfilerCallback callback) { - ReleaseOnDetach::Instance->SetBoolPtr(done); + ReleaseOnDetach::Instance->SetCallback(callback); } diff --git a/src/tests/profiler/native/releaseondetach/releaseondetach.h b/src/tests/profiler/native/releaseondetach/releaseondetach.h index 5baba46b440a31..643c915279610c 100644 --- a/src/tests/profiler/native/releaseondetach/releaseondetach.h +++ b/src/tests/profiler/native/releaseondetach/releaseondetach.h @@ -8,7 +8,7 @@ #include #include -typedef HRESULT (*GetDispenserFunc) (const CLSID &pClsid, const IID &pIid, void **ppv); +typedef void (*ProfilerCallback) (void); // This test class is very small and doesn't do much. A repeated problem we had was that // if an ICorProfilerCallback* interface was added the developer would forget to add @@ -33,7 +33,7 @@ class ReleaseOnDetach : public Profiler virtual HRESULT STDMETHODCALLTYPE ProfilerAttachComplete(); virtual HRESULT STDMETHODCALLTYPE ProfilerDetachSucceeded(); - void SetBoolPtr(bool *done); + void SetCallback(ProfilerCallback callback); private: @@ -42,5 +42,6 @@ class ReleaseOnDetach : public Profiler IMetaDataDispenserEx* _dispenser; std::atomic _failures; bool _detachSucceeded; - std::atomic _doneFlag; + ProfilerCallback _callback; + ManualEvent _callbackSet; }; diff --git a/src/tests/profiler/unittest/releaseondetach.cs b/src/tests/profiler/unittest/releaseondetach.cs index 54d5d360829216..9e99801c332105 100644 --- a/src/tests/profiler/unittest/releaseondetach.cs +++ b/src/tests/profiler/unittest/releaseondetach.cs @@ -13,31 +13,12 @@ namespace Profiler.Tests class ReleaseOnShutdown { - static readonly Guid ReleaseOnShutdownGuid = new Guid("B8C47A29-9C1D-4EEA-ABA0-8E8B3E3B792E"); + private static readonly Guid ReleaseOnShutdownGuid = new Guid("B8C47A29-9C1D-4EEA-ABA0-8E8B3E3B792E"); - static volatile bool _profilerDone = false; + private static ManualResetEvent _profilerDone; [DllImport("Profiler")] - private static extern unsafe void PassBoolToProfiler(bool *done); - - [MethodImpl(MethodImplOptions.NoInlining)] - public static void WaitForDetach() - { - // Give time for the profiler to detach - Console.WriteLine("Waiting for profiler to detach..."); - bool profilerSetFlag = false; - for (int i = 0; i < 50_000; ++i) - { - Thread.Sleep(TimeSpan.FromMilliseconds(1)); - if (_profilerDone) - { - profilerSetFlag = true; - return; - } - } - - Console.WriteLine("Warning: test will fail because the profiler never had its destructor called."); - } + private static extern void PassCallbackToProfiler(ProfilerCallback callback); public unsafe static int RunTest(string[] args) { @@ -58,19 +39,12 @@ public unsafe static int RunTest(string[] args) string rootPath = Path.GetDirectoryName(System.Reflection.Assembly.GetExecutingAssembly().Location); string profilerPath = Path.Combine(rootPath, profilerName); + _profilerDone = new ManualResetEvent(false); Console.WriteLine($"Attaching profiler {profilerPath} to self."); ProfilerControlHelpers.AttachProfilerToSelf(ReleaseOnShutdownGuid, profilerPath); - // This warning is that the pointer to the volatile bool won't be treated as volatile, - // but that's ok. The loop aboive in WastTime is what needs to read it as volatile. - // The native part just sets it. - #pragma warning disable CS0420 - fixed (bool *boolPtr = &_profilerDone) - { - PassBoolToProfiler(boolPtr); - - WaitForDetach(); - } + PassCallbackToProfiler(() => _profilerDone.Set()); + _profilerDone.WaitOne(); return 100; } From 4f418697c6a66d2f9af659617242b6e3f4491512 Mon Sep 17 00:00:00 2001 From: David Mason Date: Thu, 25 Feb 2021 12:30:50 -0800 Subject: [PATCH 3/4] Add timeout for waiting on event --- src/tests/profiler/unittest/releaseondetach.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/tests/profiler/unittest/releaseondetach.cs b/src/tests/profiler/unittest/releaseondetach.cs index 9e99801c332105..922446d5da0082 100644 --- a/src/tests/profiler/unittest/releaseondetach.cs +++ b/src/tests/profiler/unittest/releaseondetach.cs @@ -44,7 +44,10 @@ public unsafe static int RunTest(string[] args) ProfilerControlHelpers.AttachProfilerToSelf(ReleaseOnShutdownGuid, profilerPath); PassCallbackToProfiler(() => _profilerDone.Set()); - _profilerDone.WaitOne(); + if (!_profilerDone.WaitOne(TimeSpan.FromMinutes(5))) + { + Console.WriteLine("Profiler did net set the callback, test will fail."); + } return 100; } From 945721351d1e49297d124e04b45b5ed43a272dc8 Mon Sep 17 00:00:00 2001 From: David Mason Date: Thu, 25 Feb 2021 12:52:45 -0800 Subject: [PATCH 4/4] cleanup on aisle 3 --- .../native/releaseondetach/releaseondetach.cpp | 17 ----------------- .../native/releaseondetach/releaseondetach.h | 1 - src/tests/profiler/unittest/releaseondetach.cs | 2 +- 3 files changed, 1 insertion(+), 19 deletions(-) diff --git a/src/tests/profiler/native/releaseondetach/releaseondetach.cpp b/src/tests/profiler/native/releaseondetach/releaseondetach.cpp index 9b8c2589a502f5..343d1c18babbe8 100644 --- a/src/tests/profiler/native/releaseondetach/releaseondetach.cpp +++ b/src/tests/profiler/native/releaseondetach/releaseondetach.cpp @@ -3,25 +3,8 @@ #include "releaseondetach.h" -#ifdef WIN32 -#include -#else // WIN32 -#include -#include -#include -#include -#include #include -using std::string; -using std::ifstream; -using std::getline; - -#ifdef __APPLE__ -#include -#endif // __APPLE__ -#endif // WIN32 - using std::thread; ReleaseOnDetach *ReleaseOnDetach::Instance; diff --git a/src/tests/profiler/native/releaseondetach/releaseondetach.h b/src/tests/profiler/native/releaseondetach/releaseondetach.h index 643c915279610c..e4646c2b2004aa 100644 --- a/src/tests/profiler/native/releaseondetach/releaseondetach.h +++ b/src/tests/profiler/native/releaseondetach/releaseondetach.h @@ -5,7 +5,6 @@ #include "../profiler.h" -#include #include typedef void (*ProfilerCallback) (void); diff --git a/src/tests/profiler/unittest/releaseondetach.cs b/src/tests/profiler/unittest/releaseondetach.cs index 922446d5da0082..8b01318775d2fc 100644 --- a/src/tests/profiler/unittest/releaseondetach.cs +++ b/src/tests/profiler/unittest/releaseondetach.cs @@ -46,7 +46,7 @@ public unsafe static int RunTest(string[] args) PassCallbackToProfiler(() => _profilerDone.Set()); if (!_profilerDone.WaitOne(TimeSpan.FromMinutes(5))) { - Console.WriteLine("Profiler did net set the callback, test will fail."); + Console.WriteLine("Profiler did not set the callback, test will fail."); } return 100;