Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Fix frequent FuncEval abort upon hitting a breakpoint in an ASP.NET C…
…ore 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.
  • Loading branch information
Koundinya Veluri committed Aug 26, 2021
commit 5988ddff3b263c583b34217452cef9303314a878
20 changes: 14 additions & 6 deletions src/coreclr/vm/appdomain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3566,7 +3566,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)
{
Expand Down Expand Up @@ -3596,7 +3598,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;
Expand All @@ -3617,7 +3621,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);
}
Expand All @@ -3635,7 +3641,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)
Expand Down Expand Up @@ -3665,7 +3671,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;
Expand Down Expand Up @@ -3707,7 +3714,8 @@ BOOL AppDomain::RemoveAssemblyFromCache(DomainAssembly* pAssembly)
}
CONTRACTL_END;

CrstHolder holder(&m_DomainCacheCrst);
GCX_PREEMP();
DomainCacheCrstHolderForGCCoop holder(this);

return m_AssemblyCache.RemoveAssembly(pAssembly);
}
Expand Down
24 changes: 24 additions & 0 deletions src/coreclr/vm/appdomain.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -1164,6 +1164,30 @@ class BaseDomain
};
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:
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/vm/crst.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down