Move logic for restarting Node child process into NodeServicesImpl. Tidy up lots.

Author: SteveSandersonMS

src/Microsoft.AspNetCore.NodeServices/Configuration/Configuration.cs

@@ -48,7 +48,8 @@ private static INodeInstance CreateNodeInstance(NodeServicesOptions options)
                     case NodeHostingModel.Http:
                         return new HttpNodeInstance(options.ProjectPath, /* port */ 0, options.WatchFileExtensions);
                     case NodeHostingModel.Socket:
-                        return new SocketNodeInstance(options.ProjectPath, options.WatchFileExtensions);
+                        var pipeName = "pni-" + Guid.NewGuid().ToString("D"); // Arbitrary non-clashing string
+                        return new SocketNodeInstance(options.ProjectPath, options.WatchFileExtensions, pipeName);
                     default:
                         throw new ArgumentException("Unknown hosting model: " + options.HostingModel);
                 }

src/Microsoft.AspNetCore.NodeServices/Content/Node/entrypoint-socket.js

@@ -176,7 +176,7 @@
 	// Begin listening now. The underlying transport varies according to the runtime platform.
 	// On Windows it's Named Pipes; on Linux/OSX it's Domain Sockets.
 	var useWindowsNamedPipes = /^win/.test(process.platform);
-	var listenAddress = (useWindowsNamedPipes ? '\\\\.\\pipe\\' : '/tmp/') + parsedArgs.pipename;
+	var listenAddress = (useWindowsNamedPipes ? '\\\\.\\pipe\\' : '/tmp/') + parsedArgs.listenAddress;
 	server.listen(listenAddress);
 
 

src/Microsoft.AspNetCore.NodeServices/HostingModels/HttpNodeInstance.cs

@@ -9,6 +9,18 @@
 
 namespace Microsoft.AspNetCore.NodeServices.HostingModels
 {
+    /// <summary>
+    /// A specialisation of the OutOfProcessNodeInstance base class that uses HTTP to perform RPC invocations.
+    ///
+    /// The Node child process starts an HTTP listener on an arbitrary available port (except where a nonzero
+    /// port number is specified as a constructor parameter), and signals which port was selected using the same
+    /// input/output-based mechanism that the base class uses to determine when the child process is ready to
+    /// accept RPC invocations.
+    ///
+    /// TODO: Remove the file-watching logic from here and centralise it in OutOfProcessNodeInstance, implementing
+    /// the actual watching in .NET code (not Node), for consistency across platforms.
+    /// </summary>
+    /// <seealso cref="Microsoft.AspNetCore.NodeServices.HostingModels.OutOfProcessNodeInstance" />
     internal class HttpNodeInstance : OutOfProcessNodeInstance
     {
         private static readonly Regex PortMessageRegex =
@@ -19,7 +31,7 @@ internal class HttpNodeInstance : OutOfProcessNodeInstance
             ContractResolver = new CamelCasePropertyNamesContractResolver()
         };
 
-	    private HttpClient _client;
+	    private readonly HttpClient _client;
         private bool _disposed;
         private int _portNumber;
 
@@ -47,9 +59,6 @@ private static string MakeCommandLineOptions(int port, string[] watchFileExtensi
 
         protected override async Task<T> InvokeExportAsync<T>(NodeInvocationInfo invocationInfo)
         {
-            await EnsureReady();
-
-            // TODO: Use System.Net.Http.Formatting (PostAsJsonAsync etc.)
             var payloadJson = JsonConvert.SerializeObject(invocationInfo, JsonSerializerSettings);
             var payload = new StringContent(payloadJson, Encoding.UTF8, "application/json");
             var response = await _client.PostAsync("http://localhost:" + _portNumber, payload);
@@ -97,6 +106,9 @@ protected override async Task<T> InvokeExportAsync<T>(NodeInvocationInfo invocat
 
         protected override void OnOutputDataReceived(string outputData)
         {
+            // Watch for "port selected" messages, and when observed, store the port number
+            // so we can use it when making HTTP requests. The child process will always send
+            // one of these messages before it sends a "ready for connections" message.
             var match = _portNumber != 0 ? null : PortMessageRegex.Match(outputData);
             if (match != null && match.Success)
             {
@@ -108,12 +120,6 @@ protected override void OnOutputDataReceived(string outputData)
             }
         }
 
-        protected override void OnBeforeLaunchProcess()
-        {
-            // Prepare to receive a new port number
-            _portNumber = 0;
-        }
-
 	    protected override void Dispose(bool disposing) {
 	        base.Dispose(disposing);
 

src/Microsoft.AspNetCore.NodeServices/HostingModels/NodeInvocationException.cs

@@ -4,9 +4,17 @@ namespace Microsoft.AspNetCore.NodeServices.HostingModels
 {
     public class NodeInvocationException : Exception
     {
+        public bool NodeInstanceUnavailable { get; private set; }
+
         public NodeInvocationException(string message, string details)
             : base(message + Environment.NewLine + details)
         {
         }
+
+        public NodeInvocationException(string message, string details, bool nodeInstanceUnavailable)
+            : this(message, details)
+        {
+            NodeInstanceUnavailable = nodeInstanceUnavailable;
+        }
     }
 }

src/Microsoft.AspNetCore.NodeServices/HostingModels/OutOfProcessNodeInstance.cs

@@ -6,37 +6,43 @@
 namespace Microsoft.AspNetCore.NodeServices.HostingModels
 {
     /// <summary>
-    /// Class responsible for launching the Node child process, determining when it is ready to accept invocations,
-    /// and finally killing it when the parent process exits. Also it restarts the child process if it dies.
+    /// Class responsible for launching a Node child process on the local machine, determining when it is ready to
+    /// accept invocations, detecting if it dies on its own, and finally terminating it on disposal.
+    ///
+    /// This abstract base class uses the input/output streams of the child process to perform a simple handshake
+    /// to determine when the child process is ready to accept invocations. This is agnostic to the mechanism that
+    /// derived classes use to actually perform the invocations (e.g., they could use HTTP-RPC, or a binary TCP
+    /// protocol, or any other RPC-type mechanism).
     /// </summary>
-    /// <seealso cref="Microsoft.AspNetCore.NodeServices.INodeInstance" />
+    /// <seealso cref="Microsoft.AspNetCore.NodeServices.HostingModels.INodeInstance" />
     public abstract class OutOfProcessNodeInstance : INodeInstance
     {
-        private readonly object _childProcessLauncherLock;
-        private string _commandLineArguments;
-        private readonly StringAsTempFile _entryPointScript;
-        private Process _nodeProcess;
-        private TaskCompletionSource<bool> _nodeProcessIsReadySource;
-        private readonly string _projectPath;
+        private const string ConnectionEstablishedMessage = "[Microsoft.AspNetCore.NodeServices:Listening]";
+        private readonly TaskCompletionSource<object> _connectionIsReadySource = new TaskCompletionSource<object>();
         private bool _disposed;
+        private readonly StringAsTempFile _entryPointScript;
+        private readonly Process _nodeProcess;
 
         public OutOfProcessNodeInstance(string entryPointScript, string projectPath, string commandLineArguments = null)
         {
-            _childProcessLauncherLock = new object();
             _entryPointScript = new StringAsTempFile(entryPointScript);
-            _projectPath = projectPath;
-            _commandLineArguments = commandLineArguments ?? string.Empty;
+            _nodeProcess = LaunchNodeProcess(_entryPointScript.FileName, projectPath, commandLineArguments);
+            ConnectToInputOutputStreams();
         }
 
-        public string CommandLineArguments
+        public async Task<T> InvokeExportAsync<T>(string moduleName, string exportNameOrNull, params object[] args)
         {
-            get { return _commandLineArguments; }
-            set { _commandLineArguments = value; }
-        }
+            // Wait until the connection is established. This will throw if the connection fails to initialize.
+            await _connectionIsReadySource.Task;
 
-        public Task<T> InvokeExportAsync<T>(string moduleName, string exportNameOrNull, params object[] args)
-        {
-            return InvokeExportAsync<T>(new NodeInvocationInfo
+            if (_nodeProcess.HasExited)
+            {
+                // This special kind of exception triggers a transparent retry - NodeServicesImpl will launch
+                // a new Node instance and pass the invocation to that one instead.
+                throw new NodeInvocationException("The Node process has exited", null, nodeInstanceUnavailable: true);
+            }
+
+            return await InvokeExportAsync<T>(new NodeInvocationInfo
             {
                 ModuleName = moduleName,
                 ExportedFunctionName = exportNameOrNull,
@@ -52,71 +58,74 @@ public void Dispose()
 
         protected abstract Task<T> InvokeExportAsync<T>(NodeInvocationInfo invocationInfo);
 
-        protected void ExitNodeProcess()
+        protected virtual void OnOutputDataReceived(string outputData)
+        {
+            Console.WriteLine("[Node] " + outputData);
+        }
+
+        protected virtual void OnErrorDataReceived(string errorData)
+        {
+            Console.WriteLine("[Node] " + errorData);
+        }
+
+        protected virtual void Dispose(bool disposing)
         {
-            if (_nodeProcess != null && !_nodeProcess.HasExited)
+            if (!_disposed)
             {
+                if (disposing)
+                {
+                    _entryPointScript.Dispose();
+                }
+
+                // Make sure the Node process is finished
                 // TODO: Is there a more graceful way to end it? Or does this still let it perform any cleanup?
-                _nodeProcess.Kill();
+                if (!_nodeProcess.HasExited)
+                {
+                    _nodeProcess.Kill();
+                }
+
+                _disposed = true;
             }
         }
 
-        protected async Task EnsureReady()
+        private static Process LaunchNodeProcess(string entryPointFilename, string projectPath, string commandLineArguments)
         {
-            lock (_childProcessLauncherLock)
+            var startInfo = new ProcessStartInfo("node")
             {
-                if (_nodeProcess == null || _nodeProcess.HasExited)
-                {
-                    this.OnBeforeLaunchProcess();
+                Arguments = "\"" + entryPointFilename + "\" " + (commandLineArguments ?? string.Empty),
+                UseShellExecute = false,
+                RedirectStandardInput = true,
+                RedirectStandardOutput = true,
+                RedirectStandardError = true,
+                WorkingDirectory = projectPath
+            };
 
-                    var startInfo = new ProcessStartInfo("node")
-                    {
-                        Arguments = "\"" + _entryPointScript.FileName + "\" " + _commandLineArguments,
-                        UseShellExecute = false,
-                        RedirectStandardInput = true,
-                        RedirectStandardOutput = true,
-                        RedirectStandardError = true,
-                        WorkingDirectory = _projectPath
-                    };
-
-                    // Append projectPath to NODE_PATH so it can locate node_modules
-                    var existingNodePath = Environment.GetEnvironmentVariable("NODE_PATH") ?? string.Empty;
-                    if (existingNodePath != string.Empty)
-                    {
-                        existingNodePath += ":";
-                    }
+            // Append projectPath to NODE_PATH so it can locate node_modules
+            var existingNodePath = Environment.GetEnvironmentVariable("NODE_PATH") ?? string.Empty;
+            if (existingNodePath != string.Empty)
+            {
+                existingNodePath += ":";
+            }
 
-                    var nodePathValue = existingNodePath + Path.Combine(_projectPath, "node_modules");
+            var nodePathValue = existingNodePath + Path.Combine(projectPath, "node_modules");
 #if NET451
-                    startInfo.EnvironmentVariables["NODE_PATH"] = nodePathValue;
+            startInfo.EnvironmentVariables["NODE_PATH"] = nodePathValue;
 #else
-                    startInfo.Environment["NODE_PATH"] = nodePathValue;
+            startInfo.Environment["NODE_PATH"] = nodePathValue;
 #endif
 
-                    _nodeProcess = Process.Start(startInfo);
-                    ConnectToInputOutputStreams();
-                }
-            }
-
-            var task = _nodeProcessIsReadySource.Task;
-            var initializationSucceeded = await task;
-
-            if (!initializationSucceeded)
-            {
-                throw new InvalidOperationException("The Node.js process failed to initialize", task.Exception);
-            }
+            return Process.Start(startInfo);
         }
 
         private void ConnectToInputOutputStreams()
         {
-            var initializationIsCompleted = false; // TODO: Make this thread-safe? (Interlocked.Exchange etc.)
-            _nodeProcessIsReadySource = new TaskCompletionSource<bool>();
+            var initializationIsCompleted = false;
 
             _nodeProcess.OutputDataReceived += (sender, evt) =>
             {
-                if (evt.Data == "[Microsoft.AspNetCore.NodeServices:Listening]" && !initializationIsCompleted)
+                if (evt.Data == ConnectionEstablishedMessage && !initializationIsCompleted)
                 {
-                    _nodeProcessIsReadySource.SetResult(true);
+                    _connectionIsReadySource.SetResult(null);
                     initializationIsCompleted = true;
                 }
                 else if (evt.Data != null)
@@ -129,48 +138,23 @@ private void ConnectToInputOutputStreams()
             {
                 if (evt.Data != null)
                 {
-                    OnErrorDataReceived(evt.Data);
                     if (!initializationIsCompleted)
                     {
-                        _nodeProcessIsReadySource.SetResult(false);
+                        _connectionIsReadySource.SetException(
+                            new InvalidOperationException("The Node.js process failed to initialize: " + evt.Data));
                         initializationIsCompleted = true;
                     }
+                    else
+                    {
+                        OnErrorDataReceived(evt.Data);
+                    }
                 }
             };
 
             _nodeProcess.BeginOutputReadLine();
             _nodeProcess.BeginErrorReadLine();
         }
 
-        protected virtual void OnBeforeLaunchProcess()
-        {
-        }
-
-        protected virtual void OnOutputDataReceived(string outputData)
-        {
-            Console.WriteLine("[Node] " + outputData);
-        }
-
-        protected virtual void OnErrorDataReceived(string errorData)
-        {
-            Console.WriteLine("[Node] " + errorData);
-        }
-
-        protected virtual void Dispose(bool disposing)
-        {
-            if (!_disposed)
-            {
-                if (disposing)
-                {
-                    _entryPointScript.Dispose();
-                }
-
-                ExitNodeProcess();
-
-                _disposed = true;
-            }
-        }
-
         ~OutOfProcessNodeInstance()
         {
             Dispose(false);