-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Re-enable Microsoft.Extensions.Hosting shutdown functional coverage in CI and harden shutdown startup/Helix/NET481 compatibility #128252
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
base: main
Are you sure you want to change the base?
Changes from all commits
a12d2b9
6bc1499
b9cd702
609f415
3599810
6f852bf
a12c0f6
45143a9
8c11db2
8815010
2252aac
e8c2e66
ebd1e8f
306e3f5
637c6bd
5138a42
2cfc44b
01891b0
f9172b4
5986a65
16bc033
bd3dc0c
14a6f4c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
|
|
||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.ComponentModel; | ||
| using System.Diagnostics; | ||
| using System.IO; | ||
| using System.Runtime.InteropServices; | ||
|
|
@@ -11,6 +12,8 @@ namespace Microsoft.Extensions.Internal | |
| { | ||
| internal static class ProcessExtensions | ||
| { | ||
| private const int ESRCH = 3; | ||
| private const int SIGTERM = 15; | ||
| private static readonly bool _isWindows = RuntimeInformation.IsOSPlatform(OSPlatform.Windows); | ||
| private static readonly TimeSpan _defaultTimeout = TimeSpan.FromSeconds(30); | ||
|
|
||
|
|
@@ -41,11 +44,19 @@ public static void KillTree(this Process process, TimeSpan timeout) | |
|
|
||
| private static void GetAllChildIdsUnix(int parentId, ISet<int> children, TimeSpan timeout) | ||
| { | ||
| RunProcessAndWaitForExit( | ||
| "pgrep", | ||
| $"-P {parentId}", | ||
| timeout, | ||
| out var stdout); | ||
| string stdout; | ||
| try | ||
| { | ||
| RunProcessAndWaitForExit( | ||
| "pgrep", | ||
| $"-P {parentId}", | ||
| timeout, | ||
| out stdout); | ||
| } | ||
| catch (Win32Exception) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (!string.IsNullOrEmpty(stdout)) | ||
| { | ||
|
|
@@ -72,11 +83,62 @@ private static void GetAllChildIdsUnix(int parentId, ISet<int> children, TimeSpa | |
|
|
||
| private static void KillProcessUnix(int processId, TimeSpan timeout) | ||
| { | ||
| RunProcessAndWaitForExit( | ||
| "kill", | ||
| $"-TERM {processId}", | ||
| timeout, | ||
| out var stdout); | ||
| try | ||
| { | ||
| if (kill(processId, SIGTERM) != 0) | ||
| { | ||
| var error = Marshal.GetLastWin32Error(); | ||
| if (error != ESRCH) | ||
| { | ||
| KillProcessUnixHard(processId, timeout); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| using (Process process = Process.GetProcessById(processId)) | ||
| { | ||
| if (!process.WaitForExit((int)timeout.TotalMilliseconds)) | ||
| { | ||
| KillProcessUnixHard(processId, timeout); | ||
| } | ||
| } | ||
| } | ||
| catch (ArgumentException) | ||
| { | ||
| // Ignore if process has already exited. | ||
| } | ||
| catch (InvalidOperationException) | ||
| { | ||
| // Ignore if process has already exited. | ||
| } | ||
| catch (Win32Exception) | ||
| { | ||
| KillProcessUnixHard(processId, timeout); | ||
| } | ||
| } | ||
|
|
||
| private static void KillProcessUnixHard(int processId, TimeSpan timeout) | ||
| { | ||
| try | ||
| { | ||
| using (Process process = Process.GetProcessById(processId)) | ||
| { | ||
| process.Kill(); | ||
| process.WaitForExit((int)timeout.TotalMilliseconds); | ||
| } | ||
|
Comment on lines
+86
to
+104
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @copilot , use the kill -TERM command but keep the other changes wrapping the call. |
||
| } | ||
| catch (ArgumentException) | ||
| { | ||
| // Ignore if process has already exited. | ||
| } | ||
| catch (InvalidOperationException) | ||
| { | ||
| // Ignore if process has already exited. | ||
| } | ||
| catch (Win32Exception) | ||
| { | ||
| // Ignore permission or process-not-found errors. | ||
| } | ||
|
Comment on lines
+110
to
+141
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @copilot, implement the fix.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implemented in |
||
| } | ||
|
|
||
| private static void RunProcessAndWaitForExit(string fileName, string arguments, TimeSpan timeout, out string stdout) | ||
|
|
@@ -102,5 +164,8 @@ private static void RunProcessAndWaitForExit(string fileName, string arguments, | |
| process.Kill(); | ||
| } | ||
| } | ||
|
|
||
| [DllImport("libc", SetLastError = true)] | ||
| private static extern int kill(int pid, int sig); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,8 +2,8 @@ | |
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System; | ||
| using System.ComponentModel; | ||
| using System.Diagnostics; | ||
| using System.IO; | ||
| using System.Runtime.InteropServices; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.Extensions.Hosting.IntegrationTesting; | ||
|
|
@@ -16,6 +16,7 @@ namespace Microsoft.AspNetCore.Hosting.FunctionalTests | |
| { | ||
| public class ShutdownTests | ||
| { | ||
| private const int SIGINT = 2; | ||
| private static readonly string StartedMessage = "Started"; | ||
| private static readonly string CompletionMessage = "Stopping firing\n" + | ||
| "Stopping end\n" + | ||
|
|
@@ -50,34 +51,30 @@ private async Task ExecuteShutdownTest(string testName, string shutdownMechanic) | |
| builder.AddXunit(_output); | ||
| }); | ||
|
|
||
| // TODO refactor deployers to not depend on source code | ||
| // see https://github.com/dotnet/extensions/issues/1697 and https://github.com/dotnet/aspnetcore/issues/10268 | ||
| #pragma warning disable 0618 | ||
| var applicationPath = string.Empty; // disabled for now | ||
| #pragma warning restore 0618 | ||
| string applicationPath = AppContext.BaseDirectory; | ||
|
|
||
| Version version = Environment.Version; | ||
| var deploymentParameters = new DeploymentParameters( | ||
| applicationPath, | ||
| RuntimeFlavor.CoreClr, | ||
| RuntimeArchitecture.x64) | ||
| { | ||
| ApplicationName = "Microsoft.Extensions.Hosting.TestApp", | ||
| TargetFramework = $"net{version.Major}.{version.Minor}", | ||
| ApplicationType = ApplicationType.Portable, | ||
| PublishApplicationBeforeDeployment = true, | ||
| StatusMessagesEnabled = false | ||
| }; | ||
| deploymentParameters.ApplicationPublisher = new ExistingOutputApplicationPublisher(applicationPath); | ||
|
Comment on lines
+62
to
+68
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @copilot , implement the suggestion.
rosebyte marked this conversation as resolved.
|
||
|
|
||
| deploymentParameters.EnvironmentVariables["DOTNET_STARTMECHANIC"] = shutdownMechanic; | ||
|
|
||
| using (var deployer = new SelfHostDeployer(deploymentParameters, xunitTestLoggerFactory)) | ||
| { | ||
| var result = await deployer.DeployAsync(); | ||
|
|
||
| var started = new TaskCompletionSource<int>(TaskCreationOptions.RunContinuationsAsynchronously); | ||
| var completed = new TaskCompletionSource<int>(TaskCreationOptions.RunContinuationsAsynchronously); | ||
| var output = string.Empty; | ||
| deployer.HostProcess.OutputDataReceived += (sender, args) => | ||
| deployer.OutputReceived += (sender, args) => | ||
| { | ||
| if (!string.IsNullOrEmpty(args.Data) && args.Data.StartsWith(StartedMessage)) | ||
| { | ||
|
|
@@ -95,11 +92,13 @@ private async Task ExecuteShutdownTest(string testName, string shutdownMechanic) | |
| } | ||
| }; | ||
|
|
||
| await started.Task.WaitAsync(TimeSpan.FromSeconds(60)); | ||
| await deployer.DeployAsync(); | ||
|
|
||
| await started.Task.WaitAsync(TimeSpan.FromSeconds(180)); | ||
|
|
||
| SendShutdownSignal(deployer.HostProcess); | ||
|
|
||
| await completed.Task.WaitAsync(TimeSpan.FromSeconds(60)); | ||
| await completed.Task.WaitAsync(TimeSpan.FromSeconds(180)); | ||
|
|
||
| WaitForExitOrKill(deployer.HostProcess); | ||
|
|
||
|
|
@@ -132,16 +131,10 @@ private void SendShutdownSignal(Process hostProcess) | |
|
|
||
| private static void SendSIGINT(int processId) | ||
| { | ||
| var startInfo = new ProcessStartInfo | ||
| if (kill(processId, SIGINT) != 0) | ||
| { | ||
| FileName = "kill", | ||
| Arguments = processId.ToString(), | ||
| RedirectStandardOutput = true, | ||
| UseShellExecute = false | ||
| }; | ||
|
|
||
| var process = Process.Start(startInfo); | ||
| WaitForExitOrKill(process); | ||
| throw new Win32Exception(Marshal.GetLastWin32Error()); | ||
| } | ||
| } | ||
|
|
||
| private static void WaitForExitOrKill(Process process) | ||
|
|
@@ -150,9 +143,37 @@ private static void WaitForExitOrKill(Process process) | |
| if (!process.HasExited) | ||
| { | ||
| process.Kill(); | ||
| // Wait for the process to actually exit after Kill() before accessing ExitCode | ||
| if (!process.WaitForExit(5000)) | ||
|
Comment on lines
143
to
+147
|
||
| { | ||
| throw new InvalidOperationException($"Process {process.Id} did not exit within timeout after Kill()"); | ||
| } | ||
| } | ||
|
|
||
| Assert.Equal(0, process.ExitCode); | ||
| } | ||
|
|
||
| private sealed class ExistingOutputApplicationPublisher : ApplicationPublisher | ||
| { | ||
| public ExistingOutputApplicationPublisher(string applicationPath) | ||
| : base(applicationPath) | ||
| { | ||
| } | ||
|
|
||
| public override Task<PublishedApplication> Publish(DeploymentParameters deploymentParameters, ILogger logger) | ||
| => Task.FromResult<PublishedApplication>(new BorrowedPublishedApplication(ApplicationPath, logger)); | ||
|
|
||
| // Wraps a path that is borrowed (not owned) from the test output directory. | ||
| // Dispose is intentionally a no-op to prevent deleting AppContext.BaseDirectory. | ||
| private sealed class BorrowedPublishedApplication : PublishedApplication | ||
| { | ||
| public BorrowedPublishedApplication(string path, ILogger logger) : base(path, logger) { } | ||
|
|
||
| public override void Dispose() { } | ||
| } | ||
| } | ||
|
|
||
| [DllImport("libc", SetLastError = true)] | ||
| private static extern int kill(int pid, int sig); | ||
| } | ||
| } | ||
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.
@copilot, OK, do it that way.
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.
Addressed in
16bc0339:KillProcessUnixno longer throws on SIGTERM timeout; it now escalates toProcess.Kill()(hard kill) as best-effort cleanup instead of failing disposal.