Skip to content

Deliver Posix signals in reverse order of their registration #116557

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 12, 2025
+95 −66
Merged

Deliver Posix signals in reverse order of their registration #116557

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0070a8e
Deliver Posix signals in reverse order of their registration
jkotas Jun 11, 2025
bc0d5b3
Update src/libraries/System.Runtime.InteropServices/tests/System.Runt…
jkotas Jun 11, 2025
fcf65ca
Update src/libraries/System.Runtime.InteropServices/tests/System.Runt…
jkotas Jun 11, 2025
cae16d4
Revert change to stop signal processing
jkotas Jun 11, 2025
c5cb8b8
Add docs
jkotas Jun 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 20 additions & 12 deletions ...System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Member Author

@jkotas jkotas Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

List<Token> will require O(N) linear search for unregistration. I do not expect it to be a problem since the number of these registrations is expected to be fairly small.


private static unsafe Dictionary<int, HashSet<Token>> Initialize()
private static unsafe Dictionary<int, List<Token>> Initialize()
{
if (!Interop.Sys.InitializeTerminalAndSignalHandling())
{
Expand All @@ -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)
Expand All @@ -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 &&
Expand All @@ -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)
Expand All @@ -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);
Expand Down Expand Up @@ -122,16 +122,24 @@ 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 (context.Cancel)
{
// If any handler sets Cancel, we stop processing further handlers.
break;
}
}

if (!ctx.Cancel)
if (!context.Cancel)
{
Interop.Sys.HandleNonCanceledPosixSignal(signo);
}
Expand Down
103 changes: 49 additions & 54 deletions ...tem.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Member Author

@jkotas jkotas Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is structured the same way as the non-Windows signal registrations.


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)
Expand All @@ -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;
Expand All @@ -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);
Copy link
Member

@agocke agocke Jun 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand this correctly, token will almost always be at the end of this list. Do we want to search from the end to avoid quadratic behavior when removing all the tokens? It probably won't matter since there are likely < 10 signal handlers registered in the average application.

Copy link
Member

@agocke agocke Jun 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, nvm, I see no reason to assume that an arbitrary call to Unregister should be trying to remove the most recently registered signal.

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);
}
}
}
}
Expand All @@ -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;

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;
}
Token[]? tokens = null;

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);
}
}

Expand All @@ -109,10 +95,19 @@ 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);
if (context.Cancel)
{
// If any handler sets Cancel, we stop processing further handlers.
break;
}
}

return context.Cancel ? Interop.BOOL.TRUE : Interop.BOOL.FALSE;
Expand Down
35 changes: 35 additions & 0 deletions ...ropServices.UnitTests/System/Runtime/InteropServices/PosixSignalRegistrationTests.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 _ = 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()
{
Expand Down
Loading