Prevent EventPipe rundown from blocking on write (#25523)

* Write all rundown events to the buffer manager to prevent the write from blocking while holding the CodeManager lock

* * Incorporate PR feedback
* changed to one suspend rather than suspend->resume->suspend
* added some comments

* update comments

3 files changed, 32 insertions, 51 deletions

diff --git a/src/vm/eventpipe.cpp b/src/vm/eventpipe.cpp
index 6350256d00..09463d29ba 100644
--- a/src/vm/eventpipe.cpp
+++ b/src/vm/eventpipe.cpp
@@ -346,9 +346,7 @@ void EventPipe::DisableInternal(EventPipeSessionID id, EventPipeProviderCallback
     // Disable pSession tracing.
     s_config.Disable(*pSession, pEventPipeProviderCallbackDataQueue);
 
-    s_allowWrite &= ~(pSession->GetMask()); 
-
-    pSession->Disable(); // Suspend EventPipeBufferManager, and remove providers.
+    pSession->Disable(); // WriteAllBuffersToFile, and remove providers.
 
     // Do rundown before fully stopping the session unless rundown wasn't requested
     if (pSession->RundownRequested())
@@ -371,10 +369,13 @@ void EventPipe::DisableInternal(EventPipeSessionID id, EventPipeProviderCallback
         else
         {
             _ASSERTE(!"Failed to get or create the EventPipeThread for rundown events.");
-            return;
         }
     }
 
+    s_allowWrite &= ~(pSession->GetMask()); 
+    pSession->SuspendWriteEvent();
+    pSession->WriteAllBuffersToFile(); // Flush the buffers to the stream/file
+
     --s_numberOfSessions;
 
     // At this point, we should not be writing events to this session anymore
@@ -591,22 +592,6 @@ void EventPipe::WriteEventInternal(
         BYTE *pData = payload.GetFlatData();
         if (pThread != nullptr && pRundownSession != nullptr && pData != nullptr)
         {
-            // Write synchronously to the file.
-            // We're under lock and blocking the disabling thread.
-            // This copy occurs here (rather than at file write) because
-            // A) The FastSerializer API would need to change if we waited
-            // B) It is unclear there is a benefit to multiple file write calls
-            //    as opposed a a buffer copy here
-            EventPipeEventInstance instance(
-                event,
-                GetCurrentProcessorNumber(),
-                pEventPipeThread->GetOSThreadId(),
-                pData,
-                payload.GetSize(),
-                pActivityId,
-                pRelatedActivityId);
-            instance.EnsureStack(*pRundownSession);
-
             // EventPipeFile::WriteEvent needs to allocate a metadata event
             // and can therefore throw. In this context we will silently
             // fail rather than disrupt the caller
@@ -614,7 +599,14 @@ void EventPipe::WriteEventInternal(
             {
                 _ASSERTE(pRundownSession != nullptr);
                 if (pRundownSession != nullptr)
-                    pRundownSession->WriteEventUnbuffered(instance, pEventPipeThread);
+                    pRundownSession->WriteEventBuffered(
+                        pThread,
+                        event,
+                        payload,
+                        pActivityId,
+                        pRelatedActivityId,
+                        pEventThread,
+                        pStack);
             }
             EX_CATCH {}
             EX_END_CATCH(SwallowAllExceptions);
diff --git a/src/vm/eventpipesession.cpp b/src/vm/eventpipesession.cpp
index aa18df2dec..ca007dc570 100644
--- a/src/vm/eventpipesession.cpp
+++ b/src/vm/eventpipesession.cpp
@@ -319,32 +319,6 @@ bool EventPipeSession::WriteEventBuffered(
         false;
 }
 
-void EventPipeSession::WriteEventUnbuffered(EventPipeEventInstance &instance, EventPipeThread* pThread)
-{
-    CONTRACTL
-    {
-        THROWS;
-        GC_NOTRIGGER;
-        MODE_ANY;
-    }
-    CONTRACTL_END;
-
-    if (m_pFile == nullptr)
-        return;
-    ULONGLONG captureThreadId;
-    uint32_t sequenceNumber;
-    {
-        SpinLockHolder _slh(pThread->GetLock());
-        EventPipeThreadSessionState *const pState = pThread->GetOrCreateSessionState(this);
-        if (pState == nullptr)
-            return;
-        captureThreadId = pThread->GetOSThreadId();
-        sequenceNumber = pState->GetSequenceNumber();
-        pState->IncrementSequenceNumber();
-    }
-    m_pFile->WriteEvent(instance, captureThreadId, sequenceNumber, TRUE);
-}
-
 void EventPipeSession::WriteSequencePointUnbuffered()
 {
     CONTRACTL
@@ -474,11 +448,23 @@ void EventPipeSession::Disable()
     if ((m_SessionType == EventPipeSessionType::IpcStream) && m_ipcStreamingEnabled)
         DisableIpcStreamingThread();
 
+    WriteAllBuffersToFile();
+    m_pProviderList->Clear();
+}
+
+void EventPipeSession::SuspendWriteEvent()
+{
+    CONTRACTL
+    {
+        THROWS;
+        GC_TRIGGERS;
+        MODE_PREEMPTIVE;
+    }
+    CONTRACTL_END;
+
     // Force all in-progress writes to either finish or cancel
     // This is required to ensure we can safely flush and delete the buffers
     m_pBufferManager->SuspendWriteEvent(GetIndex());
-    WriteAllBuffersToFile();
-    m_pProviderList->Clear();
 }
 
 void EventPipeSession::ExecuteRundown()
diff --git a/src/vm/eventpipesession.h b/src/vm/eventpipesession.h
index 5e2f4b32c5..288f394afd 100644
--- a/src/vm/eventpipesession.h
+++ b/src/vm/eventpipesession.h
@@ -200,8 +200,6 @@ public:
         Thread *pEventThread = nullptr,
         StackContents *pStack = nullptr);
 
-    void WriteEventUnbuffered(EventPipeEventInstance &instance, EventPipeThread* pThread);
-
     // Write a sequence point into the output stream synchronously
     void WriteSequencePointUnbuffered();
 
@@ -211,8 +209,13 @@ public:
     void Enable();
 
     // Disable a session in the event pipe.
+    // side-effects: writes all buffers to stream/file
     void Disable();
 
+    // Force all in-progress writes to either finish or cancel
+    // This is required to ensure we can safely flush and delete the buffers
+    void SuspendWriteEvent();
+
     void EnableRundown();
     void ExecuteRundown();