Deliver Posix signals in reverse order of their registration (#116557)

Fixes #116556

Author: jkotas

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Unix.cs

@@ -9,9 +9,9 @@ namespace System.Runtime.InteropServices
 {
     public sealed partial class PosixSignalRegistration
     {
-        private static readonly Dictionary<int, HashSet<Token>> s_registrations = Initialize();
+        private static readonly Dictionary<int, List<Token>> s_registrations = Initialize();
 
-        private static unsafe Dictionary<int, HashSet<Token>> Initialize()
+        private static unsafe Dictionary<int, List<Token>> Initialize()
         {
             if (!Interop.Sys.InitializeTerminalAndSignalHandling())
             {
@@ -20,7 +20,7 @@ private static unsafe Dictionary<int, HashSet<Token>> Initialize()
 
             Interop.Sys.SetPosixSignalHandler(&OnPosixSignal);
 
-            return new Dictionary<int, HashSet<Token>>();
+            return new Dictionary<int, List<Token>>();
         }
 
         private static PosixSignalRegistration Register(PosixSignal signal, Action<PosixSignalContext> handler)
@@ -36,9 +36,9 @@ private static PosixSignalRegistration Register(PosixSignal signal, Action<Posix
 
             lock (s_registrations)
             {
-                if (!s_registrations.TryGetValue(signo, out HashSet<Token>? tokens))
+                if (!s_registrations.TryGetValue(signo, out List<Token>? tokens))
                 {
-                    s_registrations[signo] = tokens = new HashSet<Token>();
+                    s_registrations[signo] = tokens = new List<Token>();
                 }
 
                 if (tokens.Count == 0 &&
@@ -61,7 +61,7 @@ private void Unregister()
                 {
                     _token = null;
 
-                    if (s_registrations.TryGetValue(token.SigNo, out HashSet<Token>? tokens))
+                    if (s_registrations.TryGetValue(token.SigNo, out List<Token>? tokens))
                     {
                         tokens.Remove(token);
                         if (tokens.Count == 0)
@@ -81,7 +81,7 @@ private static int OnPosixSignal(int signo, PosixSignal signal)
 
             lock (s_registrations)
             {
-                if (s_registrations.TryGetValue(signo, out HashSet<Token>? registrations))
+                if (s_registrations.TryGetValue(signo, out List<Token>? registrations))
                 {
                     tokens = new Token[registrations.Count];
                     registrations.CopyTo(tokens);
@@ -122,16 +122,19 @@ static void HandleSignal(object? state)
             {
                 (int signo, Token[] tokens) = ((int, Token[]))state!;
 
-                PosixSignalContext ctx = new(0);
-                foreach (Token token in tokens)
+                PosixSignalContext context = new(0);
+
+                // Iterate through the tokens in reverse order to match the order of registration.
+                for (int i = tokens.Length - 1; i >= 0; i--)
                 {
+                    Token token = tokens[i];
                     // Different values for PosixSignal map to the same signo.
                     // Match the PosixSignal value used when registering.
-                    ctx.Signal = token.Signal;
-                    token.Handler(ctx);
+                    context.Signal = token.Signal;
+                    token.Handler(context);
                 }
 
-                if (!ctx.Cancel)
+                if (!context.Cancel)
                 {
                     Interop.Sys.HandleNonCanceledPosixSignal(signo);
                 }

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Windows.cs

@@ -8,23 +8,20 @@ namespace System.Runtime.InteropServices
 {
     public sealed partial class PosixSignalRegistration
     {
-        private static readonly HashSet<Token> s_registrations = new();
+        private static readonly Dictionary<int, List<Token>> s_registrations = new();
 
         private static unsafe PosixSignalRegistration Register(PosixSignal signal, Action<PosixSignalContext> handler)
         {
-            switch (signal)
+            int signo = signal switch
             {
-                case PosixSignal.SIGINT:
-                case PosixSignal.SIGQUIT:
-                case PosixSignal.SIGTERM:
-                case PosixSignal.SIGHUP:
-                    break;
-
-                default:
-                    throw new PlatformNotSupportedException();
-            }
-
-            var token = new Token(signal, handler);
+                PosixSignal.SIGINT => Interop.Kernel32.CTRL_C_EVENT,
+                PosixSignal.SIGQUIT => Interop.Kernel32.CTRL_BREAK_EVENT,
+                PosixSignal.SIGTERM => Interop.Kernel32.CTRL_SHUTDOWN_EVENT,
+                PosixSignal.SIGHUP => Interop.Kernel32.CTRL_CLOSE_EVENT,
+                _ => throw new PlatformNotSupportedException()
+            };
+
+            var token = new Token(signal, signo, handler);
             var registration = new PosixSignalRegistration(token);
 
             lock (s_registrations)
@@ -35,7 +32,12 @@ private static unsafe PosixSignalRegistration Register(PosixSignal signal, Actio
                     throw Win32Marshal.GetExceptionForLastWin32Error();
                 }
 
-                s_registrations.Add(token);
+                if (!s_registrations.TryGetValue(signo, out List<Token>? tokens))
+                {
+                    s_registrations[signo] = tokens = new List<Token>();
+                }
+
+                tokens.Add(token);
             }
 
             return registration;
@@ -49,17 +51,25 @@ private unsafe void Unregister()
                 {
                     _token = null;
 
-                    s_registrations.Remove(token);
-                    if (s_registrations.Count == 0 &&
-                        !Interop.Kernel32.SetConsoleCtrlHandler(&HandlerRoutine, Add: false))
+                    if (s_registrations.TryGetValue(token.SigNo, out List<Token>? tokens))
                     {
-                        // Ignore errors due to the handler no longer being registered; this can happen, for example, with
-                        // direct use of Alloc/Attach/FreeConsole which result in the table of control handlers being reset.
-                        // Throw for everything else.
-                        int error = Marshal.GetLastPInvokeError();
-                        if (error != Interop.Errors.ERROR_INVALID_PARAMETER)
+                        tokens.Remove(token);
+                        if (tokens.Count == 0)
                         {
-                            throw Win32Marshal.GetExceptionForWin32Error(error);
+                            s_registrations.Remove(token.SigNo);
+                        }
+
+                        if (s_registrations.Count == 0 &&
+                            !Interop.Kernel32.SetConsoleCtrlHandler(&HandlerRoutine, Add: false))
+                        {
+                            // Ignore errors due to the handler no longer being registered; this can happen, for example, with
+                            // direct use of Alloc/Attach/FreeConsole which result in the table of control handlers being reset.
+                            // Throw for everything else.
+                            int error = Marshal.GetLastPInvokeError();
+                            if (error != Interop.Errors.ERROR_INVALID_PARAMETER)
+                            {
+                                throw Win32Marshal.GetExceptionForWin32Error(error);
+                            }
                         }
                     }
                 }
@@ -69,38 +79,14 @@ private unsafe void Unregister()
         [UnmanagedCallersOnly]
         private static Interop.BOOL HandlerRoutine(int dwCtrlType)
         {
-            PosixSignal signal;
-            switch (dwCtrlType)
-            {
-                case Interop.Kernel32.CTRL_C_EVENT:
-                    signal = PosixSignal.SIGINT;
-                    break;
+            Token[]? tokens = null;
 
-                case Interop.Kernel32.CTRL_BREAK_EVENT:
-                    signal = PosixSignal.SIGQUIT;
-                    break;
-
-                case Interop.Kernel32.CTRL_SHUTDOWN_EVENT:
-                    signal = PosixSignal.SIGTERM;
-                    break;
-
-                case Interop.Kernel32.CTRL_CLOSE_EVENT:
-                    signal = PosixSignal.SIGHUP;
-                    break;
-
-                default:
-                    return Interop.BOOL.FALSE;
-            }
-
-            List<Token>? tokens = null;
             lock (s_registrations)
             {
-                foreach (Token token in s_registrations)
+                if (s_registrations.TryGetValue(dwCtrlType, out List<Token>? registrations))
                 {
-                    if (token.Signal == signal)
-                    {
-                        (tokens ??= new List<Token>()).Add(token);
-                    }
+                    tokens = new Token[registrations.Count];
+                    registrations.CopyTo(tokens);
                 }
             }
 
@@ -109,10 +95,14 @@ private static Interop.BOOL HandlerRoutine(int dwCtrlType)
                 return Interop.BOOL.FALSE;
             }
 
-            var context = new PosixSignalContext(signal);
-            foreach (Token handler in tokens)
+            var context = new PosixSignalContext(0);
+
+            // Iterate through the tokens in reverse order to match the order of registration.
+            for (int i = tokens.Length - 1; i >= 0; i--)
             {
-                handler.Handler(context);
+                Token token = tokens[i];
+                context.Signal = token.Signal;
+                token.Handler(context);
             }
 
             return context.Cancel ? Interop.BOOL.TRUE : Interop.BOOL.FALSE;

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.cs

@@ -24,6 +24,7 @@ public sealed partial class PosixSignalRegistration : IDisposable
         /// <exception cref="PlatformNotSupportedException"><paramref name="signal"/> is not supported by the platform.</exception>
         /// <exception cref="IOException">An error occurred while setting up the signal handling or while installing the handler for the specified signal.</exception>
         /// <remarks>
+        /// The handlers are executed in reverse order of their registration.
         /// Raw values can be provided for <paramref name="signal"/> on Unix by casting them to <see cref="PosixSignal"/>.
         /// Default handling of the signal can be canceled through <see cref="PosixSignalContext.Cancel"/>.
         /// <see cref="PosixSignal.SIGINT"/> and <see cref="PosixSignal.SIGQUIT"/> can be canceled on both

src/libraries/System.Runtime.InteropServices/tests/System.Runtime.InteropServices.UnitTests/System/Runtime/InteropServices/PosixSignalRegistrationTests.Unix.cs

@@ -125,6 +125,41 @@ public void SignalHandlerWorksForSecondRegistration()
             }
         }
 
+        [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotMobile))]
+        public void SignalHandlersCalledInReverseOrder()
+        {
+            PosixSignal signal = PosixSignal.SIGCONT;
+            bool secondHandlerCalled = false;
+
+            using SemaphoreSlim semaphore = new(0);
+            using var first = PosixSignalRegistration.Create(signal, ctx =>
+            {
+                Assert.Equal(signal, ctx.Signal);
+
+                // Ensure signal doesn't cause the process to terminate.
+                ctx.Cancel = true;
+
+                Assert.True(secondHandlerCalled);
+
+                semaphore.Release();
+            });
+
+            using var second = PosixSignalRegistration.Create(signal, ctx =>
+            {
+                Assert.Equal(signal, ctx.Signal);
+
+                // Ensure signal doesn't cause the process to terminate.
+                ctx.Cancel = true;
+
+                Assert.False(secondHandlerCalled);
+                secondHandlerCalled = true;
+            });
+
+            kill(signal);
+            bool entered = semaphore.Wait(SuccessTimeout);
+            Assert.True(entered);
+        }
+
         [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotMobile))]
         public void SignalHandlerNotCalledWhenDisposed()
         {