-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Do not return from CTRL_SHUTDOWN_EVENT #116652
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an issue where the process is killed immediately after handling CTRL_SHUTDOWN_EVENT on Windows by introducing a delay in the shutdown handler. Key changes include enabling unsafe blocks in the test project, modifying the ConsoleLifetimeExitTests to simulate Windows signals via a pipe, and updating the Windows SIGTERM handler in ConsoleLifetime to delay its return.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/libraries/Microsoft.Extensions.Hosting/tests/UnitTests/Microsoft.Extensions.Hosting.Unit.Tests.csproj | Enabled unsafe blocks for tests |
src/libraries/Microsoft.Extensions.Hosting/tests/UnitTests/ConsoleLifetimeExitTests.cs | Updated signal simulation logic for Windows using an anonymous pipe and adjusted RemoteExecutor parameters |
src/libraries/Microsoft.Extensions.Hosting/src/Internal/ConsoleLifetime.netcoreapp.cs | Modified SIGTERM registration to use a dedicated Windows shutdown handler that delays exit via Thread.Sleep |
Comments suppressed due to low confidence (2)
src/libraries/Microsoft.Extensions.Hosting/tests/UnitTests/ConsoleLifetimeExitTests.cs:120
- Review the use of the array initializer [ctrlType] in the method invocation. Although valid under new C# syntax, please double-check that it compiles as intended and produces the expected invocation behavior.
handlerMethod.Invoke(null, [ctrlType]);
src/libraries/Microsoft.Extensions.Hosting/src/Internal/ConsoleLifetime.netcoreapp.cs:51
- The shutdown handler invokes Thread.Sleep using HostOptions.ShutdownTimeout to delay returning, but the test expects an exit code of 123. Verify that the use of Environment.FailFast in the signal simulation (for SIGTERM) does not conflict with the expected process exit behavior in a Windows environment.
Thread.Sleep(HostOptions.ShutdownTimeout);
Tagging subscribers to this area: @dotnet/area-extensions-hosting |
cdd3e3a
to
4e05c94
Compare
Windows will kill the process after it completes handling CTRL_SHUTDOWN_EVENT To avoid this, we add a delay to our handler. This allows the process to exit before the handler returns. We use HostOptions.ShutdownTimeout for the delay. This does mean that other SIGTERM handlers will not run, but that's a tradeoff we have to make to ensure the process isn't killed.
4e05c94
to
2c3c15b
Compare
src/libraries/Microsoft.Extensions.Hosting/tests/UnitTests/ConsoleLifetimeExitTests.cs
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Hosting/tests/UnitTests/ConsoleLifetimeExitTests.cs
Show resolved
Hide resolved
The change in the shipping code looks good to me. |
src/libraries/Microsoft.Extensions.Hosting/tests/UnitTests/ConsoleLifetimeExitTests.cs
Show resolved
Hide resolved
Should you also use proof of concept programThis shows the finalization of using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
static class Program
{
static readonly ManualResetEvent s_exiting = new ManualResetEvent(false);
static PosixSignalRegistration? s_sigTermReg;
static PosixSignalRegistration? s_sigIntReg;
static PosixSignalRegistration? s_sigQuitReg;
[MethodImpl(MethodImplOptions.NoInlining)]
private static void CreateReg()
{
s_sigTermReg = PosixSignalRegistration.Create(PosixSignal.SIGTERM, HandleSigTerm);
s_sigIntReg = PosixSignalRegistration.Create(PosixSignal.SIGINT, HandleSigTerm);
s_sigQuitReg = PosixSignalRegistration.Create(PosixSignal.SIGQUIT, HandleSigTerm);
}
static void Main(string[] args)
{
Console.WriteLine($"started {Environment.ProcessId}");
CreateReg();
s_exiting.WaitOne();
Thread.Sleep(100);
Console.WriteLine("exiting main");
// Make sure the PosixSignalRegistration are ready to finialize
for (int i = 0; i < 10; i++)
{
GC.Collect();
}
// Give the finializer thread some time to start and get blocked.
Thread.Sleep(100);
}
static void HandleSigTerm(PosixSignalContext ctx)
{
Console.WriteLine($"Got {ctx.Signal}");
s_sigTermReg = null;
s_sigIntReg = null;
s_sigQuitReg = null;
s_exiting.Set();
Thread.Sleep(Timeout.Infinite);
}
} Re: writing a more realistic unit test on Windows, you could use the |
@AustinWise Good catch!
Keeping these registrations alive to prevent their finalization would be more appropriate. Calling Even with the finalization suppressed, there seems to be an opportunity for dead locks: If |
I have opened #117753 on the potential deadlocks so that it does not get lost. |
Fixes #115206
Windows will kill the process after it completes handling CTRL_SHUTDOWN_EVENT
To avoid this, we add a delay to our handler. This allows the process to exit before the handler returns.
We use HostOptions.ShutdownTimeout for the delay. This does mean that other SIGTERM handlers will not run, but that's a tradeoff we have to make to ensure the process isn't killed.