Skip to content

[Spa] Try additional hooks to kill the application #34427

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Aug 17, 2021
Merged

[Spa] Try additional hooks to kill the application #34427

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
da3cf33
[Spa] Try additional hooks to kill the application
javiercn Jul 16, 2021
44416ed
Use powershell script to monitor the .NET process from outside and to…
javiercn Aug 17, 2021
8c3291a
Fix missing header
javiercn Aug 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 47 additions & 9 deletions src/Middleware/Spa/SpaProxy/src/SpaProxyLaunchManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,12 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using System;
using System.Diagnostics;
using System.IO;
using System.Net.Http;
using System.Net.Http.Headers;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;

namespace Microsoft.AspNetCore.SpaProxy
{
Expand All @@ -25,16 +22,16 @@ internal class SpaProxyLaunchManager : IDisposable

public SpaProxyLaunchManager(
ILogger<SpaProxyLaunchManager> logger,
IHostApplicationLifetime appLifetime,
IOptions<SpaDevelopmentServerOptions> options)
{
_options = options.Value;
_logger = logger;
appLifetime.ApplicationStopping.Register(() => Dispose(true));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So SpaProxyLaunchManager is a service in DI but Dispose isn't getting called on app shutdown for some reason?

Who normally calls StopAsync?

Do you want to use ApplicationStopped instead? Requests may still be in flight otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Tratcher the problem here is that the debugger kills the app and leaves the process running.

We don't get a chance to kill the SPA process ourselves. This change just brings the current implementation more inline with the old implementation, although I suspect both suffer from the same issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I recall correctly, the debugger kills processes in the rudest way possible with no chance to run shutdown logic. What you really need is a way to register the child process with the debugger as something it should also terminate. Maybe if you launched the child process with the debugger attached?

https://docs.microsoft.com/en-us/visualstudio/debugger/debug-multiple-processes?view=vs-2019#stop-debugging-with-multiple-processes

I see some prior art here:
#5204
#11597
#17883 (claims ApplicationStopping worked for them)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of this things work, people thought they worked because they didn't see the window (or didn't try with the debugger).

I found a reasonable solution which is simple enough I'm happy with. We launch a powershell process that monitors the .NET process and when it sees it die, it kills the npm process and all its children, and that does the trick.

}

public void StartInBackground(CancellationToken cancellationToken)
{
_logger.LogInformation($"No SPA development server running at {_options.ServerUrl} found.");

// We are not waiting for the SPA proxy to launch, instead we are going to rely on a piece of
// middleware to display an HTML document while the SPA proxy is not ready, refresh every three
// seconds and redirect to the SPA proxy url once it is ready.
Expand All @@ -45,6 +42,7 @@ public void StartInBackground(CancellationToken cancellationToken)
{
if (_launchTask == null)
{
_logger.LogInformation($"No SPA development server running at {_options.ServerUrl} found.");
_launchTask = UpdateStatus(StartSpaProcessAndProbeForLiveness(cancellationToken));
}
}
Expand Down Expand Up @@ -190,6 +188,46 @@ private void LaunchDevelopmentProxy()
WorkingDirectory = Path.Combine(AppContext.BaseDirectory, _options.WorkingDirectory)
};
_spaProcess = Process.Start(info);
if (OperatingSystem.IsWindows() && _spaProcess != null)
{
var stopScript = $@"do{{
try
{{
$processId = Get-Process -PID {Environment.ProcessId} -ErrorAction Stop;
}}catch
{{
$processId = $null;
}}
Start-Sleep -Seconds 1;
}}while($processId -ne $null);

try
{{
taskkill /T /F /PID {_spaProcess.Id};
}}
catch
{{
}}";
var stopScriptInfo = new ProcessStartInfo(
"powershell.exe",
string.Join(" ", "-NoProfile", "-C", stopScript))
{
CreateNoWindow = true,
WorkingDirectory = Path.Combine(AppContext.BaseDirectory, _options.WorkingDirectory)
};

var stopProcess = Process.Start(stopScriptInfo);
if (stopProcess == null || stopProcess.HasExited)
{
_logger.LogWarning($"SPA process shutdown script '{stopProcess?.Id}' failed to start. The SPA proxy might" +
$" remain open if the dotnet process is terminated abruptly. Use the operating system command to kill" +
$"the process tree for {_spaProcess.Id}");
}
else
{
_logger.LogDebug($"Watch process '{stopProcess}' started.");
}
}
}
catch (Exception exception)
{
Expand All @@ -199,7 +237,7 @@ private void LaunchDevelopmentProxy()

public Task StopAsync(CancellationToken cancellationToken)
{
// We don't need to do anything here since Dispose will take care of cleaning up the process if necessary.
Dispose(true);
return Task.CompletedTask;
}

Expand Down