Skip to content

Commit

Permalink
Partial Revert "Remove global locks from Exception stack trace handli…
Browse files Browse the repository at this point in the history 
…ng (#26823) (#26950)

* Partial Revert "Remove global locks from Exception stack trace handling  (#26823)"

The scalability improvement claimed by the change is not there after closer examination.
Reverting to avoid visible non pay-for-play overhead added by the change.

* Delete managed exception stacktrace lock

History: The original version of the code had just the managed lock. It was discovered that the managed lock is not enough to guarantee integrity of the system and so the unmanaged lock was added. The managed lock was left around even though it does not guarantee much anymore.
  • Loading branch information
@jkotas
jkotas committed Oct 1, 2019
1 parent ce29457 commit 08423d0
Show file tree
Hide file tree
Showing 7 changed files with 46 additions and 152 deletions.
63 changes: 20 additions & 43 deletions src/System.Private.CoreLib/src/System/Exception.CoreCLR.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -285,9 +285,6 @@ internal void InternalPreserveStackTrace()
}
}

// Returns the lock, falling back to the static lock if the Exception is uninitialized (very rare).
private object StackTraceLock => _stackTraceLock ?? s_stackTraceLock;

// This is invoked by ExceptionDispatchInfo.Throw to restore the exception stack trace, corresponding to the original throw of the
// exception, just before the exception is "rethrown".
internal void RestoreDispatchState(in DispatchState dispatchState)
Expand All @@ -296,48 +293,30 @@ internal void RestoreDispatchState(in DispatchState dispatchState)
// Restore only for non-preallocated exceptions
if (fCanProcessException)
{
// Take a lock to ensure only one thread can restore the details
// at a time against this exception object that could have
// multiple ExceptionDispatchInfo instances associated with it.
// When restoring back the fields, we again create a copy and set reference to them
// in the exception object. This will ensure that when this exception is thrown and these
// fields are modified, then EDI's references remain intact.
//
// We do this inside a finally clause to ensure ThreadAbort cannot
// be injected while we have taken the lock. This is to prevent
// unrelated exception restorations from getting blocked due to TAE.
try { }
finally
{
// When restoring back the fields, we again create a copy and set reference to them
// in the exception object. This will ensure that when this exception is thrown and these
// fields are modified, then EDI's references remain intact.
//
// Since deep copying can throw on OOM, try to get the copies
// outside the lock.
object? _stackTraceCopy = (dispatchState.StackTrace == null) ? null : DeepCopyStackTrace(dispatchState.StackTrace);
object? _dynamicMethodsCopy = (dispatchState.DynamicMethods == null) ? null : DeepCopyDynamicMethods(dispatchState.DynamicMethods);

// Finally, restore the information.
//
// Since EDI can be created at various points during exception dispatch (e.g. at various frames on the stack) for the same exception instance,
// they can have different data to be restored. Thus, to ensure atomicity of restoration from each EDI, perform the restore under a lock.
lock (StackTraceLock)
{
_watsonBuckets = dispatchState.WatsonBuckets;
_ipForWatsonBuckets = dispatchState.IpForWatsonBuckets;
_remoteStackTraceString = dispatchState.RemoteStackTrace;
SaveStackTracesFromDeepCopy(this, _stackTraceCopy, _dynamicMethodsCopy);
}
_stackTraceString = null;

// Marks the TES state to indicate we have restored foreign exception
// dispatch information.
PrepareForForeignExceptionRaise();
}
object? stackTraceCopy = (dispatchState.StackTrace == null) ? null : DeepCopyStackTrace(dispatchState.StackTrace);
object? dynamicMethodsCopy = (dispatchState.DynamicMethods == null) ? null : DeepCopyDynamicMethods(dispatchState.DynamicMethods);

// Watson buckets and remoteStackTraceString fields are captured and restored without any locks. It is possible for them to
// get out of sync without violating overall integrity of the system.
_watsonBuckets = dispatchState.WatsonBuckets;
_ipForWatsonBuckets = dispatchState.IpForWatsonBuckets;
_remoteStackTraceString = dispatchState.RemoteStackTrace;

// The binary stack trace and references to dynamic methods have to be restored under a lock to guarantee integrity of the system.
SaveStackTracesFromDeepCopy(this, stackTraceCopy, dynamicMethodsCopy);

_stackTraceString = null;

// Marks the TES state to indicate we have restored foreign exception
// dispatch information.
PrepareForForeignExceptionRaise();
}
}

// This is the static object against which a lock will be taken when the Exception is uninitialized (constructor not called).
private static readonly object s_stackTraceLock = new object();

private MethodBase? _exceptionMethod; // Needed for serialization.
internal string? _message;
private IDictionary? _data;
Expand All @@ -354,8 +333,6 @@ internal void RestoreDispatchState(in DispatchState dispatchState)
// unless a System.Resolver object roots it.
private readonly object? _dynamicMethods;
private string? _source; // Mainly used by VB.
// This is the object against which a lock will be taken when attempting to restore the EDI.
private readonly object _stackTraceLock = new object();
private UIntPtr _ipForWatsonBuckets; // Used to persist the IP for Watson Bucketing
private readonly IntPtr _xptrs; // Internal EE stuff
private readonly int _xcode = _COMPlusExceptionCode; // Internal EE stuff
Expand Down
6 changes: 2 additions & 4 deletions src/vm/debugdebugger.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1086,11 +1086,9 @@ void DebugStackTrace::GetStackFramesFromException(OBJECTREF * e,

// Now get the _stackTrace reference
StackTraceArray traceData;
ZeroMemory(&traceData, sizeof(traceData));
GCPROTECT_BEGIN(traceData);

EXCEPTIONREF(*e)->GetStackTrace(traceData, pDynamicMethodArray);
EXCEPTIONREF(*e)->GetStackTrace(traceData, pDynamicMethodArray);

GCPROTECT_BEGIN(traceData);
// The number of frame info elements in the stack trace info
pData->cElements = static_cast<int>(traceData.Size());

Expand Down
4 changes: 2 additions & 2 deletions src/vm/dwreport.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -521,9 +521,9 @@ UINT_PTR GetIPOfThrowSite(
{
// Get the BYTE[] containing the stack trace.
StackTraceArray traceData;
ZeroMemory(&traceData, sizeof(traceData));
((EXCEPTIONREF)throwable)->GetStackTrace(traceData);

GCPROTECT_BEGIN(traceData);
((EXCEPTIONREF)throwable)->GetStackTrace(traceData);
// Grab the first non-zero, if there is one.
for (size_t ix = 0; ix < traceData.Size(); ++ix)
{
Expand Down
2 changes: 1 addition & 1 deletion src/vm/excep.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2211,7 +2211,7 @@ void StackTraceInfo::SaveStackTrace(BOOL bAllowAllocMem, OBJECTHANDLE hThrowable
{
CONTRACTL
{
THROWS;
NOTHROW;
GC_TRIGGERS;
MODE_COOPERATIVE;
}
Expand Down
1 change: 0 additions & 1 deletion src/vm/mscorlib.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,6 @@ DEFINE_FIELD_U(_stackTraceString, ExceptionObject, _stackTraceString)
DEFINE_FIELD_U(_remoteStackTraceString, ExceptionObject, _remoteStackTraceString)
DEFINE_FIELD_U(_dynamicMethods, ExceptionObject, _dynamicMethods)
DEFINE_FIELD_U(_source, ExceptionObject, _source)
DEFINE_FIELD_U(_stackTraceLock, ExceptionObject, _stackTraceLock)
DEFINE_FIELD_U(_ipForWatsonBuckets,ExceptionObject, _ipForWatsonBuckets)
DEFINE_FIELD_U(_xptrs, ExceptionObject, _xptrs)
DEFINE_FIELD_U(_xcode, ExceptionObject, _xcode)
Expand Down
114 changes: 19 additions & 95 deletions src/vm/object.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1969,114 +1969,30 @@ StackTraceElement & StackTraceArray::operator[](size_t index)
}

#if !defined(DACCESS_COMPILE)
// Define the lock used to access stacktrace from a global exception type
// Define the lock used to access stacktrace from an exception object
SpinLock g_StackTraceArrayLock;

void ExceptionObject::SetStackTrace(I1ARRAYREF stackTrace, PTRARRAYREF dynamicMethodArray)
{
CONTRACTL
{
THROWS;
GC_TRIGGERS;
MODE_COOPERATIVE;
}
CONTRACTL_END;

if (_stackTraceLock == NULL || GetMethodTable() == g_pOutOfMemoryExceptionClass)
{
// We can't activate the SyncBlock for an OutOfMemoryException, so use preallocated SpinLock
SpinLock::AcquireLock(&g_StackTraceArrayLock);

SetObjectReference((OBJECTREF*)&_stackTrace, (OBJECTREF)stackTrace);
SetObjectReference((OBJECTREF*)&_dynamicMethods, (OBJECTREF)dynamicMethodArray);

SpinLock::ReleaseLock(&g_StackTraceArrayLock);
}
else
{
struct _gc
{
EXCEPTIONREF throwable;
OBJECTREF dynamicMethodArray;
OBJECTREF stateLock;
OBJECTREF trace;
};
_gc gc;
gc.throwable = (EXCEPTIONREF)this;
gc.dynamicMethodArray = (OBJECTREF)dynamicMethodArray;
gc.stateLock = _stackTraceLock;
gc.trace = stackTrace;
GCPROTECT_BEGIN(gc);

// Aquire lock
gc.stateLock->EnterObjMonitor();

SetObjectReference((OBJECTREF*)&gc.throwable->_stackTrace, gc.trace);
SetObjectReference((OBJECTREF*)&gc.throwable->_dynamicMethods, gc.dynamicMethodArray);

// Release lock
gc.stateLock->LeaveObjMonitor();

GCPROTECT_END();
}
}

void ExceptionObject::GetStackTrace(StackTraceArray& stackTrace, PTRARRAYREF* outDynamicMethodArray /*= NULL*/)
{
CONTRACTL
{
THROWS;
GC_TRIGGERS;
GC_NOTRIGGER;
NOTHROW;
MODE_COOPERATIVE;
}
CONTRACTL_END;

if (_stackTraceLock == NULL || GetMethodTable() == g_pOutOfMemoryExceptionClass)
{
// We can't activate the SyncBlock for an OutOfMemoryException, so use preallocated SpinLock
SpinLock::AcquireLock(&g_StackTraceArrayLock);

StackTraceArray temp(_stackTrace);
stackTrace.Swap(temp);

if (outDynamicMethodArray != NULL)
{
*outDynamicMethodArray = _dynamicMethods;
}
SpinLock::AcquireLock(&g_StackTraceArrayLock);

SpinLock::ReleaseLock(&g_StackTraceArrayLock);
}
else
{
struct _gc
{
EXCEPTIONREF throwable;
OBJECTREF stateLock;
};
_gc gc;
gc.throwable = (EXCEPTIONREF)this;
gc.stateLock = _stackTraceLock;
GCPROTECT_BEGIN(gc);

// Aquire lock
gc.stateLock->EnterObjMonitor();

StackTraceArray temp(gc.throwable->_stackTrace);
stackTrace.Swap(temp);

if (outDynamicMethodArray != NULL)
{
*outDynamicMethodArray = gc.throwable->_dynamicMethods;
}
SetObjectReference((OBJECTREF*)&_stackTrace, (OBJECTREF)stackTrace);
SetObjectReference((OBJECTREF*)&_dynamicMethods, (OBJECTREF)dynamicMethodArray);

// Release lock
gc.stateLock->LeaveObjMonitor();
SpinLock::ReleaseLock(&g_StackTraceArrayLock);

GCPROTECT_END();
}
}
#else
void ExceptionObject::GetStackTrace(StackTraceArray& stackTrace, PTRARRAYREF* outDynamicMethodArray /*= NULL*/) const
#endif // !defined(DACCESS_COMPILE)

void ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * outDynamicMethodArray /*= NULL*/) const
{
CONTRACTL
{
Expand All @@ -2086,16 +2002,24 @@ void ExceptionObject::GetStackTrace(StackTraceArray& stackTrace, PTRARRAYREF* ou
}
CONTRACTL_END;

#if !defined(DACCESS_COMPILE)
SpinLock::AcquireLock(&g_StackTraceArrayLock);
#endif // !defined(DACCESS_COMPILE)

StackTraceArray temp(_stackTrace);
stackTrace.Swap(temp);

if (outDynamicMethodArray != NULL)
{
*outDynamicMethodArray = _dynamicMethods;
}
}

#if !defined(DACCESS_COMPILE)
SpinLock::ReleaseLock(&g_StackTraceArrayLock);
#endif // !defined(DACCESS_COMPILE)

}

bool LAHashDependentHashTrackerObject::IsLoaderAllocatorLive()
{
return (ObjectFromHandle(_dependentHandle) != NULL);
Expand Down
8 changes: 2 additions & 6 deletions src/vm/object.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2508,16 +2508,13 @@ class ExceptionObject : public Object

void SetStackTrace(I1ARRAYREF stackTrace, PTRARRAYREF dynamicMethodArray);

#ifdef DACCESS_COMPILE
void GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * outDynamicMethodArray = NULL) const;

I1ARRAYREF GetStackTraceArrayObject() const
{
LIMITED_METHOD_DAC_CONTRACT;
return _stackTrace;
}
void GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * outDynamicMethodArray = NULL) const;
#else
void GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * outDynamicMethodArray = NULL);
#endif // DACCESS_COMPILE

void SetInnerException(OBJECTREF innerException)
{
Expand Down Expand Up @@ -2673,7 +2670,6 @@ class ExceptionObject : public Object
STRINGREF _remoteStackTraceString;
PTRARRAYREF _dynamicMethods;
STRINGREF _source; // Mainly used by VB.
OBJECTREF _stackTraceLock; // Lock used to access stacktrace from an exception object

UINT_PTR _ipForWatsonBuckets; // Contains the IP of exception for watson bucketing
void* _xptrs;
Expand Down

0 comments on commit 08423d0

Please sign in to comment.