Deliver Posix signals in reverse order of their registration #116557
Conversation
|
Tagging subscribers to this area: @dotnet/interop-contrib |
| @@ -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(); | |||
There was a problem hiding this comment.
This is structured the same way as the non-Windows signal registrations.
| @@ -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(); | |||
There was a problem hiding this comment.
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.
...nteropServices.UnitTests/System/Runtime/InteropServices/PosixSignalRegistrationTests.Unix.cs
Outdated
Show resolved
Hide resolved
…ime.InteropServices.UnitTests/System/Runtime/InteropServices/PosixSignalRegistrationTests.Unix.cs
...nteropServices.UnitTests/System/Runtime/InteropServices/PosixSignalRegistrationTests.Unix.cs
Outdated
Show resolved
Hide resolved
…ime.InteropServices.UnitTests/System/Runtime/InteropServices/PosixSignalRegistrationTests.Unix.cs
|
Would you mind adding a note to documentation of the registration API that specifies the order of execution of the handlers? So that we don't rely on undocumented behavior in dotnet-watch (or elsewhere). |
It is unnecessary behavior change
...es/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Unix.cs
Outdated
Show resolved
Hide resolved
| @@ -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. | |||
There was a problem hiding this comment.
I will submit this to the official docs too once this is merged.
| // Throw for everything else. | ||
| int error = Marshal.GetLastPInvokeError(); | ||
| if (error != Interop.Errors.ERROR_INVALID_PARAMETER) | ||
| tokens.Remove(token); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah, nvm, I see no reason to assume that an arbitrary call to Unregister should be trying to remove the most recently registered signal.
|
/ba-g runtime (Build android-x64 Release AllSubsets_CoreCLR) timing out on many PRs |
Fixes #116556