From f93ded43352b9f1276f35aa1ea82ae0195d8a366 Mon Sep 17 00:00:00 2001 From: Anton Lapounov Date: Sat, 16 Jul 2022 19:04:13 -0700 Subject: [PATCH 01/10] Implement ControlledExecution API --- docs/project/list-of-diagnostics.md | 1 + .../System.Private.CoreLib.csproj | 1 + .../Runtime/ControlledExecution.CoreCLR.cs | 173 ++++++++++++++++++ .../src/System.Private.CoreLib.csproj | 1 + .../Runtime/ControlledExecution.NativeAot.cs | 15 ++ src/coreclr/vm/comsynchronizable.cpp | 30 +++ src/coreclr/vm/comsynchronizable.h | 2 + src/coreclr/vm/ecalllist.h | 5 + src/coreclr/vm/qcallentrypoints.cpp | 1 + .../Common/src/System/Obsoletions.cs | 3 + .../src/Resources/Strings.resx | 4 + .../System.Runtime/ref/System.Runtime.cs | 5 + .../tests/System.Runtime.Tests.csproj | 1 + .../Runtime/ControlledExecutionTests.cs | 170 +++++++++++++++++ .../System.Private.CoreLib.csproj | 1 + .../Runtime/ControlledExecution.Mono.cs | 15 ++ 16 files changed, 428 insertions(+) create mode 100644 src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs create mode 100644 src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/ControlledExecution.NativeAot.cs create mode 100644 src/libraries/System.Runtime/tests/System/Runtime/ControlledExecutionTests.cs create mode 100644 src/mono/System.Private.CoreLib/src/System/Runtime/ControlledExecution.Mono.cs diff --git a/docs/project/list-of-diagnostics.md b/docs/project/list-of-diagnostics.md index 750e556d6d9ac..f3a7e8eec5b2d 100644 --- a/docs/project/list-of-diagnostics.md +++ b/docs/project/list-of-diagnostics.md @@ -100,6 +100,7 @@ The PR that reveals the implementation of the ` + diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs new file mode 100644 index 0000000000000..6bedd2b4f973c --- /dev/null +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs @@ -0,0 +1,173 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Diagnostics; +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; +using System.Threading; + +namespace System.Runtime +{ + /// + /// Allows to run code and abort it asynchronously. + /// + public static partial class ControlledExecution + { + /// + /// Runs code that may be aborted asynchronously. + /// + /// The delegate that represents the code to execute. + /// The cancellation token that may be used to abort execution. + /// The argument is null. + /// + /// The current thread is already running the method. + /// + /// The execution was aborted. + [Obsolete(Obsoletions.ControlledExecutionRunMessage, DiagnosticId = Obsoletions.ControlledExecutionRunDiagId, UrlFormat = Obsoletions.SharedUrlFormat)] + public static void Run(Action action, CancellationToken cancellationToken) + { + ArgumentNullException.ThrowIfNull(action); + var execution = new Execution(action, cancellationToken); + cancellationToken.Register(execution.Abort, useSynchronizationContext: false); + execution.Run(); + } + + [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "ThreadNative_Abort")] + private static partial void AbortThread(ThreadHandle thread); + + [MethodImpl(MethodImplOptions.InternalCall)] + private static extern bool ResetAbortThread(); + + private sealed partial class Execution + { + // The state transition diagram (S means the Started flag and so on): + // N ⟶ S ⟶ SF + // ↓ ↓ + // AR SAR ⟶ SFAR + // ↓ ↓ ↓ + // A SA ⟶ SFA + private enum State : int + { + None = 0, + Started = 1, + Finished = 2, + AbortRequested = 4, + RunningAbort = 8 + } + + [ThreadStatic] + private static Execution? t_execution; + + // Interpreted as a value of the State enumeration type + private int _state; + private readonly Action _action; + private readonly CancellationToken _cancellationToken; + private Thread? _thread; + + public Execution(Action action, CancellationToken cancellationToken) + { + _action = action; + _cancellationToken = cancellationToken; + } + + public void Run() + { + Debug.Assert((_state & (int)State.Started) == 0 && _thread == null); + + // Nested ControlledExecution.Run methods are not supported + if (t_execution != null) + throw new InvalidOperationException(SR.InvalidOperation_NestedControlledExecutionRun); + + _thread = Thread.CurrentThread; + + try + { + try + { + // As soon as the Started flag is set, this thread may be aborted asynchronously + if (Interlocked.CompareExchange(ref _state, (int)State.Started, (int)State.None) == (int)State.None) + { + t_execution = this; + _action(); + } + } + finally + { + if ((_state & (int)State.Started) != 0) + { + // Set the Finished flag to prevent a potential subsequent AbortThread call + State oldState = (State)Interlocked.Or(ref _state, (int)State.Finished); + + if ((oldState & State.AbortRequested) != 0) + { + // Either in SFAR or SFA state + while (true) + { + // The enclosing finally may be cloned by the JIT for the non-exceptional code flow. + // In that case this code is not guarded against a thread abort, so make this FCall as + // soon as possible. + bool resetAbortRequest = ResetAbortThread(); + + // If there is an Abort in progress, we need to wait until it sets the TS_AbortRequested + // flag on this thread, then we can reset the flag and safely exit this frame. + if (((oldState & State.RunningAbort) == 0) || resetAbortRequest) + { + break; + } + + // It should take very short time for AbortThread to set the TS_AbortRequested flag + Thread.Sleep(0); + oldState = (State)Volatile.Read(ref _state); + } + } + + t_execution = null; + } + } + } + catch (ThreadAbortException) when (_cancellationToken.IsCancellationRequested) + { + throw new OperationCanceledException(_cancellationToken); + } + } + + public void Abort() + { + // Prevent potential refetching of _state from shared memory + State curState = (State)Volatile.Read(ref _state); + State oldState; + + do + { + Debug.Assert((curState & (State.AbortRequested | State.RunningAbort)) == 0); + + // If the execution has finished, there is nothing to do + if ((curState & State.Finished) != 0) + return; + + // Try to set the AbortRequested and RunningAbort flags + oldState = curState; + curState = (State)Interlocked.CompareExchange(ref _state, + (int)(oldState | State.AbortRequested | State.RunningAbort), (int)oldState); + } + while (curState != oldState); + + try + { + // If the execution has not started yet, we are done + if ((curState & State.Started) == 0) + return; + + // Must be in SAR or SFAR state now + Debug.Assert(_thread != null); + AbortThread(_thread.GetNativeHandle()); + } + finally + { + // Reset the RunningAbort flag to signal the executing thread it is safe to exit + Interlocked.And(ref _state, (int)~State.RunningAbort); + } + } + } + } +} diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj b/src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj index 57906233af649..ef54a1296368d 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj @@ -189,6 +189,7 @@ + diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/ControlledExecution.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/ControlledExecution.NativeAot.cs new file mode 100644 index 0000000000000..285e5b30e1715 --- /dev/null +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/ControlledExecution.NativeAot.cs @@ -0,0 +1,15 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Threading; + +namespace System.Runtime +{ + public static class ControlledExecution + { + public static void Run(Action action, CancellationToken cancellationToken) + { + throw new PlatformNotSupportedException(); + } + } +} diff --git a/src/coreclr/vm/comsynchronizable.cpp b/src/coreclr/vm/comsynchronizable.cpp index 1d7fcf8b15c2c..9bef9b0b7cce5 100644 --- a/src/coreclr/vm/comsynchronizable.cpp +++ b/src/coreclr/vm/comsynchronizable.cpp @@ -1149,6 +1149,36 @@ extern "C" BOOL QCALLTYPE ThreadNative_YieldThread() return ret; } +extern "C" void QCALLTYPE ThreadNative_Abort(QCall::ThreadHandle thread) +{ + QCALL_CONTRACT; + + BEGIN_QCALL + + thread->UserAbort(EEPolicy::TA_Safe, INFINITE); + + END_QCALL +} + +// Unmarks the current thread for abort. +// Returns true if the thread had the TS_AbortRequested flag set. +FCIMPL0(FC_BOOL_RET, ThreadNative::ResetAbort) +{ + FCALL_CONTRACT; + + BOOL ret = FALSE; + + Thread *pThread = GetThreadNULLOk(); + if (pThread != NULL && pThread->IsAbortRequested()) + { + pThread->UnmarkThreadForAbort(); + ret = TRUE; + } + + FC_RETURN_BOOL(ret); +} +FCIMPLEND + FCIMPL0(INT32, ThreadNative::GetCurrentProcessorNumber) { FCALL_CONTRACT; diff --git a/src/coreclr/vm/comsynchronizable.h b/src/coreclr/vm/comsynchronizable.h index 43f998d0055af..fdf420dc89430 100644 --- a/src/coreclr/vm/comsynchronizable.h +++ b/src/coreclr/vm/comsynchronizable.h @@ -62,6 +62,7 @@ friend class ThreadBaseObject; static FCDECL1(INT32, GetPriority, ThreadBaseObject* pThisUNSAFE); static FCDECL2(void, SetPriority, ThreadBaseObject* pThisUNSAFE, INT32 iPriority); static FCDECL1(void, Interrupt, ThreadBaseObject* pThisUNSAFE); + static FCDECL0(FC_BOOL_RET, ResetAbort); static FCDECL1(FC_BOOL_RET, IsAlive, ThreadBaseObject* pThisUNSAFE); static FCDECL2(FC_BOOL_RET, Join, ThreadBaseObject* pThisUNSAFE, INT32 Timeout); static FCDECL1(void, Sleep, INT32 iTime); @@ -110,6 +111,7 @@ extern "C" void QCALLTYPE ThreadNative_InformThreadNameChange(QCall::ThreadHandl extern "C" UINT64 QCALLTYPE ThreadNative_GetProcessDefaultStackSize(); extern "C" BOOL QCALLTYPE ThreadNative_YieldThread(); extern "C" UINT64 QCALLTYPE ThreadNative_GetCurrentOSThreadId(); +extern "C" void QCALLTYPE ThreadNative_Abort(QCall::ThreadHandle thread); #endif // _COMSYNCHRONIZABLE_H diff --git a/src/coreclr/vm/ecalllist.h b/src/coreclr/vm/ecalllist.h index 6c06aacbf4fc7..41ce04b1287f3 100644 --- a/src/coreclr/vm/ecalllist.h +++ b/src/coreclr/vm/ecalllist.h @@ -715,6 +715,10 @@ FCFuncStart(gWeakReferenceOfTFuncs) FCFuncElement("IsTrackResurrection", WeakReferenceOfTNative::IsTrackResurrection) FCFuncEnd() +FCFuncStart(gControlledExecutionFuncs) + FCFuncElement("ResetAbortThread", ThreadNative::ResetAbort) +FCFuncEnd() + #ifdef FEATURE_COMINTEROP // @@ -753,6 +757,7 @@ FCClassElement("AssemblyLoadContext", "System.Runtime.Loader", gAssemblyLoadCont FCClassElement("Buffer", "System", gBufferFuncs) FCClassElement("CastHelpers", "System.Runtime.CompilerServices", gCastHelpers) FCClassElement("CompatibilitySwitch", "System.Runtime.Versioning", gCompatibilitySwitchFuncs) +FCClassElement("ControlledExecution", "System.Runtime", gControlledExecutionFuncs) FCClassElement("CustomAttribute", "System.Reflection", gCOMCustomAttributeFuncs) FCClassElement("CustomAttributeEncodedArgument", "System.Reflection", gCustomAttributeEncodedArgument) FCClassElement("Debugger", "System.Diagnostics", gDiagnosticsDebugger) diff --git a/src/coreclr/vm/qcallentrypoints.cpp b/src/coreclr/vm/qcallentrypoints.cpp index 5b5f4c8a8ac1b..3057f0129242b 100644 --- a/src/coreclr/vm/qcallentrypoints.cpp +++ b/src/coreclr/vm/qcallentrypoints.cpp @@ -209,6 +209,7 @@ static const Entry s_QCall[] = DllImportEntry(ThreadNative_InformThreadNameChange) DllImportEntry(ThreadNative_YieldThread) DllImportEntry(ThreadNative_GetCurrentOSThreadId) + DllImportEntry(ThreadNative_Abort) DllImportEntry(ThreadPool_GetCompletedWorkItemCount) DllImportEntry(ThreadPool_RequestWorkerThread) DllImportEntry(ThreadPool_PerformGateActivities) diff --git a/src/libraries/Common/src/System/Obsoletions.cs b/src/libraries/Common/src/System/Obsoletions.cs index 7c252ea7e3dca..c2f9369137f95 100644 --- a/src/libraries/Common/src/System/Obsoletions.cs +++ b/src/libraries/Common/src/System/Obsoletions.cs @@ -147,5 +147,8 @@ internal static class Obsoletions internal const string CryptoStringFactoryMessage = "Cryptographic factory methods accepting an algorithm name are obsolete. Use the parameterless Create factory method on the algorithm type instead."; internal const string CryptoStringFactoryDiagId = "SYSLIB0045"; + + internal const string ControlledExecutionRunMessage = "ControlledExecution.Run method may corrupt the process and should not be used in production code."; + internal const string ControlledExecutionRunDiagId = "SYSLIB0046"; } } diff --git a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx index 3fc2d2014809e..010002d549ef5 100644 --- a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx +++ b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx @@ -2500,6 +2500,10 @@ NativeOverlapped cannot be reused for multiple operations. + + The thread is already executing the ControlledExecution.Run method. + {Locked="ControlledExecution.Run"} + You cannot have more than one dynamic module in each dynamic assembly in this version of the runtime. diff --git a/src/libraries/System.Runtime/ref/System.Runtime.cs b/src/libraries/System.Runtime/ref/System.Runtime.cs index f0425a2bf00c2..5ceeee7d411a0 100644 --- a/src/libraries/System.Runtime/ref/System.Runtime.cs +++ b/src/libraries/System.Runtime/ref/System.Runtime.cs @@ -12131,6 +12131,11 @@ public sealed partial class AssemblyTargetedPatchBandAttribute : System.Attribut public AssemblyTargetedPatchBandAttribute(string targetedPatchBand) { } public string TargetedPatchBand { get { throw null; } } } + public static partial class ControlledExecution + { + [System.ObsoleteAttribute("ControlledExecution.Run method may corrupt the process and should not be used in production code.", DiagnosticId = "SYSLIB0046", UrlFormat = "https://aka.ms/dotnet-warnings/{0}")] + public static void Run(System.Action action, System.Threading.CancellationToken cancellationToken) { throw null; } + } public partial struct DependentHandle : System.IDisposable { private object _dummy; diff --git a/src/libraries/System.Runtime/tests/System.Runtime.Tests.csproj b/src/libraries/System.Runtime/tests/System.Runtime.Tests.csproj index 036977e8d25f8..8ff7eb9bc2675 100644 --- a/src/libraries/System.Runtime/tests/System.Runtime.Tests.csproj +++ b/src/libraries/System.Runtime/tests/System.Runtime.Tests.csproj @@ -226,6 +226,7 @@ + diff --git a/src/libraries/System.Runtime/tests/System/Runtime/ControlledExecutionTests.cs b/src/libraries/System.Runtime/tests/System/Runtime/ControlledExecutionTests.cs new file mode 100644 index 0000000000000..a007065068a47 --- /dev/null +++ b/src/libraries/System.Runtime/tests/System/Runtime/ControlledExecutionTests.cs @@ -0,0 +1,170 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Threading; +using Xunit; + +// Disable warnings for ControlledExecution.Run +#pragma warning disable SYSLIB0046 + +namespace System.Runtime.Tests +{ + public class ControlledExecutionTests + { + private bool _startedExecution, _caughtException, _finishedExecution; + private Exception _exception; + private volatile int _counter; + + // Tests cancellation on timeout. The ThreadAbortException must be mapped to OperationCanceledException. + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotNetFramework), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] + public void CancelOnTimeout() + { + var cts = new CancellationTokenSource(); + cts.CancelAfter(200); + RunTest(LengthyAction, cts.Token); + + Assert.True(_startedExecution); + Assert.True(_caughtException); + Assert.False(_finishedExecution); + Assert.IsType(_exception); + } + + // Tests that catch blocks are not aborted. The action catches the ThreadAbortException and throws an exception of a different type. + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotNetFramework), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] + public void CancelOnTimeout_ThrowFromCatch() + { + var cts = new CancellationTokenSource(); + cts.CancelAfter(200); + RunTest(LengthyAction_ThrowFromCatch, cts.Token); + + Assert.True(_startedExecution); + Assert.True(_caughtException); + Assert.False(_finishedExecution); + Assert.IsType(_exception); + } + + // Tests that finally blocks are not aborted. The action catches the ThreadAbortException and throws an exception of a different type. + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotNetFramework), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] + public void CancelOnTimeout_ThrowFromFinally() + { + var cts = new CancellationTokenSource(); + cts.CancelAfter(200); + RunTest(LengthyAction_ThrowFromFinally, cts.Token); + + Assert.True(_startedExecution); + Assert.IsType(_exception); + } + + // Tests that finally blocks are not aborted. The action throws an exception of a different type. + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotNetFramework), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] + public void CancelOnTimeout_Finally() + { + var cts = new CancellationTokenSource(); + cts.CancelAfter(200); + RunTest(LengthyAction_Finally, cts.Token); + + Assert.True(_startedExecution); + Assert.True(_finishedExecution); + Assert.IsType(_exception); + } + + // Tests cancellation before calling the Run method + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotNetFramework), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] + public void CancelBeforeRun() + { + var cts = new CancellationTokenSource(); + cts.Cancel(); + Thread.Sleep(100); + RunTest(LengthyAction, cts.Token); + + Assert.False(_startedExecution); + Assert.Null(_exception); + } + + private void RunTest(Action action, CancellationToken cancellationToken) + { + _startedExecution = _caughtException = _finishedExecution = false; + _exception = null; + + try + { + ControlledExecution.Run(action, cancellationToken); + } + catch (Exception e) + { + _exception = e; + } + } + + private void LengthyAction() + { + _startedExecution = true; + + try + { + for (_counter = 0; _counter < int.MaxValue; _counter++) { } + } + catch + { + // Swallow all exceptions to verify that the exception is automatically rethrown + _caughtException = true; + } + + _finishedExecution = true; + } + + private void LengthyAction_ThrowFromCatch() + { + _startedExecution = true; + + try + { + for (_counter = 0; _counter < int.MaxValue; _counter++) { } + } + catch + { + _caughtException = true; + // Catch blocks must not be aborted + Thread.Sleep(100); + throw new TimeoutException(); + } + + _finishedExecution = true; + } + + private void LengthyAction_ThrowFromFinally() + { + _startedExecution = true; + + try + { + // Make sure to run the non-inlined finally + throw new Exception(); + } + finally + { + // Finally blocks must not be aborted + Thread.Sleep(400); + throw new TimeoutException(); + } + } + + private void LengthyAction_Finally() + { + _startedExecution = true; + + try + { + // Make sure to run the non-inlined finally + throw new TimeoutException(); + } + finally + { + // Finally blocks must not be aborted + Thread.Sleep(400); + _finishedExecution = true; + } + + } + } +} diff --git a/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj b/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj index 4945fcc682af3..e9ffd3db21647 100644 --- a/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj +++ b/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj @@ -254,6 +254,7 @@ + diff --git a/src/mono/System.Private.CoreLib/src/System/Runtime/ControlledExecution.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Runtime/ControlledExecution.Mono.cs new file mode 100644 index 0000000000000..285e5b30e1715 --- /dev/null +++ b/src/mono/System.Private.CoreLib/src/System/Runtime/ControlledExecution.Mono.cs @@ -0,0 +1,15 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Threading; + +namespace System.Runtime +{ + public static class ControlledExecution + { + public static void Run(Action action, CancellationToken cancellationToken) + { + throw new PlatformNotSupportedException(); + } + } +} From b3fb148338cb62f80d6578476f30be65a6c61e33 Mon Sep 17 00:00:00 2001 From: Anton Lapounov Date: Sat, 16 Jul 2022 20:12:18 -0700 Subject: [PATCH 02/10] Fix build breaks --- .../src/System/Runtime/ControlledExecution.CoreCLR.cs | 2 +- .../src/System/Runtime/ControlledExecution.NativeAot.cs | 1 + .../src/System/Runtime/ControlledExecution.Mono.cs | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs index 6bedd2b4f973c..e965b7ebfe86d 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs @@ -74,7 +74,7 @@ public void Run() { Debug.Assert((_state & (int)State.Started) == 0 && _thread == null); - // Nested ControlledExecution.Run methods are not supported + // Recursive ControlledExecution.Run calls are not supported if (t_execution != null) throw new InvalidOperationException(SR.InvalidOperation_NestedControlledExecutionRun); diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/ControlledExecution.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/ControlledExecution.NativeAot.cs index 285e5b30e1715..8d6bcbaba73df 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/ControlledExecution.NativeAot.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/ControlledExecution.NativeAot.cs @@ -7,6 +7,7 @@ namespace System.Runtime { public static class ControlledExecution { + [Obsolete("ControlledExecution.Run method may corrupt the process and should not be used in production code.", DiagnosticId = "SYSLIB0046", UrlFormat = "https://aka.ms/dotnet-warnings/{0}")] public static void Run(Action action, CancellationToken cancellationToken) { throw new PlatformNotSupportedException(); diff --git a/src/mono/System.Private.CoreLib/src/System/Runtime/ControlledExecution.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Runtime/ControlledExecution.Mono.cs index 285e5b30e1715..8d6bcbaba73df 100644 --- a/src/mono/System.Private.CoreLib/src/System/Runtime/ControlledExecution.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Runtime/ControlledExecution.Mono.cs @@ -7,6 +7,7 @@ namespace System.Runtime { public static class ControlledExecution { + [Obsolete("ControlledExecution.Run method may corrupt the process and should not be used in production code.", DiagnosticId = "SYSLIB0046", UrlFormat = "https://aka.ms/dotnet-warnings/{0}")] public static void Run(Action action, CancellationToken cancellationToken) { throw new PlatformNotSupportedException(); From f57bce71eaf9b5ed806a58620ad0301107e1aaf5 Mon Sep 17 00:00:00 2001 From: Anton Lapounov Date: Mon, 25 Jul 2022 06:01:13 -0700 Subject: [PATCH 03/10] Address feedback --- .../Runtime/ControlledExecution.CoreCLR.cs | 125 +++++++++--------- src/coreclr/vm/arm64/asmhelpers.asm | 27 ++++ src/coreclr/vm/arm64/stubs.cpp | 6 - .../Runtime/ControlledExecutionTests.cs | 53 ++++++-- 4 files changed, 134 insertions(+), 77 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs index e965b7ebfe86d..8b3fc1153392c 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs @@ -3,6 +3,7 @@ using System.Diagnostics; using System.Runtime.CompilerServices; +using System.Runtime.ExceptionServices; using System.Runtime.InteropServices; using System.Threading; @@ -13,6 +14,9 @@ namespace System.Runtime /// public static partial class ControlledExecution { + [ThreadStatic] + private static bool t_executing; + /// /// Runs code that may be aborted asynchronously. /// @@ -23,13 +27,24 @@ public static partial class ControlledExecution /// The current thread is already running the method. /// /// The execution was aborted. + /// + /// enables aborting arbitrary code in a non-cooperative manner. + /// Doing so may corrupt the process. This method is not recommended for use in production code + /// in which reliability is important. + /// [Obsolete(Obsoletions.ControlledExecutionRunMessage, DiagnosticId = Obsoletions.ControlledExecutionRunDiagId, UrlFormat = Obsoletions.SharedUrlFormat)] public static void Run(Action action, CancellationToken cancellationToken) { + if (!OperatingSystem.IsWindows()) + throw new PlatformNotSupportedException(); + ArgumentNullException.ThrowIfNull(action); - var execution = new Execution(action, cancellationToken); - cancellationToken.Register(execution.Abort, useSynchronizationContext: false); - execution.Run(); + + // Recursive ControlledExecution.Run calls are not supported + if (t_executing) + throw new InvalidOperationException(SR.InvalidOperation_NestedControlledExecutionRun); + + new Execution(action, cancellationToken).Run(); } [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "ThreadNative_Abort")] @@ -40,24 +55,20 @@ public static void Run(Action action, CancellationToken cancellationToken) private sealed partial class Execution { - // The state transition diagram (S means the Started flag and so on): - // N ⟶ S ⟶ SF - // ↓ ↓ - // AR SAR ⟶ SFAR - // ↓ ↓ ↓ - // A SA ⟶ SFA + // The state transition diagram (F means the Finished flag and so on): + // N ⟶ F + // ↓ + // AR ⟶ FAR + // ↓ ↓ + // A ⟶ FA private enum State : int { None = 0, - Started = 1, - Finished = 2, - AbortRequested = 4, - RunningAbort = 8 + Finished = 1, + AbortRequested = 2, + RunningAbort = 4 } - [ThreadStatic] - private static Execution? t_execution; - // Interpreted as a value of the State enumeration type private int _state; private readonly Action _action; @@ -72,62 +83,62 @@ public Execution(Action action, CancellationToken cancellationToken) public void Run() { - Debug.Assert((_state & (int)State.Started) == 0 && _thread == null); - - // Recursive ControlledExecution.Run calls are not supported - if (t_execution != null) - throw new InvalidOperationException(SR.InvalidOperation_NestedControlledExecutionRun); - + Debug.Assert(_state == (int)State.None && _thread == null); _thread = Thread.CurrentThread; try { try { - // As soon as the Started flag is set, this thread may be aborted asynchronously - if (Interlocked.CompareExchange(ref _state, (int)State.Started, (int)State.None) == (int)State.None) - { - t_execution = this; - _action(); - } + t_executing = true; + // Cannot Dispose this registration in a finally or a catch block as that may deadlock with AbortThread + _cancellationToken.UnsafeRegister(e => ((Execution)e!).Abort(), this); + _action(); } finally { - if ((_state & (int)State.Started) != 0) - { - // Set the Finished flag to prevent a potential subsequent AbortThread call - State oldState = (State)Interlocked.Or(ref _state, (int)State.Finished); + t_executing = false; - if ((oldState & State.AbortRequested) != 0) + // Set the Finished flag to prevent a potential subsequent AbortThread call + State oldState = (State)Interlocked.Or(ref _state, (int)State.Finished); + + if ((oldState & State.AbortRequested) != 0) + { + // Either in FAR or FA state + while (true) { - // Either in SFAR or SFA state - while (true) + // The enclosing finally may be cloned by the JIT for the non-exceptional code flow. + // In that case this code is not guarded against a thread abort. In particular, any + // QCall may be aborted. That is OK as we will catch the ThreadAbortException and call + // ResetAbortThread again below. The only downside is that a successfully executed + // action may be reported as canceled. + bool resetAbortRequest = ResetAbortThread(); + + // If there is an Abort in progress, we need to wait until it sets the TS_AbortRequested + // flag on this thread, then we can reset the flag and safely exit this frame. + if (((oldState & State.RunningAbort) == 0) || resetAbortRequest) { - // The enclosing finally may be cloned by the JIT for the non-exceptional code flow. - // In that case this code is not guarded against a thread abort, so make this FCall as - // soon as possible. - bool resetAbortRequest = ResetAbortThread(); - - // If there is an Abort in progress, we need to wait until it sets the TS_AbortRequested - // flag on this thread, then we can reset the flag and safely exit this frame. - if (((oldState & State.RunningAbort) == 0) || resetAbortRequest) - { - break; - } - - // It should take very short time for AbortThread to set the TS_AbortRequested flag - Thread.Sleep(0); - oldState = (State)Volatile.Read(ref _state); + break; } - } - t_execution = null; + // It should take very short time for AbortThread to set the TS_AbortRequested flag + Thread.Sleep(0); + oldState = (State)Volatile.Read(ref _state); + } } } } - catch (ThreadAbortException) when (_cancellationToken.IsCancellationRequested) + catch (ThreadAbortException tae) when (_cancellationToken.IsCancellationRequested) { - throw new OperationCanceledException(_cancellationToken); + t_executing = false; + ResetAbortThread(); + + var e = new OperationCanceledException(_cancellationToken); + if (tae.StackTrace is string stackTrace) + { + ExceptionDispatchInfo.SetRemoteStackTrace(e, stackTrace); + } + throw e; } } @@ -154,11 +165,7 @@ public void Abort() try { - // If the execution has not started yet, we are done - if ((curState & State.Started) == 0) - return; - - // Must be in SAR or SFAR state now + // Must be in AR or FAR state now Debug.Assert(_thread != null); AbortThread(_thread.GetNativeHandle()); } diff --git a/src/coreclr/vm/arm64/asmhelpers.asm b/src/coreclr/vm/arm64/asmhelpers.asm index 371790376b5a9..158bf76d227a1 100644 --- a/src/coreclr/vm/arm64/asmhelpers.asm +++ b/src/coreclr/vm/arm64/asmhelpers.asm @@ -20,6 +20,8 @@ #ifdef FEATURE_READYTORUN IMPORT DynamicHelperWorker #endif + IMPORT HijackHandler + IMPORT ThrowControlForThread #ifdef FEATURE_USE_SOFTWARE_WRITE_WATCH_FOR_GC_HEAP IMPORT g_sw_ww_table @@ -1028,6 +1030,31 @@ FaultingExceptionFrame_FrameOffset SETA SIZEOF__GSCookie MEND +; ------------------------------------------------------------------ +; +; Helpers for ThreadAbort exceptions +; + + NESTED_ENTRY RedirectForThreadAbort2,,HijackHandler + PROLOG_SAVE_REG_PAIR fp,lr, #-16! + + ; stack must be 16 byte aligned + CHECK_STACK_ALIGNMENT + + ; On entry: + ; + ; x0 = address of FaultingExceptionFrame + ; + ; Invoke the helper to setup the FaultingExceptionFrame and raise the exception + bl ThrowControlForThread + + ; ThrowControlForThread doesn't return. + EMIT_BREAKPOINT + + NESTED_END RedirectForThreadAbort2 + + GenerateRedirectedStubWithFrame RedirectForThreadAbort, RedirectForThreadAbort2 + ; ------------------------------------------------------------------ ; ResolveWorkerChainLookupAsmStub ; diff --git a/src/coreclr/vm/arm64/stubs.cpp b/src/coreclr/vm/arm64/stubs.cpp index bd1aaaa632f42..df3797a441f3a 100644 --- a/src/coreclr/vm/arm64/stubs.cpp +++ b/src/coreclr/vm/arm64/stubs.cpp @@ -918,12 +918,6 @@ PTR_CONTEXT GetCONTEXTFromRedirectedStubStackFrame(T_CONTEXT * pContext) return *ppContext; } -void RedirectForThreadAbort() -{ - // ThreadAbort is not supported in .net core - throw "NYI"; -} - #if !defined(DACCESS_COMPILE) FaultingExceptionFrame *GetFrameFromRedirectedStubStackFrame (DISPATCHER_CONTEXT *pDispatcherContext) { diff --git a/src/libraries/System.Runtime/tests/System/Runtime/ControlledExecutionTests.cs b/src/libraries/System.Runtime/tests/System/Runtime/ControlledExecutionTests.cs index a007065068a47..ffd826f85aa99 100644 --- a/src/libraries/System.Runtime/tests/System/Runtime/ControlledExecutionTests.cs +++ b/src/libraries/System.Runtime/tests/System/Runtime/ControlledExecutionTests.cs @@ -13,10 +13,11 @@ public class ControlledExecutionTests { private bool _startedExecution, _caughtException, _finishedExecution; private Exception _exception; + private CancellationTokenSource _cts; private volatile int _counter; // Tests cancellation on timeout. The ThreadAbortException must be mapped to OperationCanceledException. - [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotNetFramework), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsWindows), nameof(PlatformDetection.IsNotNetFramework), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] public void CancelOnTimeout() { var cts = new CancellationTokenSource(); @@ -30,7 +31,7 @@ public void CancelOnTimeout() } // Tests that catch blocks are not aborted. The action catches the ThreadAbortException and throws an exception of a different type. - [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotNetFramework), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsWindows), nameof(PlatformDetection.IsNotNetFramework), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] public void CancelOnTimeout_ThrowFromCatch() { var cts = new CancellationTokenSource(); @@ -43,8 +44,8 @@ public void CancelOnTimeout_ThrowFromCatch() Assert.IsType(_exception); } - // Tests that finally blocks are not aborted. The action catches the ThreadAbortException and throws an exception of a different type. - [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotNetFramework), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] + // Tests that finally blocks are not aborted. The action throws an exception from a finally block. + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsWindows), nameof(PlatformDetection.IsNotNetFramework), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] public void CancelOnTimeout_ThrowFromFinally() { var cts = new CancellationTokenSource(); @@ -55,8 +56,8 @@ public void CancelOnTimeout_ThrowFromFinally() Assert.IsType(_exception); } - // Tests that finally blocks are not aborted. The action throws an exception of a different type. - [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotNetFramework), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] + // Tests that finally blocks are not aborted. The action throws an exception from a try block. + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsWindows), nameof(PlatformDetection.IsNotNetFramework), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] public void CancelOnTimeout_Finally() { var cts = new CancellationTokenSource(); @@ -69,7 +70,7 @@ public void CancelOnTimeout_Finally() } // Tests cancellation before calling the Run method - [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotNetFramework), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsWindows), nameof(PlatformDetection.IsNotNetFramework), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] public void CancelBeforeRun() { var cts = new CancellationTokenSource(); @@ -77,8 +78,20 @@ public void CancelBeforeRun() Thread.Sleep(100); RunTest(LengthyAction, cts.Token); - Assert.False(_startedExecution); - Assert.Null(_exception); + Assert.IsType(_exception); + } + + // Tests cancellation by the action itself + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsWindows), nameof(PlatformDetection.IsNotNetFramework), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] + public void CancelItself() + { + _cts = new CancellationTokenSource(); + RunTest(Action_CancelItself, _cts.Token); + + Assert.True(_startedExecution); + Assert.False(_finishedExecution); + Assert.IsType(_exception); + Assert.IsType(_exception.InnerException); } private void RunTest(Action action, CancellationToken cancellationToken) @@ -124,7 +137,7 @@ private void LengthyAction_ThrowFromCatch() catch { _caughtException = true; - // Catch blocks must not be aborted + // The catch block must not be aborted Thread.Sleep(100); throw new TimeoutException(); } @@ -143,7 +156,7 @@ private void LengthyAction_ThrowFromFinally() } finally { - // Finally blocks must not be aborted + // The finally block must not be aborted Thread.Sleep(400); throw new TimeoutException(); } @@ -160,11 +173,27 @@ private void LengthyAction_Finally() } finally { - // Finally blocks must not be aborted + // The finally block must not be aborted Thread.Sleep(400); _finishedExecution = true; } + } + + private void Action_CancelItself() + { + _startedExecution = true; + try + { + // Make sure to run the non-inlined finally + throw new TimeoutException(); + } + finally + { + // The finally block must be aborted + _cts.Cancel(); + _finishedExecution = true; + } } } } From 9f8f1eb67efead62556c965f4c6d438c6130723a Mon Sep 17 00:00:00 2001 From: Anton Lapounov Date: Mon, 25 Jul 2022 07:49:43 -0700 Subject: [PATCH 04/10] Fix ARM64 build break --- src/coreclr/vm/arm64/asmmacros.h | 18 ++++++++++++++++++ src/coreclr/vm/arm64/stubs.cpp | 7 +++++++ 2 files changed, 25 insertions(+) diff --git a/src/coreclr/vm/arm64/asmmacros.h b/src/coreclr/vm/arm64/asmmacros.h index 65f9c42a8937f..493a01c7c3e32 100644 --- a/src/coreclr/vm/arm64/asmmacros.h +++ b/src/coreclr/vm/arm64/asmmacros.h @@ -154,6 +154,24 @@ __EndLabelName SETS "$FuncName":CC:"_End" MEND +;----------------------------------------------------------------------------- +; Macro used to check (in debug builds only) whether the stack is 16-bytes aligned (a requirement before calling +; out into C++/OS code). Invoke this directly after your prolog (if the stack frame size is fixed) or directly +; before a call (if you have a frame pointer and a dynamic stack). A breakpoint will be invoked if the stack +; is misaligned. +; + MACRO + CHECK_STACK_ALIGNMENT + +#ifdef _DEBUG + add x9, sp, xzr + tst x9, #15 + beq %F0 + EMIT_BREAKPOINT +0 +#endif + MEND + ;----------------------------------------------------------------------------- ; The Following sets of SAVE_*_REGISTERS expect the memory to be reserved and ; base address to be passed in $reg diff --git a/src/coreclr/vm/arm64/stubs.cpp b/src/coreclr/vm/arm64/stubs.cpp index df3797a441f3a..0f94c5d612cbb 100644 --- a/src/coreclr/vm/arm64/stubs.cpp +++ b/src/coreclr/vm/arm64/stubs.cpp @@ -918,6 +918,13 @@ PTR_CONTEXT GetCONTEXTFromRedirectedStubStackFrame(T_CONTEXT * pContext) return *ppContext; } +#ifndef TARGET_WINDOWS +void RedirectForThreadAbort() +{ + throw "NYI"; +} +#endif // TARGET_WINDOWS + #if !defined(DACCESS_COMPILE) FaultingExceptionFrame *GetFrameFromRedirectedStubStackFrame (DISPATCHER_CONTEXT *pDispatcherContext) { From 3a5fc3cccfaad590923a75b6697086365f721e03 Mon Sep 17 00:00:00 2001 From: Anton Lapounov Date: Mon, 25 Jul 2022 16:03:47 -0700 Subject: [PATCH 05/10] Address Jan's feedback --- src/coreclr/vm/arm64/stubs.cpp | 7 ---- src/coreclr/vm/arm64/unixstubs.cpp | 5 +++ src/coreclr/vm/exceptionhandling.cpp | 10 +++--- .../Runtime/ControlledExecutionTests.cs | 33 ++++++++++++++----- 4 files changed, 34 insertions(+), 21 deletions(-) diff --git a/src/coreclr/vm/arm64/stubs.cpp b/src/coreclr/vm/arm64/stubs.cpp index 0f94c5d612cbb..df3797a441f3a 100644 --- a/src/coreclr/vm/arm64/stubs.cpp +++ b/src/coreclr/vm/arm64/stubs.cpp @@ -918,13 +918,6 @@ PTR_CONTEXT GetCONTEXTFromRedirectedStubStackFrame(T_CONTEXT * pContext) return *ppContext; } -#ifndef TARGET_WINDOWS -void RedirectForThreadAbort() -{ - throw "NYI"; -} -#endif // TARGET_WINDOWS - #if !defined(DACCESS_COMPILE) FaultingExceptionFrame *GetFrameFromRedirectedStubStackFrame (DISPATCHER_CONTEXT *pDispatcherContext) { diff --git a/src/coreclr/vm/arm64/unixstubs.cpp b/src/coreclr/vm/arm64/unixstubs.cpp index d51902a949f26..9b313f8475f68 100644 --- a/src/coreclr/vm/arm64/unixstubs.cpp +++ b/src/coreclr/vm/arm64/unixstubs.cpp @@ -9,4 +9,9 @@ extern "C" { PORTABILITY_ASSERT("Implement for PAL"); } + + void RedirectForThreadAbort() + { + PORTABILITY_ASSERT("Implement for PAL"); + } }; diff --git a/src/coreclr/vm/exceptionhandling.cpp b/src/coreclr/vm/exceptionhandling.cpp index 0832e7b548dff..3ad266569055d 100644 --- a/src/coreclr/vm/exceptionhandling.cpp +++ b/src/coreclr/vm/exceptionhandling.cpp @@ -775,16 +775,14 @@ UINT_PTR ExceptionTracker::FinishSecondPass( #if defined(DEBUGGING_SUPPORTED) // Don't honour thread abort requests at this time for intercepted exceptions. - if (fIntercepted) - { - uAbortAddr = 0; - } - else -#endif // !DEBUGGING_SUPPORTED + if (!fIntercepted) +#endif // DEBUGGING_SUPPORTED { CopyOSContext(pThread->m_OSContext, pContextRecord); SetIP(pThread->m_OSContext, (PCODE)uResumePC); +#ifndef TARGET_UNIX uAbortAddr = (UINT_PTR)COMPlusCheckForAbort(uResumePC); +#endif // TARGET_UNIX } if (uAbortAddr) diff --git a/src/libraries/System.Runtime/tests/System/Runtime/ControlledExecutionTests.cs b/src/libraries/System.Runtime/tests/System/Runtime/ControlledExecutionTests.cs index ffd826f85aa99..469e3cfd60b82 100644 --- a/src/libraries/System.Runtime/tests/System/Runtime/ControlledExecutionTests.cs +++ b/src/libraries/System.Runtime/tests/System/Runtime/ControlledExecutionTests.cs @@ -17,7 +17,8 @@ public class ControlledExecutionTests private volatile int _counter; // Tests cancellation on timeout. The ThreadAbortException must be mapped to OperationCanceledException. - [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsWindows), nameof(PlatformDetection.IsNotNetFramework), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] + [ActiveIssue("https://github.com/dotnet/runtime/issues/72703", TestPlatforms.AnyUnix)] public void CancelOnTimeout() { var cts = new CancellationTokenSource(); @@ -31,7 +32,7 @@ public void CancelOnTimeout() } // Tests that catch blocks are not aborted. The action catches the ThreadAbortException and throws an exception of a different type. - [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsWindows), nameof(PlatformDetection.IsNotNetFramework), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] public void CancelOnTimeout_ThrowFromCatch() { var cts = new CancellationTokenSource(); @@ -45,7 +46,7 @@ public void CancelOnTimeout_ThrowFromCatch() } // Tests that finally blocks are not aborted. The action throws an exception from a finally block. - [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsWindows), nameof(PlatformDetection.IsNotNetFramework), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] public void CancelOnTimeout_ThrowFromFinally() { var cts = new CancellationTokenSource(); @@ -57,7 +58,7 @@ public void CancelOnTimeout_ThrowFromFinally() } // Tests that finally blocks are not aborted. The action throws an exception from a try block. - [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsWindows), nameof(PlatformDetection.IsNotNetFramework), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] public void CancelOnTimeout_Finally() { var cts = new CancellationTokenSource(); @@ -70,7 +71,7 @@ public void CancelOnTimeout_Finally() } // Tests cancellation before calling the Run method - [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsWindows), nameof(PlatformDetection.IsNotNetFramework), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] public void CancelBeforeRun() { var cts = new CancellationTokenSource(); @@ -82,7 +83,7 @@ public void CancelBeforeRun() } // Tests cancellation by the action itself - [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsWindows), nameof(PlatformDetection.IsNotNetFramework), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] public void CancelItself() { _cts = new CancellationTokenSource(); @@ -112,10 +113,19 @@ private void RunTest(Action action, CancellationToken cancellationToken) private void LengthyAction() { _startedExecution = true; + // Redirection via thread suspension is supported on Windows only. + // Make a call in the loop to allow redirection on other platforms. + bool sleep = !PlatformDetection.IsWindows; try { - for (_counter = 0; _counter < int.MaxValue; _counter++) { } + for (_counter = 0; _counter < int.MaxValue; _counter++) + { + if ((_counter & 0xfffff) == 0 && sleep) + { + Thread.Sleep(0); + } + } } catch { @@ -129,10 +139,17 @@ private void LengthyAction() private void LengthyAction_ThrowFromCatch() { _startedExecution = true; + bool sleep = !PlatformDetection.IsWindows; try { - for (_counter = 0; _counter < int.MaxValue; _counter++) { } + for (_counter = 0; _counter < int.MaxValue; _counter++) + { + if ((_counter & 0xfffff) == 0 && sleep) + { + Thread.Sleep(0); + } + } } catch { From 0553d5236f18085c8b1d5d1c255eb3faf4447bd2 Mon Sep 17 00:00:00 2001 From: Anton Lapounov Date: Mon, 25 Jul 2022 17:06:57 -0700 Subject: [PATCH 06/10] Missed one change --- .../src/System/Runtime/ControlledExecution.CoreCLR.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs index 8b3fc1153392c..580f42f5f7dd6 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs @@ -35,9 +35,6 @@ public static partial class ControlledExecution [Obsolete(Obsoletions.ControlledExecutionRunMessage, DiagnosticId = Obsoletions.ControlledExecutionRunDiagId, UrlFormat = Obsoletions.SharedUrlFormat)] public static void Run(Action action, CancellationToken cancellationToken) { - if (!OperatingSystem.IsWindows()) - throw new PlatformNotSupportedException(); - ArgumentNullException.ThrowIfNull(action); // Recursive ControlledExecution.Run calls are not supported From b95b2e35ab1fcd30ffc820f382f36e70efff0604 Mon Sep 17 00:00:00 2001 From: Anton Lapounov Date: Tue, 26 Jul 2022 11:05:42 -0700 Subject: [PATCH 07/10] Re-disable tests for Unix --- .../src/System/Runtime/ControlledExecution.CoreCLR.cs | 3 +++ src/coreclr/vm/exceptionhandling.cpp | 10 ++++++---- .../tests/System/Runtime/ControlledExecutionTests.cs | 5 +++++ 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs index 580f42f5f7dd6..8b3fc1153392c 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs @@ -35,6 +35,9 @@ public static partial class ControlledExecution [Obsolete(Obsoletions.ControlledExecutionRunMessage, DiagnosticId = Obsoletions.ControlledExecutionRunDiagId, UrlFormat = Obsoletions.SharedUrlFormat)] public static void Run(Action action, CancellationToken cancellationToken) { + if (!OperatingSystem.IsWindows()) + throw new PlatformNotSupportedException(); + ArgumentNullException.ThrowIfNull(action); // Recursive ControlledExecution.Run calls are not supported diff --git a/src/coreclr/vm/exceptionhandling.cpp b/src/coreclr/vm/exceptionhandling.cpp index 3ad266569055d..0832e7b548dff 100644 --- a/src/coreclr/vm/exceptionhandling.cpp +++ b/src/coreclr/vm/exceptionhandling.cpp @@ -775,14 +775,16 @@ UINT_PTR ExceptionTracker::FinishSecondPass( #if defined(DEBUGGING_SUPPORTED) // Don't honour thread abort requests at this time for intercepted exceptions. - if (!fIntercepted) -#endif // DEBUGGING_SUPPORTED + if (fIntercepted) + { + uAbortAddr = 0; + } + else +#endif // !DEBUGGING_SUPPORTED { CopyOSContext(pThread->m_OSContext, pContextRecord); SetIP(pThread->m_OSContext, (PCODE)uResumePC); -#ifndef TARGET_UNIX uAbortAddr = (UINT_PTR)COMPlusCheckForAbort(uResumePC); -#endif // TARGET_UNIX } if (uAbortAddr) diff --git a/src/libraries/System.Runtime/tests/System/Runtime/ControlledExecutionTests.cs b/src/libraries/System.Runtime/tests/System/Runtime/ControlledExecutionTests.cs index 469e3cfd60b82..2d615b36ce904 100644 --- a/src/libraries/System.Runtime/tests/System/Runtime/ControlledExecutionTests.cs +++ b/src/libraries/System.Runtime/tests/System/Runtime/ControlledExecutionTests.cs @@ -33,6 +33,7 @@ public void CancelOnTimeout() // Tests that catch blocks are not aborted. The action catches the ThreadAbortException and throws an exception of a different type. [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] + [ActiveIssue("https://github.com/dotnet/runtime/issues/72703", TestPlatforms.AnyUnix)] public void CancelOnTimeout_ThrowFromCatch() { var cts = new CancellationTokenSource(); @@ -47,6 +48,7 @@ public void CancelOnTimeout_ThrowFromCatch() // Tests that finally blocks are not aborted. The action throws an exception from a finally block. [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] + [ActiveIssue("https://github.com/dotnet/runtime/issues/72703", TestPlatforms.AnyUnix)] public void CancelOnTimeout_ThrowFromFinally() { var cts = new CancellationTokenSource(); @@ -59,6 +61,7 @@ public void CancelOnTimeout_ThrowFromFinally() // Tests that finally blocks are not aborted. The action throws an exception from a try block. [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] + [ActiveIssue("https://github.com/dotnet/runtime/issues/72703", TestPlatforms.AnyUnix)] public void CancelOnTimeout_Finally() { var cts = new CancellationTokenSource(); @@ -72,6 +75,7 @@ public void CancelOnTimeout_Finally() // Tests cancellation before calling the Run method [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] + [ActiveIssue("https://github.com/dotnet/runtime/issues/72703", TestPlatforms.AnyUnix)] public void CancelBeforeRun() { var cts = new CancellationTokenSource(); @@ -84,6 +88,7 @@ public void CancelBeforeRun() // Tests cancellation by the action itself [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotMonoRuntime), nameof(PlatformDetection.IsNotNativeAot))] + [ActiveIssue("https://github.com/dotnet/runtime/issues/72703", TestPlatforms.AnyUnix)] public void CancelItself() { _cts = new CancellationTokenSource(); From 702a21a2baf099a4b604467d7d57f78c057fe357 Mon Sep 17 00:00:00 2001 From: Anton Lapounov Date: Tue, 26 Jul 2022 15:49:07 -0700 Subject: [PATCH 08/10] Make ResetAbortThread a QCall --- .../Runtime/ControlledExecution.CoreCLR.cs | 23 ++++++++++--------- src/coreclr/vm/comsynchronizable.cpp | 21 ++++++++--------- src/coreclr/vm/comsynchronizable.h | 2 +- src/coreclr/vm/ecalllist.h | 5 ---- src/coreclr/vm/qcallentrypoints.cpp | 1 + 5 files changed, 24 insertions(+), 28 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs index 8b3fc1153392c..7dbd1f35b14d4 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs @@ -14,9 +14,6 @@ namespace System.Runtime /// public static partial class ControlledExecution { - [ThreadStatic] - private static bool t_executing; - /// /// Runs code that may be aborted asynchronously. /// @@ -41,18 +38,12 @@ public static void Run(Action action, CancellationToken cancellationToken) ArgumentNullException.ThrowIfNull(action); // Recursive ControlledExecution.Run calls are not supported - if (t_executing) + if (Execution.t_executing) throw new InvalidOperationException(SR.InvalidOperation_NestedControlledExecutionRun); new Execution(action, cancellationToken).Run(); } - [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "ThreadNative_Abort")] - private static partial void AbortThread(ThreadHandle thread); - - [MethodImpl(MethodImplOptions.InternalCall)] - private static extern bool ResetAbortThread(); - private sealed partial class Execution { // The state transition diagram (F means the Finished flag and so on): @@ -69,6 +60,9 @@ private enum State : int RunningAbort = 4 } + [ThreadStatic] + internal static bool t_executing; + // Interpreted as a value of the State enumeration type private int _state; private readonly Action _action; @@ -112,7 +106,7 @@ public void Run() // QCall may be aborted. That is OK as we will catch the ThreadAbortException and call // ResetAbortThread again below. The only downside is that a successfully executed // action may be reported as canceled. - bool resetAbortRequest = ResetAbortThread(); + bool resetAbortRequest = ResetAbortThread() != Interop.BOOL.FALSE; // If there is an Abort in progress, we need to wait until it sets the TS_AbortRequested // flag on this thread, then we can reset the flag and safely exit this frame. @@ -175,6 +169,13 @@ public void Abort() Interlocked.And(ref _state, (int)~State.RunningAbort); } } + + [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "ThreadNative_Abort")] + private static partial void AbortThread(ThreadHandle thread); + + [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "ThreadNative_ResetAbort")] + [SuppressGCTransition] + private static partial Interop.BOOL ResetAbortThread(); } } } diff --git a/src/coreclr/vm/comsynchronizable.cpp b/src/coreclr/vm/comsynchronizable.cpp index 9bef9b0b7cce5..c8064cea585d0 100644 --- a/src/coreclr/vm/comsynchronizable.cpp +++ b/src/coreclr/vm/comsynchronizable.cpp @@ -1140,11 +1140,11 @@ extern "C" BOOL QCALLTYPE ThreadNative_YieldThread() BOOL ret = FALSE; - BEGIN_QCALL + BEGIN_QCALL; ret = __SwitchToThread(0, CALLER_LIMITS_SPINNING); - END_QCALL + END_QCALL; return ret; } @@ -1153,31 +1153,30 @@ extern "C" void QCALLTYPE ThreadNative_Abort(QCall::ThreadHandle thread) { QCALL_CONTRACT; - BEGIN_QCALL + BEGIN_QCALL; thread->UserAbort(EEPolicy::TA_Safe, INFINITE); - END_QCALL + END_QCALL; } // Unmarks the current thread for abort. -// Returns true if the thread had the TS_AbortRequested flag set. -FCIMPL0(FC_BOOL_RET, ThreadNative::ResetAbort) +// Returns TRUE if the thread had the TS_AbortRequested flag set. +extern "C" BOOL QCALLTYPE ThreadNative_ResetAbort() { - FCALL_CONTRACT; + QCALL_CONTRACT_NO_GC_TRANSITION; BOOL ret = FALSE; - Thread *pThread = GetThreadNULLOk(); - if (pThread != NULL && pThread->IsAbortRequested()) + Thread *pThread = GetThread(); + if (pThread->IsAbortRequested()) { pThread->UnmarkThreadForAbort(); ret = TRUE; } - FC_RETURN_BOOL(ret); + return ret; } -FCIMPLEND FCIMPL0(INT32, ThreadNative::GetCurrentProcessorNumber) { diff --git a/src/coreclr/vm/comsynchronizable.h b/src/coreclr/vm/comsynchronizable.h index fdf420dc89430..eb2ae1c16db60 100644 --- a/src/coreclr/vm/comsynchronizable.h +++ b/src/coreclr/vm/comsynchronizable.h @@ -62,7 +62,6 @@ friend class ThreadBaseObject; static FCDECL1(INT32, GetPriority, ThreadBaseObject* pThisUNSAFE); static FCDECL2(void, SetPriority, ThreadBaseObject* pThisUNSAFE, INT32 iPriority); static FCDECL1(void, Interrupt, ThreadBaseObject* pThisUNSAFE); - static FCDECL0(FC_BOOL_RET, ResetAbort); static FCDECL1(FC_BOOL_RET, IsAlive, ThreadBaseObject* pThisUNSAFE); static FCDECL2(FC_BOOL_RET, Join, ThreadBaseObject* pThisUNSAFE, INT32 Timeout); static FCDECL1(void, Sleep, INT32 iTime); @@ -112,6 +111,7 @@ extern "C" UINT64 QCALLTYPE ThreadNative_GetProcessDefaultStackSize(); extern "C" BOOL QCALLTYPE ThreadNative_YieldThread(); extern "C" UINT64 QCALLTYPE ThreadNative_GetCurrentOSThreadId(); extern "C" void QCALLTYPE ThreadNative_Abort(QCall::ThreadHandle thread); +extern "C" BOOL QCALLTYPE ThreadNative_ResetAbort(); #endif // _COMSYNCHRONIZABLE_H diff --git a/src/coreclr/vm/ecalllist.h b/src/coreclr/vm/ecalllist.h index 41ce04b1287f3..6c06aacbf4fc7 100644 --- a/src/coreclr/vm/ecalllist.h +++ b/src/coreclr/vm/ecalllist.h @@ -715,10 +715,6 @@ FCFuncStart(gWeakReferenceOfTFuncs) FCFuncElement("IsTrackResurrection", WeakReferenceOfTNative::IsTrackResurrection) FCFuncEnd() -FCFuncStart(gControlledExecutionFuncs) - FCFuncElement("ResetAbortThread", ThreadNative::ResetAbort) -FCFuncEnd() - #ifdef FEATURE_COMINTEROP // @@ -757,7 +753,6 @@ FCClassElement("AssemblyLoadContext", "System.Runtime.Loader", gAssemblyLoadCont FCClassElement("Buffer", "System", gBufferFuncs) FCClassElement("CastHelpers", "System.Runtime.CompilerServices", gCastHelpers) FCClassElement("CompatibilitySwitch", "System.Runtime.Versioning", gCompatibilitySwitchFuncs) -FCClassElement("ControlledExecution", "System.Runtime", gControlledExecutionFuncs) FCClassElement("CustomAttribute", "System.Reflection", gCOMCustomAttributeFuncs) FCClassElement("CustomAttributeEncodedArgument", "System.Reflection", gCustomAttributeEncodedArgument) FCClassElement("Debugger", "System.Diagnostics", gDiagnosticsDebugger) diff --git a/src/coreclr/vm/qcallentrypoints.cpp b/src/coreclr/vm/qcallentrypoints.cpp index 3057f0129242b..ddaa9a3e98444 100644 --- a/src/coreclr/vm/qcallentrypoints.cpp +++ b/src/coreclr/vm/qcallentrypoints.cpp @@ -210,6 +210,7 @@ static const Entry s_QCall[] = DllImportEntry(ThreadNative_YieldThread) DllImportEntry(ThreadNative_GetCurrentOSThreadId) DllImportEntry(ThreadNative_Abort) + DllImportEntry(ThreadNative_ResetAbort) DllImportEntry(ThreadPool_GetCompletedWorkItemCount) DllImportEntry(ThreadPool_RequestWorkerThread) DllImportEntry(ThreadPool_PerformGateActivities) From a73aae79b5158c78db09bb054c3a20feee8bb567 Mon Sep 17 00:00:00 2001 From: Anton Lapounov Date: Tue, 26 Jul 2022 23:43:39 -0700 Subject: [PATCH 09/10] Address Stephen's feedback --- .../Runtime/ControlledExecution.CoreCLR.cs | 190 ++++++++---------- src/coreclr/vm/comsynchronizable.cpp | 12 +- src/coreclr/vm/comsynchronizable.h | 2 +- src/coreclr/vm/threads.h | 2 +- src/coreclr/vm/threadsuspend.cpp | 11 +- 5 files changed, 92 insertions(+), 125 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs index 7dbd1f35b14d4..2b57ede9fa759 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs @@ -1,7 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Diagnostics; using System.Runtime.CompilerServices; using System.Runtime.ExceptionServices; using System.Runtime.InteropServices; @@ -14,6 +13,9 @@ namespace System.Runtime /// public static partial class ControlledExecution { + [ThreadStatic] + private static bool t_executing; + /// /// Runs code that may be aborted asynchronously. /// @@ -33,149 +35,117 @@ public static partial class ControlledExecution public static void Run(Action action, CancellationToken cancellationToken) { if (!OperatingSystem.IsWindows()) + { throw new PlatformNotSupportedException(); + } ArgumentNullException.ThrowIfNull(action); - // Recursive ControlledExecution.Run calls are not supported - if (Execution.t_executing) - throw new InvalidOperationException(SR.InvalidOperation_NestedControlledExecutionRun); - - new Execution(action, cancellationToken).Run(); - } - - private sealed partial class Execution - { - // The state transition diagram (F means the Finished flag and so on): - // N ⟶ F - // ↓ - // AR ⟶ FAR - // ↓ ↓ - // A ⟶ FA - private enum State : int + // ControlledExecution.Run does not support nested invocations. If there's one already in flight + // on this thread, fail. + if (t_executing) { - None = 0, - Finished = 1, - AbortRequested = 2, - RunningAbort = 4 + throw new InvalidOperationException(SR.InvalidOperation_NestedControlledExecutionRun); } - [ThreadStatic] - internal static bool t_executing; + // Store the current thread so that it may be referenced by the Canceler.Cancel callback if one occurs. + Canceler canceler = new(Thread.CurrentThread); - // Interpreted as a value of the State enumeration type - private int _state; - private readonly Action _action; - private readonly CancellationToken _cancellationToken; - private Thread? _thread; - - public Execution(Action action, CancellationToken cancellationToken) + try { - _action = action; - _cancellationToken = cancellationToken; - } - - public void Run() - { - Debug.Assert(_state == (int)State.None && _thread == null); - _thread = Thread.CurrentThread; + // Mark this thread as now running a ControlledExecution.Run to prevent recursive usage. + t_executing = true; + // Register for aborting. From this moment until ctr.Unregister is called, this thread is subject to being + // interrupted at any moment. This could happen during the call to UnsafeRegister if cancellation has + // already been requested at the time of the registration. + CancellationTokenRegistration ctr = cancellationToken.UnsafeRegister(e => ((Canceler)e!).Cancel(), canceler); try { - try - { - t_executing = true; - // Cannot Dispose this registration in a finally or a catch block as that may deadlock with AbortThread - _cancellationToken.UnsafeRegister(e => ((Execution)e!).Abort(), this); - _action(); - } - finally + // Invoke the caller's code. + action(); + } + finally + { + // This finally block may be cloned by JIT for the non-exceptional code flow. In that case the code + // below is not guarded against aborting. That is OK as the outer try block will catch the + // ThreadAbortException and call ResetAbortThread. + + // Unregister the callback. Unlike Dispose, Unregister will not block waiting for an callback in flight + // to complete, and will instead return false if the callback has already been invoked or is currently + // in flight. + if (!ctr.Unregister()) { - t_executing = false; - - // Set the Finished flag to prevent a potential subsequent AbortThread call - State oldState = (State)Interlocked.Or(ref _state, (int)State.Finished); - - if ((oldState & State.AbortRequested) != 0) + // Wait until the callback has completed. Either the callback is already invoked and completed + // (in which case IsCancelCompleted will be true), or it may still be in flight. If it's in flight, + // the AbortThread call may be waiting for this thread to exit this finally block to exit, so while + // spinning waiting for the callback to complete, we also need to call ResetAbortThread in order to + // reset the flag the AbortThread call is polling in its waiting loop. + SpinWait sw = default; + while (!canceler.IsCancelCompleted) { - // Either in FAR or FA state - while (true) - { - // The enclosing finally may be cloned by the JIT for the non-exceptional code flow. - // In that case this code is not guarded against a thread abort. In particular, any - // QCall may be aborted. That is OK as we will catch the ThreadAbortException and call - // ResetAbortThread again below. The only downside is that a successfully executed - // action may be reported as canceled. - bool resetAbortRequest = ResetAbortThread() != Interop.BOOL.FALSE; - - // If there is an Abort in progress, we need to wait until it sets the TS_AbortRequested - // flag on this thread, then we can reset the flag and safely exit this frame. - if (((oldState & State.RunningAbort) == 0) || resetAbortRequest) - { - break; - } - - // It should take very short time for AbortThread to set the TS_AbortRequested flag - Thread.Sleep(0); - oldState = (State)Volatile.Read(ref _state); - } + ResetAbortThread(); + sw.SpinOnce(); } } } - catch (ThreadAbortException tae) when (_cancellationToken.IsCancellationRequested) + } + catch (ThreadAbortException tae) + { + // We don't want to leak ThreadAbortExceptions to user code. Instead, translate the exception into + // an OperationCanceledException, preserving stack trace details from the ThreadAbortException in + // order to aid in diagnostics and debugging. + OperationCanceledException e = cancellationToken.IsCancellationRequested ? new(cancellationToken) : new(); + if (tae.StackTrace is string stackTrace) { - t_executing = false; - ResetAbortThread(); - - var e = new OperationCanceledException(_cancellationToken); - if (tae.StackTrace is string stackTrace) - { - ExceptionDispatchInfo.SetRemoteStackTrace(e, stackTrace); - } - throw e; + ExceptionDispatchInfo.SetRemoteStackTrace(e, stackTrace); } + throw e; } - - public void Abort() + finally { - // Prevent potential refetching of _state from shared memory - State curState = (State)Volatile.Read(ref _state); - State oldState; + // Unmark this thread for recursion detection. + t_executing = false; - do + if (cancellationToken.IsCancellationRequested) { - Debug.Assert((curState & (State.AbortRequested | State.RunningAbort)) == 0); + // Reset an abort request that may still be pending on this thread. + ResetAbortThread(); + } + } + } - // If the execution has finished, there is nothing to do - if ((curState & State.Finished) != 0) - return; + [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "ThreadNative_Abort")] + private static partial void AbortThread(ThreadHandle thread); - // Try to set the AbortRequested and RunningAbort flags - oldState = curState; - curState = (State)Interlocked.CompareExchange(ref _state, - (int)(oldState | State.AbortRequested | State.RunningAbort), (int)oldState); - } - while (curState != oldState); + [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "ThreadNative_ResetAbort")] + [SuppressGCTransition] + private static partial void ResetAbortThread(); + + private sealed class Canceler + { + private readonly Thread _thread; + private volatile bool _cancelCompleted; + + public Canceler(Thread thread) + { + _thread = thread; + } + + public bool IsCancelCompleted => _cancelCompleted; + public void Cancel() + { try { - // Must be in AR or FAR state now - Debug.Assert(_thread != null); + // Abort the thread executing the action (which may be the current thread). AbortThread(_thread.GetNativeHandle()); } finally { - // Reset the RunningAbort flag to signal the executing thread it is safe to exit - Interlocked.And(ref _state, (int)~State.RunningAbort); + _cancelCompleted = true; } } - - [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "ThreadNative_Abort")] - private static partial void AbortThread(ThreadHandle thread); - - [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "ThreadNative_ResetAbort")] - [SuppressGCTransition] - private static partial Interop.BOOL ResetAbortThread(); } } } diff --git a/src/coreclr/vm/comsynchronizable.cpp b/src/coreclr/vm/comsynchronizable.cpp index c8064cea585d0..e27555248aa2c 100644 --- a/src/coreclr/vm/comsynchronizable.cpp +++ b/src/coreclr/vm/comsynchronizable.cpp @@ -1160,22 +1160,16 @@ extern "C" void QCALLTYPE ThreadNative_Abort(QCall::ThreadHandle thread) END_QCALL; } -// Unmarks the current thread for abort. -// Returns TRUE if the thread had the TS_AbortRequested flag set. -extern "C" BOOL QCALLTYPE ThreadNative_ResetAbort() +// Unmark the current thread for a safe abort. +extern "C" void QCALLTYPE ThreadNative_ResetAbort() { QCALL_CONTRACT_NO_GC_TRANSITION; - BOOL ret = FALSE; - Thread *pThread = GetThread(); if (pThread->IsAbortRequested()) { - pThread->UnmarkThreadForAbort(); - ret = TRUE; + pThread->UnmarkThreadForAbort(EEPolicy::TA_Safe); } - - return ret; } FCIMPL0(INT32, ThreadNative::GetCurrentProcessorNumber) diff --git a/src/coreclr/vm/comsynchronizable.h b/src/coreclr/vm/comsynchronizable.h index eb2ae1c16db60..f45e9e5237c29 100644 --- a/src/coreclr/vm/comsynchronizable.h +++ b/src/coreclr/vm/comsynchronizable.h @@ -111,7 +111,7 @@ extern "C" UINT64 QCALLTYPE ThreadNative_GetProcessDefaultStackSize(); extern "C" BOOL QCALLTYPE ThreadNative_YieldThread(); extern "C" UINT64 QCALLTYPE ThreadNative_GetCurrentOSThreadId(); extern "C" void QCALLTYPE ThreadNative_Abort(QCall::ThreadHandle thread); -extern "C" BOOL QCALLTYPE ThreadNative_ResetAbort(); +extern "C" void QCALLTYPE ThreadNative_ResetAbort(); #endif // _COMSYNCHRONIZABLE_H diff --git a/src/coreclr/vm/threads.h b/src/coreclr/vm/threads.h index ee2ca0a1de948..e21c7568563c9 100644 --- a/src/coreclr/vm/threads.h +++ b/src/coreclr/vm/threads.h @@ -2496,7 +2496,7 @@ class Thread public: void MarkThreadForAbort(EEPolicy::ThreadAbortTypes abortType); - void UnmarkThreadForAbort(); + void UnmarkThreadForAbort(EEPolicy::ThreadAbortTypes abortType = EEPolicy::TA_Rude); static ULONGLONG GetNextSelfAbortEndTime() { diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index 89e5598a10e13..de83a5a673d48 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -1785,7 +1785,7 @@ void Thread::RemoveAbortRequestBit() } // Make sure that when AbortRequest bit is cleared, we also dec TrapReturningThreads count. -void Thread::UnmarkThreadForAbort() +void Thread::UnmarkThreadForAbort(EEPolicy::ThreadAbortTypes abortType /* = EEPolicy::TA_Rude */) { CONTRACTL { @@ -1794,11 +1794,14 @@ void Thread::UnmarkThreadForAbort() } CONTRACTL_END; - // Switch to COOP (for ClearAbortReason) before acquiring AbortRequestLock - GCX_COOP(); - AbortRequestLockHolder lh(this); + if (m_AbortType > (DWORD)abortType) + { + // Aborting at a higher level + return; + } + m_AbortType = EEPolicy::TA_None; m_AbortEndTime = MAXULONGLONG; m_RudeAbortEndTime = MAXULONGLONG; From b4716dc7296c6aedb132b52069f8aebf0e55cdef Mon Sep 17 00:00:00 2001 From: Anton Lapounov Date: Wed, 27 Jul 2022 14:30:30 -0700 Subject: [PATCH 10/10] Update comments --- .../System/Runtime/ControlledExecution.CoreCLR.cs | 14 +++++++++++--- .../Runtime/ControlledExecution.NativeAot.cs | 2 +- .../src/System/Runtime/ControlledExecution.Mono.cs | 2 +- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs index 2b57ede9fa759..3b419f87f8b52 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs @@ -21,15 +21,23 @@ public static partial class ControlledExecution /// /// The delegate that represents the code to execute. /// The cancellation token that may be used to abort execution. + /// The method is not supported on this platform. /// The argument is null. /// /// The current thread is already running the method. /// /// The execution was aborted. /// - /// enables aborting arbitrary code in a non-cooperative manner. - /// Doing so may corrupt the process. This method is not recommended for use in production code - /// in which reliability is important. + /// This method enables aborting arbitrary managed code in a non-cooperative manner by throwing an exception + /// in the thread executing that code. While the exception may be caught by the code, it is re-thrown at the end + /// of `catch` blocks until the execution flow returns to the `ControlledExecution.Run` method. + /// Execution of the code is not guaranteed to abort immediately, or at all. This situation can occur, for + /// example, if a thread is stuck executing unmanaged code or the `catch` and `finally` blocks that are called as + /// part of the abort procedure, thereby indefinitely delaying the abort. Furthermore, execution may not be + /// aborted immediately if the thread is currently executing a `catch` or `finally` block. + /// Aborting code at an unexpected location may corrupt the state of data structures in the process and lead + /// to unpredictable results. For that reason, this method should not be used in production code and calling it + /// produces a compile-time warning. /// [Obsolete(Obsoletions.ControlledExecutionRunMessage, DiagnosticId = Obsoletions.ControlledExecutionRunDiagId, UrlFormat = Obsoletions.SharedUrlFormat)] public static void Run(Action action, CancellationToken cancellationToken) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/ControlledExecution.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/ControlledExecution.NativeAot.cs index 8d6bcbaba73df..b2031db1cec0b 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/ControlledExecution.NativeAot.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/ControlledExecution.NativeAot.cs @@ -7,7 +7,7 @@ namespace System.Runtime { public static class ControlledExecution { - [Obsolete("ControlledExecution.Run method may corrupt the process and should not be used in production code.", DiagnosticId = "SYSLIB0046", UrlFormat = "https://aka.ms/dotnet-warnings/{0}")] + [Obsolete(Obsoletions.ControlledExecutionRunMessage, DiagnosticId = Obsoletions.ControlledExecutionRunDiagId, UrlFormat = Obsoletions.SharedUrlFormat)] public static void Run(Action action, CancellationToken cancellationToken) { throw new PlatformNotSupportedException(); diff --git a/src/mono/System.Private.CoreLib/src/System/Runtime/ControlledExecution.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Runtime/ControlledExecution.Mono.cs index 8d6bcbaba73df..b2031db1cec0b 100644 --- a/src/mono/System.Private.CoreLib/src/System/Runtime/ControlledExecution.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Runtime/ControlledExecution.Mono.cs @@ -7,7 +7,7 @@ namespace System.Runtime { public static class ControlledExecution { - [Obsolete("ControlledExecution.Run method may corrupt the process and should not be used in production code.", DiagnosticId = "SYSLIB0046", UrlFormat = "https://aka.ms/dotnet-warnings/{0}")] + [Obsolete(Obsoletions.ControlledExecutionRunMessage, DiagnosticId = Obsoletions.ControlledExecutionRunDiagId, UrlFormat = Obsoletions.SharedUrlFormat)] public static void Run(Action action, CancellationToken cancellationToken) { throw new PlatformNotSupportedException();