Fix possible deadlocks on PosixSignalRegistration disposal on Windows

[mrek-msft]

Implements following proposal from #117753

What do you think about changing PosixSignalRegistration to not try to remove it's handler once it's been registered once?

Sounds good to me on Windows. I am not sure whether it is a good idea on Unix.

PR removes unregistration to prevent AB/BA deadlock (where A = lock (s_registrations), B = critical section in KernelBase.dll) when one thread is unregistering and other thread fires HandlerRoutine. The same can happen when registering, so I moved SetConsoleCtrlHandler registration from s_registrations locked section.

Also removes leaking handlers in Hosting because now we can call Dispose on them directly in Handler.

[Copilot]

Pull request overview

Updates Windows PosixSignalRegistration to reduce shutdown deadlock risk by ensuring the console control handler is only registered once and by removing handler unregistration.

Changes:

• Introduces s_isCtrlHandlerRegisteredOnce to prevent repeated SetConsoleCtrlHandler registrations.
• Moves SetConsoleCtrlHandler(Add: true) outside the s_registrations lock to avoid A→B / B→A deadlock scenarios.
• Removes SetConsoleCtrlHandler(Add: false) unregistration when the last registration is disposed.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

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

• The token is added to s_registrations even when the SetConsoleCtrlHandler registration failed or was skipped. If registerCtrlHandler is false (another thread is registering), the current thread will proceed to add its token to s_registrations before the other thread's SetConsoleCtrlHandler call completes. If that call fails, this registration will be in s_registrations but the handler won't be registered with Windows, causing signals to be silently ignored.

Consider adding the token to s_registrations only after successful handler registration, or checking s_isCtrlHandlerRegistering again and verifying the handler was successfully registered.

            lock (s_registrations)
            {
                s_isCtrlHandlerRegistering = false;

                if (!s_registrations.TryGetValue(signo, out List<Token>? tokens))
                {
                    s_registrations[signo] = tokens = new List<Token>();
                }

                tokens.Add(token);
            }

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

• The variable name s_isCtrlHandlerRegistering uses the present continuous tense (-ing form) suggesting an ongoing action, but it's set to true before registration starts and false after it completes (or fails), making it a state flag. Consider renaming to s_ctrlHandlerRegistrationInProgress or s_isRegisteringCtrlHandler for clarity, or use a simpler name like s_handlerRegistrationInProgress that better conveys it's a guard flag for the registration operation.

        private static bool s_isCtrlHandlerRegistering;

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

• The concurrent registration scenario where one thread's SetConsoleCtrlHandler call fails while another thread is waiting is not covered by tests. This is a critical scenario because it can lead to registrations being added to s_registrations without the handler actually being registered with Windows, causing signals to be silently ignored. Consider adding a test that simulates concurrent registration with one failing, or at least document this edge case if it's considered too difficult to test reliably.

            lock (s_registrations)
            {
                s_isCtrlHandlerRegistering = false;

                if (!s_registrations.TryGetValue(signo, out List<Token>? tokens))
                {
                    s_registrations[signo] = tokens = new List<Token>();
                }

                tokens.Add(token);
            }

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

---

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

---

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

---

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

---

You can also share your feedback on Copilot code review. Take the survey.

[mrek-msft]

@copilot please invoke the code-review skill and post the analysis/results as a comment on this PR.

[mrek-msft]

🤖 Copilot Code Review — PR #124893

Holistic Assessment

Motivation: The deadlock is real. The linked discussion in #117753 describes the AB/BA ordering between s_registrations and the Windows console handler critical section, and this PR addresses both the PosixSignalRegistration bug and the ConsoleLifetime shutdown fallout.

Approach: The chosen Windows-only approach is the one maintainers converged on in the issue: keep the native console handler registered once, stop unregistering it, and move SetConsoleCtrlHandler calls out of the managed registration lock. That removes the lock-order inversion without changing Unix behavior.

Summary: ✅ LGTM. I did not find any new blocking correctness or test gaps on the current head. I also checked the surrounding hosting/tests code and an independent Gemini review reached the same conclusion; the earlier bot comments I saw are resolved or outdated on the current diff.

---

Detailed Findings

✅ Deadlock fix — lock ordering looks correct on the current implementation

Register now serializes first-time native registration with s_registerLock, but performs SetConsoleCtrlHandler outside lock (s_registrations). Unregister no longer calls SetConsoleCtrlHandler(Add: false) at all. That removes both sides of the AB/BA deadlock described in #117753: the handler thread can still take s_registrations, and non-handler threads can still call SetConsoleCtrlHandler, but no path now holds s_registrations while entering the Windows console critical section.

✅ Hosting shutdown path — reverting the leak is appropriate after the CoreLib fix

ConsoleLifetime.HandleWindowsShutdown can now call UnregisterShutdownHandlers() directly instead of intentionally leaking the registrations. That matches the issue discussion and keeps the long-lived-host / repeated create-dispose scenario working without depending on finalization.

✅ Test quality — the new regression test exercises the risky path

SignalHandler_CanDisposeInHandler specifically validates the previously dangerous case: disposing a PosixSignalRegistration from inside its own handler, then re-sending the signal and verifying the process falls back to default handling. Together with the existing ConsoleLifetimeExitTests, this gives reasonable coverage of both the core primitive and the hosting consumer.

[jkotas]

cc @adamsitnik More fun with Windows signals

[jkotas]

Thanks

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.