Cleanup processes started by UseReactDevelopmentServer and UseAngularCliServer

src/Middleware/SpaServices.Extensions/src/AngularCli/AngularCliBuilder.cs

@@ -1,15 +1,18 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System;
+using System.Diagnostics;
+using System.IO;
+using System.Text.RegularExpressions;
+using System.Threading.Tasks;
 using Microsoft.AspNetCore.Builder;
 using Microsoft.AspNetCore.NodeServices.Npm;
 using Microsoft.AspNetCore.NodeServices.Util;
 using Microsoft.AspNetCore.SpaServices.Prerendering;
 using Microsoft.AspNetCore.SpaServices.Util;
-using System;
-using System.IO;
-using System.Text.RegularExpressions;
-using System.Threading.Tasks;
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Hosting;
 
 namespace Microsoft.AspNetCore.SpaServices.AngularCli
 {
@@ -48,15 +51,20 @@ public async Task Build(ISpaBuilder spaBuilder)
                 throw new InvalidOperationException($"To use {nameof(AngularCliBuilder)}, you must supply a non-empty value for the {nameof(SpaOptions.SourcePath)} property of {nameof(SpaOptions)} when calling {nameof(SpaApplicationBuilderExtensions.UseSpa)}.");
             }
 
+            var appBuilder = spaBuilder.ApplicationBuilder;
+            var applicationStoppingToken = appBuilder.ApplicationServices.GetRequiredService<IHostApplicationLifetime>().ApplicationStopping;
             var logger = LoggerFinder.GetOrCreateLogger(
-                spaBuilder.ApplicationBuilder,
+                appBuilder,
                 nameof(AngularCliBuilder));
+            var diagnosticSource = appBuilder.ApplicationServices.GetRequiredService<DiagnosticSource>();
             var scriptRunner = new NodeScriptRunner(
                 sourcePath,
                 _scriptName,
                 "--watch",
                 null,
-                pkgManagerCommand);
+                pkgManagerCommand,
+                diagnosticSource,
+                applicationStoppingToken);
             scriptRunner.AttachToLogger(logger);
 
             using (var stdOutReader = new EventedStreamStringReader(scriptRunner.StdOut))

src/Middleware/SpaServices.Extensions/src/AngularCli/AngularCliMiddleware.cs

@@ -1,18 +1,21 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
-using Microsoft.AspNetCore.Builder;
-using Microsoft.Extensions.Logging;
-using Microsoft.AspNetCore.NodeServices.Npm;
-using Microsoft.AspNetCore.NodeServices.Util;
-using Microsoft.AspNetCore.SpaServices.Util;
 using System;
+using System.Diagnostics;
 using System.IO;
+using System.Net.Http;
 using System.Text.RegularExpressions;
-using System.Threading.Tasks;
 using System.Threading;
-using System.Net.Http;
+using System.Threading.Tasks;
+using Microsoft.AspNetCore.Builder;
+using Microsoft.AspNetCore.NodeServices.Npm;
+using Microsoft.AspNetCore.NodeServices.Util;
 using Microsoft.AspNetCore.SpaServices.Extensions.Util;
+using Microsoft.AspNetCore.SpaServices.Util;
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Hosting;
+using Microsoft.Extensions.Logging;
 
 namespace Microsoft.AspNetCore.SpaServices.AngularCli
 {
@@ -40,8 +43,10 @@ public static void Attach(
 
             // Start Angular CLI and attach to middleware pipeline
             var appBuilder = spaBuilder.ApplicationBuilder;
+            var applicationStoppingToken = appBuilder.ApplicationServices.GetRequiredService<IHostApplicationLifetime>().ApplicationStopping;
             var logger = LoggerFinder.GetOrCreateLogger(appBuilder, LogCategoryName);
-            var angularCliServerInfoTask = StartAngularCliServerAsync(sourcePath, scriptName, pkgManagerCommand, devServerPort, logger);
+            var diagnosticSource = appBuilder.ApplicationServices.GetRequiredService<DiagnosticSource>();
+            var angularCliServerInfoTask = StartAngularCliServerAsync(sourcePath, scriptName, pkgManagerCommand, devServerPort, logger, diagnosticSource, applicationStoppingToken);
 
             // Everything we proxy is hardcoded to target http://localhost because:
             // - the requests are always from the local machine (we're not accepting remote
@@ -64,7 +69,7 @@ public static void Attach(
         }
 
         private static async Task<AngularCliServerInfo> StartAngularCliServerAsync(
-            string sourcePath, string scriptName, string pkgManagerCommand, int portNumber, ILogger logger)
+            string sourcePath, string scriptName, string pkgManagerCommand, int portNumber, ILogger logger, DiagnosticSource diagnosticSource, CancellationToken applicationStoppingToken)
         {
             if (portNumber == default(int))
             {
@@ -73,7 +78,7 @@ private static async Task<AngularCliServerInfo> StartAngularCliServerAsync(
             logger.LogInformation($"Starting @angular/cli on port {portNumber}...");
 
             var scriptRunner = new NodeScriptRunner(
-                sourcePath, scriptName, $"--port {portNumber}", null, pkgManagerCommand);
+                sourcePath, scriptName, $"--port {portNumber}", null, pkgManagerCommand, diagnosticSource, applicationStoppingToken);
             scriptRunner.AttachToLogger(logger);
 
             Match openBrowserLine;

src/Middleware/SpaServices.Extensions/src/Npm/NodeScriptRunner.cs

@@ -1,13 +1,14 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
-using Microsoft.Extensions.Logging;
-using Microsoft.AspNetCore.NodeServices.Util;
 using System;
+using System.Collections.Generic;
 using System.Diagnostics;
 using System.Runtime.InteropServices;
 using System.Text.RegularExpressions;
-using System.Collections.Generic;
+using System.Threading;
+using Microsoft.AspNetCore.NodeServices.Util;
+using Microsoft.Extensions.Logging;
 
 // This is under the NodeServices namespace because post 2.1 it will be moved to that package
 namespace Microsoft.AspNetCore.NodeServices.Npm
@@ -16,14 +17,15 @@ namespace Microsoft.AspNetCore.NodeServices.Npm
     /// Executes the <c>script</c> entries defined in a <c>package.json</c> file,
     /// capturing any output written to stdio.
     /// </summary>
-    internal class NodeScriptRunner
+    internal class NodeScriptRunner : IDisposable
     {
+        private Process _npmProcess;
         public EventedStreamReader StdOut { get; }
         public EventedStreamReader StdErr { get; }
 
         private static Regex AnsiColorRegex = new Regex("\x001b\\[[0-9;]*m", RegexOptions.None, TimeSpan.FromSeconds(1));
 
-        public NodeScriptRunner(string workingDirectory, string scriptName, string arguments, IDictionary<string, string> envVars, string pkgManagerCommand)
+        public NodeScriptRunner(string workingDirectory, string scriptName, string arguments, IDictionary<string, string> envVars, string pkgManagerCommand, DiagnosticSource diagnosticSource, CancellationToken applicationStoppingToken)
         {
             if (string.IsNullOrEmpty(workingDirectory))
             {
@@ -69,9 +71,22 @@ public NodeScriptRunner(string workingDirectory, string scriptName, string argum
                 }
             }
 
-            var process = LaunchNodeProcess(processStartInfo, pkgManagerCommand);
-            StdOut = new EventedStreamReader(process.StandardOutput);
-            StdErr = new EventedStreamReader(process.StandardError);
+            _npmProcess = LaunchNodeProcess(processStartInfo, pkgManagerCommand);
+            StdOut = new EventedStreamReader(_npmProcess.StandardOutput);
+            StdErr = new EventedStreamReader(_npmProcess.StandardError);
+
+            applicationStoppingToken.Register(((IDisposable)this).Dispose);
+
+            if (diagnosticSource.IsEnabled("Microsoft.AspNetCore.NodeServices.Npm.NpmStarted"))
+            {
+                diagnosticSource.Write(
+                    "Microsoft.AspNetCore.NodeServices.Npm.NpmStarted",
+                    new
+                    {
+                        processStartInfo = processStartInfo,
+                        process = _npmProcess
+                    });
+            }
         }
 
         public void AttachToLogger(ILogger logger)
@@ -132,5 +147,14 @@ private static Process LaunchNodeProcess(ProcessStartInfo startInfo, string comm
                 throw new InvalidOperationException(message, ex);
             }
         }
+
+        void IDisposable.Dispose()
+        {
+            if (_npmProcess != null && !_npmProcess.HasExited)
+            {
+                _npmProcess.Kill(entireProcessTree: true);
+                _npmProcess = null;
+            }
+        }
     }
 }

src/Middleware/SpaServices.Extensions/src/ReactDevelopmentServer/ReactDevelopmentServerMiddleware.cs

@@ -1,17 +1,21 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
-using Microsoft.AspNetCore.Builder;
-using Microsoft.Extensions.Logging;
-using Microsoft.AspNetCore.NodeServices.Npm;
-using Microsoft.AspNetCore.NodeServices.Util;
-using Microsoft.AspNetCore.SpaServices.Util;
 using System;
-using System.IO;
 using System.Collections.Generic;
+using System.Diagnostics;
+using System.IO;
 using System.Text.RegularExpressions;
+using System.Threading;
 using System.Threading.Tasks;
+using Microsoft.AspNetCore.Builder;
+using Microsoft.AspNetCore.NodeServices.Npm;
+using Microsoft.AspNetCore.NodeServices.Util;
 using Microsoft.AspNetCore.SpaServices.Extensions.Util;
+using Microsoft.AspNetCore.SpaServices.Util;
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Hosting;
+using Microsoft.Extensions.Logging;
 
 namespace Microsoft.AspNetCore.SpaServices.ReactDevelopmentServer
 {
@@ -39,8 +43,10 @@ public static void Attach(
 
             // Start create-react-app and attach to middleware pipeline
             var appBuilder = spaBuilder.ApplicationBuilder;
+            var applicationStoppingToken = appBuilder.ApplicationServices.GetRequiredService<IHostApplicationLifetime>().ApplicationStopping;
             var logger = LoggerFinder.GetOrCreateLogger(appBuilder, LogCategoryName);
-            var portTask = StartCreateReactAppServerAsync(sourcePath, scriptName, pkgManagerCommand, devServerPort, logger);
+            var diagnosticSource = appBuilder.ApplicationServices.GetRequiredService<DiagnosticSource>();
+            var portTask = StartCreateReactAppServerAsync(sourcePath, scriptName, pkgManagerCommand, devServerPort, logger, diagnosticSource, applicationStoppingToken);
 
             // Everything we proxy is hardcoded to target http://localhost because:
             // - the requests are always from the local machine (we're not accepting remote
@@ -63,7 +69,7 @@ public static void Attach(
         }
 
         private static async Task<int> StartCreateReactAppServerAsync(
-            string sourcePath, string scriptName, string pkgManagerCommand, int portNumber, ILogger logger)
+            string sourcePath, string scriptName, string pkgManagerCommand, int portNumber, ILogger logger, DiagnosticSource diagnosticSource, CancellationToken applicationStoppingToken)
         {
             if (portNumber == default(int))
             {
@@ -77,7 +83,7 @@ private static async Task<int> StartCreateReactAppServerAsync(
                 { "BROWSER", "none" }, // We don't want create-react-app to open its own extra browser window pointing to the internal dev server port
             };
             var scriptRunner = new NodeScriptRunner(
-                sourcePath, scriptName, null, envVars, pkgManagerCommand);
+                sourcePath, scriptName, null, envVars, pkgManagerCommand, diagnosticSource, applicationStoppingToken);
             scriptRunner.AttachToLogger(logger);
 
             using (var stdErrReader = new EventedStreamStringReader(scriptRunner.StdErr))

src/Middleware/SpaServices.Extensions/src/ReactDevelopmentServer/ReactDevelopmentServerMiddlewareExtensions.cs

@@ -1,8 +1,8 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
-using Microsoft.AspNetCore.Builder;
 using System;
+using Microsoft.AspNetCore.Builder;
 
 namespace Microsoft.AspNetCore.SpaServices.ReactDevelopmentServer
 {

src/Middleware/SpaServices.Extensions/test/ListLoggerFactory.cs

@@ -0,0 +1,101 @@
+using System;
+using System.Collections.Generic;
+using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Logging.Abstractions;
+using Xunit.Abstractions;
+
+namespace Microsoft.AspNetCore.SpaServices.Extensions.Tests
+{
+    public class ListLoggerFactory : ILoggerFactory
+    {
+        private readonly Func<string, bool> _shouldLogCategory;
+        private bool _disposed;
+
+        public ListLoggerFactory()
+            : this(_ => true)
+        {
+        }
+
+        public ListLoggerFactory(Func<string, bool> shouldLogCategory)
+        {
+            _shouldLogCategory = shouldLogCategory;
+            Logger = new ListLogger();
+        }
+
+        public List<(LogLevel Level, EventId Id, string Message, object State, Exception Exception)> Log => Logger.LoggedEvents;
+        protected ListLogger Logger { get; set; }
+
+        public virtual void Clear() => Logger.Clear();
+
+        public void SetTestOutputHelper(ITestOutputHelper testOutputHelper)
+        {
+            Logger.TestOutputHelper = testOutputHelper;
+        }
+
+        public virtual ILogger CreateLogger(string name)
+        {
+            CheckDisposed();
+
+            return !_shouldLogCategory(name)
+                ? (ILogger)NullLogger.Instance
+                : Logger;
+        }
+
+        private void CheckDisposed()
+        {
+            if (_disposed)
+            {
+                throw new ObjectDisposedException(nameof(ListLoggerFactory));
+            }
+        }
+
+        public void AddProvider(ILoggerProvider provider)
+        {
+            CheckDisposed();
+        }
+
+        public void Dispose()
+        {
+            _disposed = true;
+        }
+
+        protected class ListLogger : ILogger
+        {
+            private readonly object _sync = new object();
+
+            public ITestOutputHelper TestOutputHelper { get; set; }
+
+            public List<(LogLevel, EventId, string, object, Exception)> LoggedEvents { get; }
+                = new List<(LogLevel, EventId, string, object, Exception)>();
+
+            public void Clear()
+            {
+                lock (_sync) // Guard against tests with explicit concurrency
+                {
+                    LoggedEvents.Clear();
+                }
+            }
+
+            public void Log<TState>(
+                LogLevel logLevel, EventId eventId, TState state, Exception exception, Func<TState, Exception, string> formatter)
+            {
+                lock (_sync) // Guard against tests with explicit concurrency
+                {
+                    var message = formatter(state, exception)?.Trim();
+                    if (message != null)
+                    {
+                        TestOutputHelper?.WriteLine(message + Environment.NewLine);
+                    }
+
+                    LoggedEvents.Add((logLevel, eventId, message, state, exception));
+                }
+            }
+
+            public bool IsEnabled(LogLevel logLevel) => true;
+
+            public IDisposable BeginScope(object state) => null;
+
+            public IDisposable BeginScope<TState>(TState state) => null;
+        }
+    }
+}

src/Middleware/SpaServices.Extensions/test/Microsoft.AspNetCore.SpaServices.Extensions.Tests.csproj

@@ -1,17 +1,27 @@
-<Project Sdk="Microsoft.NET.Sdk">
+<Project Sdk="Microsoft.NET.Sdk">
 
   <PropertyGroup>
     <TargetFramework>$(DefaultNetCoreTargetFramework)</TargetFramework>
     <TestDependsOnNode>true</TestDependsOnNode>
+	<!-- Depends on npm which is not picked up on helix -->
+    <!-- https://github.com/dotnet/aspnetcore/issues/18672 -->
+	<BuildHelixPayload>false</BuildHelixPayload>
   </PropertyGroup>
 
   <ItemGroup>
     <Reference Include="Microsoft.AspNetCore.SpaServices.Extensions" />
     <Reference Include="Microsoft.AspNetCore.Hosting" />
     <Reference Include="Microsoft.AspNetCore.TestHost" />
+    <Reference Include="Microsoft.Extensions.DiagnosticAdapter" />
     <Reference Include="Microsoft.Extensions.Hosting" />
     <Reference Include="Microsoft.Extensions.Logging.Testing" />
     <Content Include="js\**\*" />
   </ItemGroup>
 
+  <ItemGroup>
+    <None Update="package.json">
+      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
+    </None>
+  </ItemGroup>
+
 </Project>