From 6ff1beb2795d56659e3ffbd9dfc73a54c2c814d6 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Mon, 12 Jul 2021 22:59:20 -0400 Subject: [PATCH 1/4] Simplify PosixSignalRegistration implementation on Unix Bring it onto the same scheme as the Windows implementation. --- ...SignalRegistration.PlatformNotSupported.cs | 11 +- .../PosixSignalRegistration.Unix.cs | 263 ++++++------------ .../PosixSignalRegistration.Windows.cs | 72 ++--- .../PosixSignalRegistration.cs | 50 +++- 4 files changed, 153 insertions(+), 243 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.PlatformNotSupported.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.PlatformNotSupported.cs index 3ccc169b1a84b3..e726194c0c0e7f 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.PlatformNotSupported.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.PlatformNotSupported.cs @@ -10,16 +10,9 @@ public sealed partial class PosixSignalRegistration private PosixSignalRegistration() { } [DynamicDependency("#ctor")] // Prevent the private ctor and the IDisposable implementation from getting linked away - public static partial PosixSignalRegistration Create(PosixSignal signal, Action handler) - { - if (handler is null) - { - throw new ArgumentNullException(nameof(handler)); - } - + private static PosixSignalRegistration Register(PosixSignal signal, Action handler) => throw new PlatformNotSupportedException(); - } - public partial void Dispose() { } + private void Unregister() { } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Unix.cs index f01b2de0130385..7507ff13580203 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Unix.cs @@ -8,260 +8,153 @@ namespace System.Runtime.InteropServices { public sealed partial class PosixSignalRegistration { - private static volatile bool s_initialized; - private static readonly Dictionary?>> s_registrations = new(); + private static readonly Dictionary> s_registrations = Initialize(); - private readonly Action _handler; - private readonly PosixSignal _signal; - private readonly int _signo; - private bool _registered; - private readonly object _gate = new object(); - - private PosixSignalRegistration(PosixSignal signal, int signo, Action handler) - { - _signal = signal; - _signo = signo; - _handler = handler; - } - - public static partial PosixSignalRegistration Create(PosixSignal signal, Action handler) + private static unsafe Dictionary> Initialize() { - if (handler == null) + if (!Interop.Sys.InitializeTerminalAndSignalHandling()) { - throw new ArgumentNullException(nameof(handler)); + Interop.CheckIo(-1); } - int signo = Interop.Sys.GetPlatformSignalNumber(signal); - if (signo == 0) - { - throw new PlatformNotSupportedException(); - } + Interop.Sys.SetPosixSignalHandler(&OnPosixSignal); - PosixSignalRegistration registration = new PosixSignalRegistration(signal, signo, handler); - registration.Register(); - return registration; + return new Dictionary>(); } - private unsafe void Register() + private static PosixSignalRegistration Register(PosixSignal signal, Action handler) { - if (!s_initialized) + int signo = Interop.Sys.GetPlatformSignalNumber(signal); + if (signo == 0) { - if (!Interop.Sys.InitializeTerminalAndSignalHandling()) - { - // We can't use Win32Exception because that causes a cycle with - // Microsoft.Win32.Primitives. - Interop.CheckIo(-1); - } - - Interop.Sys.SetPosixSignalHandler(&OnPosixSignal); - s_initialized = true; + throw new PlatformNotSupportedException(); } + var token = new Token(signal, signo, handler); + var registration = new PosixSignalRegistration(token); + lock (s_registrations) { - if (!s_registrations.TryGetValue(_signo, out List?>? signalRegistrations)) + if (!s_registrations.TryGetValue(signo, out HashSet? tokens)) { - signalRegistrations = new List?>(); - s_registrations.Add(_signo, signalRegistrations); + s_registrations[signo] = tokens = new HashSet(); } - if (signalRegistrations.Count == 0) + if (tokens.Count == 0 && + !Interop.Sys.EnablePosixSignalHandling(signo)) { - if (!Interop.Sys.EnablePosixSignalHandling(_signo)) - { - // We can't use Win32Exception because that causes a cycle with - // Microsoft.Win32.Primitives. - Interop.CheckIo(-1); - } + // We can't use Win32Exception because that causes a cycle with + // Microsoft.Win32.Primitives. + Interop.CheckIo(-1); } - signalRegistrations.Add(new WeakReference(this)); + tokens.Add(token); } - _registered = true; + return registration; } - private bool CallHandler(PosixSignalContext context) + private void Unregister() { - lock (_gate) + lock (s_registrations) { - if (_registered) + if (_token is Token token) { - _handler(context); - return true; - } + _token = null; - return false; + if (s_registrations.TryGetValue(token.SigNo, out HashSet? tokens)) + { + tokens.Remove(token); + if (tokens.Count == 0) + { + s_registrations.Remove(token.SigNo); + Interop.Sys.DisablePosixSignalHandling(token.SigNo); + } + } + } } } [UnmanagedCallersOnly] private static int OnPosixSignal(int signo, PosixSignal signal) { - PosixSignalRegistration?[]? registrations = GetRegistrations(signo); - if (registrations != null) + if (GetTokens(signo) is Token[] tokens) { // This is called on the native signal handling thread. We need to move to another thread so // signal handling is not blocked. Otherwise we may get deadlocked when the handler depends // on work triggered from the signal handling thread. - - // For terminate/interrupt signals we use a dedicated Thread - // in case the ThreadPool is saturated. - bool useDedicatedThread = signal == PosixSignal.SIGINT || - signal == PosixSignal.SIGQUIT || - signal == PosixSignal.SIGTERM; - - if (useDedicatedThread) - { - Thread handlerThread = new Thread(HandleSignal) - { - IsBackground = true, - Name = ".NET Signal Handler" - }; - handlerThread.UnsafeStart((signo, registrations)); - } - else + switch (signal) { - ThreadPool.UnsafeQueueUserWorkItem(HandleSignal, (signo, registrations)); + case PosixSignal.SIGINT: + case PosixSignal.SIGQUIT: + case PosixSignal.SIGTERM: + // For terminate/interrupt signals we use a dedicated Thread in case the ThreadPool is saturated. + new Thread(HandleSignal) + { + IsBackground = true, + Name = ".NET Signal Handler" + }.UnsafeStart((signo, tokens)); + break; + + default: + ThreadPool.UnsafeQueueUserWorkItem(HandleSignal, (signo, tokens)); + break; } return 1; } return 0; - } - private static PosixSignalRegistration?[]? GetRegistrations(int signo) - { - lock (s_registrations) + static void HandleSignal(object? state) { - if (s_registrations.TryGetValue(signo, out List?>? signalRegistrations)) + (int signo, Token[]? tokens) = ((int, Token[]?))state!; + do { - if (signalRegistrations.Count != 0) + bool handlersCalled = false; + if (tokens != null) { - var registrations = new PosixSignalRegistration?[signalRegistrations.Count]; - bool hasRegistrations = false; - bool pruneWeakReferences = false; - - for (int i = 0; i < signalRegistrations.Count; i++) + PosixSignalContext ctx = new(0); + foreach (Token token in tokens) { - if (signalRegistrations[i]!.TryGetTarget(out PosixSignalRegistration? registration)) - { - registrations[i] = registration; - hasRegistrations = true; - } - else - { - // WeakReference no longer holds an object. PosixSignalRegistration got finalized. - signalRegistrations[i] = null; - pruneWeakReferences = true; - } - } - - if (pruneWeakReferences) - { - signalRegistrations.RemoveAll(item => item is null); + // Different values for PosixSignal map to the same signo. + // Match the PosixSignal value used when registering. + ctx.Signal = token.Signal; + token.Handler(ctx); + handlersCalled = true; } - if (hasRegistrations) - { - return registrations; - } - else - { - Interop.Sys.DisablePosixSignalHandling(signo); - } - } - } - return null; - } - } - - private static void HandleSignal(object? state) - { - HandleSignal(((int, PosixSignalRegistration?[]))state!); - } - - private static void HandleSignal((int signo, PosixSignalRegistration?[]? registrations) state) - { - do - { - bool handlersCalled = false; - if (state.registrations != null) - { - PosixSignalContext ctx = new(0); - foreach (PosixSignalRegistration? registration in state.registrations) - { - if (registration != null) + if (ctx.Cancel) { - // Different values for PosixSignal map to the same signo. - // Match the PosixSignal value used when registering. - ctx.Signal = registration._signal; - if (registration.CallHandler(ctx)) - { - handlersCalled = true; - } + return; } } - if (ctx.Cancel) + if (Interop.Sys.HandleNonCanceledPosixSignal(signo, handlersCalled ? 0 : 1)) { return; } - } - if (Interop.Sys.HandleNonCanceledPosixSignal(state.signo, handlersCalled ? 0 : 1)) - { - return; + // HandleNonCanceledPosixSignal returns false when handlers got registered. + tokens = GetTokens(signo); } + while (true); + } - // HandleNonCanceledPosixSignal returns false when handlers got registered. - state.registrations = GetRegistrations(state.signo); - } while (true); - } - - public partial void Dispose() - { - if (_registered) + static Token[]? GetTokens(int signo) { + Token[]? results = null; + lock (s_registrations) { - List?> signalRegistrations = s_registrations[_signo]; - bool pruneWeakReferences = false; - for (int i = 0; i < signalRegistrations.Count; i++) + if (s_registrations.TryGetValue(signo, out HashSet? tokens)) { - if (signalRegistrations[i]!.TryGetTarget(out PosixSignalRegistration? registration)) - { - if (ReferenceEquals(this, registration)) - { - signalRegistrations.RemoveAt(i); - break; - } - } - else - { - // WeakReference no longer holds an object. PosixSignalRegistration got finalized. - signalRegistrations[i] = null; - pruneWeakReferences = true; - } - } - - if (pruneWeakReferences) - { - signalRegistrations.RemoveAll(item => item is null); - } - - if (signalRegistrations.Count == 0) - { - Interop.Sys.DisablePosixSignalHandling(_signo); + results = new Token[tokens.Count]; + tokens.CopyTo(results); } } - // Synchronize with _handler invocations. - lock (_gate) - { - _registered = false; - } + return results; } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Windows.cs index d3e5405053e56c..a50fca2bbee906 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Windows.cs @@ -6,59 +6,51 @@ namespace System.Runtime.InteropServices { - public sealed unsafe partial class PosixSignalRegistration + public sealed partial class PosixSignalRegistration { - private static readonly HashSet s_handlers = new(); + private static readonly HashSet s_registrations = new(); - private Token? _token; - - private PosixSignalRegistration(Token token) => _token = token; - - private static object SyncObj => s_handlers; - - public static partial PosixSignalRegistration Create(PosixSignal signal, Action handler) + private static unsafe PosixSignalRegistration Register(PosixSignal signal, Action handler) { - if (handler is null) + switch (signal) { - throw new ArgumentNullException(nameof(handler)); + case PosixSignal.SIGINT: + case PosixSignal.SIGQUIT: + case PosixSignal.SIGTERM: + case PosixSignal.SIGHUP: + break; + + default: + throw new PlatformNotSupportedException(); } - lock (SyncObj) - { - switch (signal) - { - case PosixSignal.SIGINT: - case PosixSignal.SIGQUIT: - case PosixSignal.SIGTERM: - case PosixSignal.SIGHUP: - break; - - default: - throw new PlatformNotSupportedException(); - } + var token = new Token(signal, handler); + var registration = new PosixSignalRegistration(token); - if (s_handlers.Count == 0 && + lock (s_registrations) + { + if (s_registrations.Count == 0 && !Interop.Kernel32.SetConsoleCtrlHandler(&HandlerRoutine, Add: true)) { throw Win32Marshal.GetExceptionForLastWin32Error(); } - var token = new Token(signal, handler); - s_handlers.Add(token); - return new PosixSignalRegistration(token); + s_registrations.Add(token); } + + return registration; } - public partial void Dispose() + private unsafe void Unregister() { - lock (SyncObj) + lock (s_registrations) { if (_token is Token token) { _token = null; - s_handlers.Remove(token); - if (s_handlers.Count == 0 && + s_registrations.Remove(token); + if (s_registrations.Count == 0 && !Interop.Kernel32.SetConsoleCtrlHandler(&HandlerRoutine, Add: false)) { throw Win32Marshal.GetExceptionForLastWin32Error(); @@ -94,9 +86,9 @@ private static Interop.BOOL HandlerRoutine(int dwCtrlType) } List? tokens = null; - lock (SyncObj) + lock (s_registrations) { - foreach (Token token in s_handlers) + foreach (Token token in s_registrations) { if (token.Signal == signal) { @@ -118,17 +110,5 @@ private static Interop.BOOL HandlerRoutine(int dwCtrlType) return context.Cancel ? Interop.BOOL.TRUE : Interop.BOOL.FALSE; } - - private sealed class Token - { - public Token(PosixSignal signal, Action handler) - { - Signal = signal; - Handler = handler; - } - - public PosixSignal Signal { get; } - public Action Handler { get; } - } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.cs index fb2c675acb8b14..811d75ddd51822 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.cs @@ -9,6 +9,13 @@ namespace System.Runtime.InteropServices /// Handles a . public sealed partial class PosixSignalRegistration : IDisposable { + /// The state associated with this registration. + /// + /// This is separate from the registration instance so that this token may be stored + /// in a statically rooted table, with a finalizer on the registration able to remove it. + /// + private Token? _token; + /// Registers a that is invoked when the occurs. /// The signal to register for. /// The handler that gets invoked. @@ -28,11 +35,48 @@ public sealed partial class PosixSignalRegistration : IDisposable [UnsupportedOSPlatform("ios")] [UnsupportedOSPlatform("maccatalyst")] [UnsupportedOSPlatform("tvos")] - public static partial PosixSignalRegistration Create(PosixSignal signal, Action handler); + public static PosixSignalRegistration Create(PosixSignal signal, Action handler) + { + if (handler is null) + { + throw new ArgumentNullException(nameof(handler)); + } + + return Register(signal, handler); + } + + /// Initializes the registration to wrap the specified token. + private PosixSignalRegistration(Token token) => _token = token; /// Unregister the handler. - public partial void Dispose(); + public void Dispose() + { + Unregister(); + GC.SuppressFinalize(this); + } + + /// Unregister the handler. + ~PosixSignalRegistration() => Unregister(); + + /// The state associated with a registration. + private sealed class Token + { + public Token(PosixSignal signal, Action handler) + { + Signal = signal; + Handler = handler; + } + + public Token(PosixSignal signal, int sigNo, Action handler) + { + Signal = signal; + Handler = handler; + SigNo = sigNo; + } - ~PosixSignalRegistration() => Dispose(); + public PosixSignal Signal { get; } + public Action Handler { get; } + public int SigNo { get; } + } } } From b5a86a1fbfa6626d1f234ccc0d3f934e8b8fb57b Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Tue, 13 Jul 2021 06:55:52 -0400 Subject: [PATCH 2/4] Address PR feedback --- .../Unix/System.Native/Interop.PosixSignal.cs | 2 +- .../Native/Unix/System.Native/pal_signal.c | 5 +- .../Native/Unix/System.Native/pal_signal.h | 2 +- .../PosixSignalRegistration.Unix.cs | 104 ++++++++---------- 4 files changed, 48 insertions(+), 65 deletions(-) diff --git a/src/libraries/Common/src/Interop/Unix/System.Native/Interop.PosixSignal.cs b/src/libraries/Common/src/Interop/Unix/System.Native/Interop.PosixSignal.cs index b42b5138ec622d..8d030d0e11c966 100644 --- a/src/libraries/Common/src/Interop/Unix/System.Native/Interop.PosixSignal.cs +++ b/src/libraries/Common/src/Interop/Unix/System.Native/Interop.PosixSignal.cs @@ -18,7 +18,7 @@ internal static partial class Sys internal static extern void DisablePosixSignalHandling(int signal); [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_HandleNonCanceledPosixSignal")] - internal static extern bool HandleNonCanceledPosixSignal(int signal, int handlersDisposed); + internal static extern void HandleNonCanceledPosixSignal(int signal, int handlersDisposed); [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_GetPlatformSignalNumber")] [SuppressGCTransition] diff --git a/src/libraries/Native/Unix/System.Native/pal_signal.c b/src/libraries/Native/Unix/System.Native/pal_signal.c index 4c4017f6547463..aa4762a8fb069e 100644 --- a/src/libraries/Native/Unix/System.Native/pal_signal.c +++ b/src/libraries/Native/Unix/System.Native/pal_signal.c @@ -233,7 +233,7 @@ static void SignalHandler(int sig, siginfo_t* siginfo, void* context) } } -int32_t SystemNative_HandleNonCanceledPosixSignal(int32_t signalCode, int32_t handlersDisposed) +void SystemNative_HandleNonCanceledPosixSignal(int32_t signalCode, int32_t handlersDisposed) { switch (signalCode) { @@ -279,7 +279,7 @@ int32_t SystemNative_HandleNonCanceledPosixSignal(int32_t signalCode, int32_t ha if (handlersDisposed && g_hasPosixSignalRegistrations[signalCode - 1]) { // New handlers got registered. - return 0; + return; } // Restore and invoke the original handler. pthread_mutex_lock(&lock); @@ -293,7 +293,6 @@ int32_t SystemNative_HandleNonCanceledPosixSignal(int32_t signalCode, int32_t ha kill(getpid(), signalCode); break; } - return 1; } // Entrypoint for the thread that handles signals where our handling diff --git a/src/libraries/Native/Unix/System.Native/pal_signal.h b/src/libraries/Native/Unix/System.Native/pal_signal.h index 138f37835a8ca6..46369661ae8722 100644 --- a/src/libraries/Native/Unix/System.Native/pal_signal.h +++ b/src/libraries/Native/Unix/System.Native/pal_signal.h @@ -75,7 +75,7 @@ PALEXPORT void SystemNative_DisablePosixSignalHandling(int signalCode); /** * Performs the default runtime action for a non-canceled PosixSignal. */ -PALEXPORT int32_t SystemNative_HandleNonCanceledPosixSignal(int32_t signalCode, int32_t handlersDisposed); +PALEXPORT void SystemNative_HandleNonCanceledPosixSignal(int32_t signalCode, int32_t handlersDisposed); typedef void (*ConsoleSigTtouHandler)(void); diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Unix.cs index 7507ff13580203..33bcdc8c1cccaa 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Unix.cs @@ -43,8 +43,6 @@ private static PosixSignalRegistration Register(PosixSignal signal, Action? registrations)) { - case PosixSignal.SIGINT: - case PosixSignal.SIGQUIT: - case PosixSignal.SIGTERM: - // For terminate/interrupt signals we use a dedicated Thread in case the ThreadPool is saturated. - new Thread(HandleSignal) - { - IsBackground = true, - Name = ".NET Signal Handler" - }.UnsafeStart((signo, tokens)); - break; - - default: - ThreadPool.UnsafeQueueUserWorkItem(HandleSignal, (signo, tokens)); - break; + tokens = new Token[registrations.Count]; + registrations.CopyTo(tokens); } + } + + if (tokens is null) + { + return 0; + } - return 1; + // This is called on the native signal handling thread. We need to move to another thread so + // signal handling is not blocked. Otherwise we may get deadlocked when the handler depends + // on work triggered from the signal handling thread. + switch (signal) + { + case PosixSignal.SIGINT: + case PosixSignal.SIGQUIT: + case PosixSignal.SIGTERM: + // For terminate/interrupt signals we use a dedicated Thread in case the ThreadPool is saturated. + new Thread(HandleSignal) + { + IsBackground = true, + Name = ".NET Signal Handler" + }.UnsafeStart((signo, tokens)); + break; + + default: + ThreadPool.UnsafeQueueUserWorkItem(HandleSignal, (signo, tokens)); + break; } - return 0; + return 1; static void HandleSignal(object? state) { (int signo, Token[]? tokens) = ((int, Token[]?))state!; - do + + bool handlersCalled = false; + if (tokens != null) { - bool handlersCalled = false; - if (tokens != null) + PosixSignalContext ctx = new(0); + foreach (Token token in tokens) { - PosixSignalContext ctx = new(0); - foreach (Token token in tokens) - { - // Different values for PosixSignal map to the same signo. - // Match the PosixSignal value used when registering. - ctx.Signal = token.Signal; - token.Handler(ctx); - handlersCalled = true; - } - - if (ctx.Cancel) - { - return; - } + // Different values for PosixSignal map to the same signo. + // Match the PosixSignal value used when registering. + ctx.Signal = token.Signal; + token.Handler(ctx); + handlersCalled = true; } - if (Interop.Sys.HandleNonCanceledPosixSignal(signo, handlersCalled ? 0 : 1)) + if (ctx.Cancel) { return; } - - // HandleNonCanceledPosixSignal returns false when handlers got registered. - tokens = GetTokens(signo); - } - while (true); - } - - static Token[]? GetTokens(int signo) - { - Token[]? results = null; - - lock (s_registrations) - { - if (s_registrations.TryGetValue(signo, out HashSet? tokens)) - { - results = new Token[tokens.Count]; - tokens.CopyTo(results); - } } - return results; + Interop.Sys.HandleNonCanceledPosixSignal(signo, handlersCalled ? 0 : 1); } } } From f3db75efc8147f2172f9615fd94f4fa106abba8e Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Tue, 13 Jul 2021 09:08:26 -0400 Subject: [PATCH 3/4] Delete more dead code --- .../Unix/System.Native/Interop.PosixSignal.cs | 2 +- .../Native/Unix/System.Native/pal_signal.c | 9 ++---- .../Native/Unix/System.Native/pal_signal.h | 2 +- .../PosixSignalRegistration.Unix.cs | 31 ++++++++----------- 4 files changed, 17 insertions(+), 27 deletions(-) diff --git a/src/libraries/Common/src/Interop/Unix/System.Native/Interop.PosixSignal.cs b/src/libraries/Common/src/Interop/Unix/System.Native/Interop.PosixSignal.cs index 8d030d0e11c966..65e3ca35f4844c 100644 --- a/src/libraries/Common/src/Interop/Unix/System.Native/Interop.PosixSignal.cs +++ b/src/libraries/Common/src/Interop/Unix/System.Native/Interop.PosixSignal.cs @@ -18,7 +18,7 @@ internal static partial class Sys internal static extern void DisablePosixSignalHandling(int signal); [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_HandleNonCanceledPosixSignal")] - internal static extern void HandleNonCanceledPosixSignal(int signal, int handlersDisposed); + internal static extern void HandleNonCanceledPosixSignal(int signal); [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_GetPlatformSignalNumber")] [SuppressGCTransition] diff --git a/src/libraries/Native/Unix/System.Native/pal_signal.c b/src/libraries/Native/Unix/System.Native/pal_signal.c index aa4762a8fb069e..ee6ccd833c0de9 100644 --- a/src/libraries/Native/Unix/System.Native/pal_signal.c +++ b/src/libraries/Native/Unix/System.Native/pal_signal.c @@ -233,7 +233,7 @@ static void SignalHandler(int sig, siginfo_t* siginfo, void* context) } } -void SystemNative_HandleNonCanceledPosixSignal(int32_t signalCode, int32_t handlersDisposed) +void SystemNative_HandleNonCanceledPosixSignal(int32_t signalCode) { switch (signalCode) { @@ -276,11 +276,6 @@ void SystemNative_HandleNonCanceledPosixSignal(int32_t signalCode, int32_t handl // Original handler doesn't do anything. break; } - if (handlersDisposed && g_hasPosixSignalRegistrations[signalCode - 1]) - { - // New handlers got registered. - return; - } // Restore and invoke the original handler. pthread_mutex_lock(&lock); { @@ -384,7 +379,7 @@ static void* SignalHandlerLoop(void* arg) if (!usePosixSignalHandler) { - SystemNative_HandleNonCanceledPosixSignal(signalCode, 0); + SystemNative_HandleNonCanceledPosixSignal(signalCode); } } } diff --git a/src/libraries/Native/Unix/System.Native/pal_signal.h b/src/libraries/Native/Unix/System.Native/pal_signal.h index 46369661ae8722..146fdabbfceb14 100644 --- a/src/libraries/Native/Unix/System.Native/pal_signal.h +++ b/src/libraries/Native/Unix/System.Native/pal_signal.h @@ -75,7 +75,7 @@ PALEXPORT void SystemNative_DisablePosixSignalHandling(int signalCode); /** * Performs the default runtime action for a non-canceled PosixSignal. */ -PALEXPORT void SystemNative_HandleNonCanceledPosixSignal(int32_t signalCode, int32_t handlersDisposed); +PALEXPORT void SystemNative_HandleNonCanceledPosixSignal(int32_t signalCode); typedef void (*ConsoleSigTtouHandler)(void); diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Unix.cs index 33bcdc8c1cccaa..dc65ae4e749c25 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Unix.cs @@ -92,6 +92,8 @@ private static int OnPosixSignal(int signo, PosixSignal signal) return 0; } + Debug.Assert(tokens.Length != 0); + // This is called on the native signal handling thread. We need to move to another thread so // signal handling is not blocked. Otherwise we may get deadlocked when the handler depends // on work triggered from the signal handling thread. @@ -117,28 +119,21 @@ private static int OnPosixSignal(int signo, PosixSignal signal) static void HandleSignal(object? state) { - (int signo, Token[]? tokens) = ((int, Token[]?))state!; + (int signo, Token[] tokens) = ((int, Token[]))state!; - bool handlersCalled = false; - if (tokens != null) + PosixSignalContext ctx = new(0); + foreach (Token token in tokens) { - PosixSignalContext ctx = new(0); - foreach (Token token in tokens) - { - // Different values for PosixSignal map to the same signo. - // Match the PosixSignal value used when registering. - ctx.Signal = token.Signal; - token.Handler(ctx); - handlersCalled = true; - } - - if (ctx.Cancel) - { - return; - } + // Different values for PosixSignal map to the same signo. + // Match the PosixSignal value used when registering. + ctx.Signal = token.Signal; + token.Handler(ctx); } - Interop.Sys.HandleNonCanceledPosixSignal(signo, handlersCalled ? 0 : 1); + if (!ctx.Cancel) + { + Interop.Sys.HandleNonCanceledPosixSignal(signo); + } } } } From a3c53074ec847043a221f2f70e1f9c81c767d543 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Tue, 13 Jul 2021 09:30:39 -0400 Subject: [PATCH 4/4] Update PosixSignalRegistration.Unix.cs --- .../Runtime/InteropServices/PosixSignalRegistration.Unix.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Unix.cs index dc65ae4e749c25..d91dd1c3a8dc75 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Unix.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Collections.Generic; +using System.Diagnostics; using System.Threading; namespace System.Runtime.InteropServices