Potential shutdown deadlocks in Microsoft.Extensions.Hosting on Windows

[jkotas]

Introduced by #116652 - see discussion in #116652 (comment).

• We should keep the PosixSignalRegistrations alive to prevent them from getting finalized in case GC collects them during shutdown.

• We may want to dispose PosixSignalRegistrations on a separate thread to avoid A-B/B-A deadlock between the thread trying dispose PosixSignalRegistration and the signal handler thread in case the signal is delivered while the application is shutting down

[ericstj]

From @jkotas

Should you also use GC.SuppressFinalize on the PosixSignalRegistration?

Keeping these registrations alive to prevent their finalization would be more appropriate. Calling GC.SuppressFinalize on a type that you do not own is not the best practice.

Even with the finalization suppressed, there seems to be an opportunity for dead locks: If ConsoleLifetime.Dispose gets called at the same as the signal is delivered, ConsoleLifetime.Dispose will deadlock on the Windows lock trying to unregister the handlers. We may need some sort of handshake between ConsoleLifetime.Dispose and the signal handler to prevent this.

I'm not sure if we can ever anticipate when the OS will invoke the handler. There will always be the case where the OS has already taken the lock when invoking shutdown handler before it's called us.

I think we have to live with the potential race where we have already entered Dispose when the OS is about to call our handler. We should focus on trying to fix the case where "normal shutdown" might end up trying to do finalization.

Even if we do something in ConsoleLifetime to avoid finalizing these so long as ConsoleLifetime is alive, that doesn't prevent the finalizer from eventually running on the PosixSignalRegistration types, which will always deadlock without any chance for ConsoleLifetime to intervene. Suppose someone has a main routine that tries to add back a wait on GC.WaitForPendingFinalizers after all the hosting stuff is out of scope.

I think we might have to suppress finalization on these. Or were you imagining we create some static to "intetionally leak" these?

[jkotas]

I have suggested above to dispose PosixSignalRegistrations on a separate thread.

If there is simultaneously delivered signal callback, the separate thread can get blocked - but it won't prevent ConsoleLifetime shutdown from making a progress. The separate thread is likely to be blocked till the application exits in this case. That's fine, it is ok to exit without disposed PosixSignalRegistrations.

if there is no simultaneously delivered signal callback, the separate thread will dispose the registration, ConsoleLifetime shutdowns and any code that runs after ConsoleLifetime shutdown can continue executing. The scenario that I have in mind here are tests that startup and shutdown ConsoleLifetime lifetime multiple times per process. Is it a valid scenario?

[ericstj]

Not through Shutdown event. That only can happen once then the process is killed. There's literally no way to dispose these after the handler is invoked for shutdown. The OS holds the lock until process is killed.

I understand the issue that @AustinWise mentioned. I can reproduce in existing tests by simply making main thread block on finalizers. Maybe an elegant solution is to just keepalive the registrations in the handler after nulling out the fields. This will effectively leak them while still allowing main to exit and ConsoleLifetime to be disposed.

[jkotas]

I agree that keeping the registrations alive is going to fix the dead lock with repro. It is not going to fix other potential deadlocks like this:

        private partial void UnregisterShutdownHandlers()
        {
                 <--- If the application is shutting down for other reasons and the shutdown signal is delivered here, application shutdown is going to deadlock. Shutdown signal is going to wait for the application to shutdown, and the Dispose on the next line is going to wait for the lock hold by the signal thread.

            _sigIntRegistration?.Dispose();
            _sigQuitRegistration?.Dispose();
            _sigTermRegistration?.Dispose();
        }

It is hard to write reliable repros for these other deadlocks.

I believe both the dead lock with repro and the other potential deadlocks can be fixed by doing this:

        private partial void UnregisterShutdownHandlers()
        {
            if (OperatingSystem.IsWindows())
            {
                 // Windows has a lock that prevents the unregistration while this signal handler is running
                 // Unregister the handlers on a separate thread to avoid deadlocks with signal handler
                 // that block until the application shutdowns.
                 new Thread(() => {
                    _sigIntRegistration?.Dispose();
                    _sigQuitRegistration?.Dispose();
                    _sigTermRegistration?.Dispose();
                 }).UnsafeStart();
            }
            else
            {
                _sigIntRegistration?.Dispose();
                _sigQuitRegistration?.Dispose();
                _sigTermRegistration?.Dispose();
            }
        }

And delete the code that nulls out the registrations in HandleWindowsShutdown.

[ericstj]

I see what you mean - I'm still not sure that Thread would ever run. It would get blocked on the first which would still leave others pending finalization. They'd still be rooted by that hung thread so maybe it wouldn't block the finalizer queue, but it feels like a worse hack.

I was able to test one solution to the problem which kept the registration active ericstj@f8952b1. This did reproduce and fix the issue.

Then as part of researching what the OS is doing here -- I see it's using critical section. The deadlock happens because we were previously trying to let the main thread do the Dispose so it couldn't acquire the critical section as the callback thread held it. If instead we had the callback dispose the registrations I think they wouldn't deadlock since the callback thread already holds the critical section.

I think this might be the cleanest fix. Have the shutdown callback dispose all the registrations itself. I need to try this out with an actual shutdown repro (resurrecting my docker environment) rather than the simulation, but I think it should work.

[jkotas]

Have the shutdown callback dispose all the registrations itself.

It is valid to create and dispose the default host (that comes with ConsoleLifetime) many times in a single process today. For example:

using Microsoft.Extensions.Hosting;

for (int i = 0; i < 1000; i++)
    await RunOnce();

async Task RunOnce()
{
    using (var host = Host.CreateDefaultBuilder().Build())
    {
        await host.StartAsync();
        // do stuff
        await host.StopAsync();
    }
}

Do we care about this working about as good as it works today (without memory leaks or missing Dispose calls)? If we have the shutdown callback dispose the registrations, this repro will start depending on cleaning up the registrations on the finalizer thread.

[ericstj]

With my suggested fix above (not the linked one, but what I describe) to have the shutdown handler to early dispose of the registrations it will work just as it does today. We won't change any of the other dispose logic, it will just mean that if the shutdown handler is invoked by Windows we'll dispose of the registrations at that point.

[jkotas]

With my suggested fix above (not the linked one, but what I describe) to have the shutdown handler to early dispose of the registrations it will work just as it does today. We won't change any of the other dispose logi

Is this fix going to suffer from a dead lock that I have described in #117753 (comment) ?

[ericstj]

The only way the shutdown handler will ever be invoked is if the OS already holds the critical section required for calling SetConsoleCtrlHandler -- so it should never deadlock if all we do is try to call SetConsoleCtrlHandler. Unfortunately if we go through PosixSignalRegistration.Dispose that needs another lock.

If any other thread in the process has already entered into

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

Line 48 in b5d37d1

lock (s_registrations)

then we'd deadlock the handler thread if we tried to call that, and the other thread that ends up calling SetConsoleCtrlHandler is already going to be blocked until the process is killed.

The problem here is lock ordering. The design of PosixSignalRegistration requires acquiring a lock on s_registrations before implicitly acquiring a lock when calling SetConsoleCtrlHandler. The OS cannot acquire our s_registrations lock before it acquires it's own critical section when invoking our shutdown handler.

There's an even more subtle deadlock here due to the ordering problem, which doesn't involve ConsoleLifetime at all. In PosixSignalRegistration.HandlerRoutine we do try to acquire the s_registrations lock.

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

Lines 84 to 91 in f4f3be6

	lock (s_registrations)
	{
	if (s_registrations.TryGetValue(dwCtrlType, out List<Token>? registrations))
	{
	tokens = new Token[registrations.Count];
	registrations.CopyTo(tokens);
	}
	}

Suppose we have one thread with the OS acquiring the critical section, then another thread in the process acquires s_registrations lock - neither thread can proceed since the handler thread cannot acquire s_registrations and the other thread cannot get the critical section. Its an AB/BA lock ordering bug.

Once the OS has acquired the shutdown handler critical section, there is no way to call SetConsoleCtrlHandler except from the shutdown handler thread. The process will not be able to predict this either - since the OS might acquire the critical section at any point and the only way to observe that is that a call to SetConsoleCtrlHandler will hang.

I didn't like the solution that kicks off a thread for UnregisterShutdownHandlers since that thread will never complete. It also doesn't fix hang from a finalizer running (like I showed in the change above). This is what I meant by

I'm not sure if we can ever anticipate when the OS will invoke the handler. There will always be the case where the OS has already taken the lock when invoking shutdown handler before it's called us.

What do you think about changing PosixSignalRegistration to not try to remove it's handler once it's been registered once? We can avoid the lock ordering bug this way, since we know a custom handler should never be running if we only ever call SetConsoleCtrlHandler once, while holding the s_registrations lock.