Fix frequent FuncEval abort upon hitting a breakpoint in an ASP.NET Core web app...

- `AssemblySpecBindingCache` uses a cooperative-GC-mode data structure for the cache and operates on the cache from inside a lock taken in preemptive-GC-mode
- So when the cache is being used, cooperative-GC-mode is entered while holding the lock, which can in turn suspend for the debugger. Then a FuncEval that also happens to operate on the cache would deadlock on acquiring the lock and would have to be aborted.
- This seems to be happening very frequently when hitting an early breakpoint in a default new ASP.NET Core web app in .NET 6 when hot reload is enabled
- Fixed by using the same solution that was used for the slot backpatching lock. When cooperative-GC-mode would be entered inside the lock, a different lock holder based on `CrstAndForbidSuspendForDebuggerHolder` is used, which prevents the thread from suspending for the debugger while the lock is held. The thread would instead suspend for the debugger after leaving the forbid region after releasing the lock.

Co-authored-by: Koundinya Veluri <kouvel@microsoft.com>

diff --git a/src/coreclr/vm/appdomain.cpp b/src/coreclr/vm/appdomain.cpp
index f7bf57c..a55090b 100644 (file)
--- a/src/coreclr/vm/appdomain.cpp
+++ b/src/coreclr/vm/appdomain.cpp
@@ -3709,7 +3709,9 @@ BOOL AppDomain::AddFileToCache(AssemblySpec* pSpec, PEAssembly *pFile, BOOL fAll
     }
     CONTRACTL_END;
 
-    CrstHolder holder(&m_DomainCacheCrst);
+    GCX_PREEMP();
+    DomainCacheCrstHolderForGCCoop holder(this);
+
     // !!! suppress exceptions
     if(!m_AssemblyCache.StoreFile(pSpec, pFile) && !fAllowFailure)
     {
@@ -3739,7 +3741,9 @@ BOOL AppDomain::AddAssemblyToCache(AssemblySpec* pSpec, DomainAssembly *pAssembl
     }
     CONTRACTL_END;
 
-    CrstHolder holder(&m_DomainCacheCrst);
+    GCX_PREEMP();
+    DomainCacheCrstHolderForGCCoop holder(this);
+
     // !!! suppress exceptions
     BOOL bRetVal = m_AssemblyCache.StoreAssembly(pSpec, pAssembly);
     return bRetVal;
@@ -3760,7 +3764,9 @@ BOOL AppDomain::AddExceptionToCache(AssemblySpec* pSpec, Exception *ex)
     if (ex->IsTransient())
         return TRUE;
 
-    CrstHolder holder(&m_DomainCacheCrst);
+    GCX_PREEMP();
+    DomainCacheCrstHolderForGCCoop holder(this);
+
     // !!! suppress exceptions
     return m_AssemblyCache.StoreException(pSpec, ex);
 }
@@ -3778,7 +3784,7 @@ void AppDomain::AddUnmanagedImageToCache(LPCWSTR libraryName, NATIVE_LIBRARY_HAN
     }
     CONTRACTL_END;
 
-    CrstHolder lock(&m_DomainCacheCrst);
+    DomainCacheCrstHolderForGCPreemp lock(this);
 
     const UnmanagedImageCacheEntry *existingEntry = m_unmanagedCache.LookupPtr(libraryName);
     if (existingEntry != NULL)
@@ -3808,7 +3814,8 @@ NATIVE_LIBRARY_HANDLE AppDomain::FindUnmanagedImageInCache(LPCWSTR libraryName)
     }
     CONTRACT_END;
 
-    CrstHolder lock(&m_DomainCacheCrst);
+    DomainCacheCrstHolderForGCPreemp lock(this);
+
     const UnmanagedImageCacheEntry *existingEntry = m_unmanagedCache.LookupPtr(libraryName);
     if (existingEntry == NULL)
         RETURN NULL;
@@ -3850,7 +3857,8 @@ BOOL AppDomain::RemoveAssemblyFromCache(DomainAssembly* pAssembly)
     }
     CONTRACTL_END;
 
-    CrstHolder holder(&m_DomainCacheCrst);
+    GCX_PREEMP();
+    DomainCacheCrstHolderForGCCoop holder(this);
 
     return m_AssemblyCache.RemoveAssembly(pAssembly);
 }

diff --git a/src/coreclr/vm/appdomain.hpp b/src/coreclr/vm/appdomain.hpp
index fb2e456..2babce3 100644 (file)
--- a/src/coreclr/vm/appdomain.hpp
+++ b/src/coreclr/vm/appdomain.hpp
@@ -1171,6 +1171,30 @@ public:
     };
     friend class LockHolder;
 
+    // To be used when the thread will remain in preemptive GC mode while holding the lock
+    class DomainCacheCrstHolderForGCPreemp : private CrstHolder
+    {
+    public:
+        DomainCacheCrstHolderForGCPreemp(BaseDomain *pD)
+            : CrstHolder(&pD->m_DomainCacheCrst)
+        {
+            WRAPPER_NO_CONTRACT;
+        }
+    };
+
+    // To be used when the thread may enter cooperative GC mode while holding the lock. The thread enters a
+    // forbid-suspend-for-debugger region along with acquiring the lock, such that it would not suspend for the debugger while
+    // holding the lock, as that may otherwise cause a FuncEval to deadlock when trying to acquire the lock.
+    class DomainCacheCrstHolderForGCCoop : private CrstAndForbidSuspendForDebuggerHolder
+    {
+    public:
+        DomainCacheCrstHolderForGCCoop(BaseDomain *pD)
+            : CrstAndForbidSuspendForDebuggerHolder(&pD->m_DomainCacheCrst)
+        {
+            WRAPPER_NO_CONTRACT;
+        }
+    };
+
     class DomainLocalBlockLockHolder : public CrstHolder
     {
     public:

diff --git a/src/coreclr/vm/crst.cpp b/src/coreclr/vm/crst.cpp
index 63472e4..d617647 100644 (file)
--- a/src/coreclr/vm/crst.cpp
+++ b/src/coreclr/vm/crst.cpp
@@ -789,8 +789,8 @@ CrstBase::CrstAndForbidSuspendForDebuggerHolder::CrstAndForbidSuspendForDebugger
     // Reentrant locks are currently not supported
     _ASSERTE((pCrst->m_dwFlags & CRST_REENTRANCY) == 0);
 
-    Thread *pThread = GetThread();
-    if (pThread->IsInForbidSuspendForDebuggerRegion())
+    Thread *pThread = GetThreadNULLOk();
+    if (pThread == nullptr || pThread->IsInForbidSuspendForDebuggerRegion())
     {
         AcquireLock(pCrst);
         return;