From d7d1a04751b545c3c55352d428fdf1f48e6be035 Mon Sep 17 00:00:00 2001 From: SteveSandersonMS Date: Fri, 20 Jan 2017 17:32:26 +0000 Subject: [PATCH] When Node is launched with a debug listener, disable connection draining on restart. Fixes #506. --- .../HostingModels/NodeInvocationException.cs | 19 ++++++++++++++++++- .../HostingModels/OutOfProcessNodeInstance.cs | 13 +++++++++++-- .../NodeServicesImpl.cs | 3 ++- 3 files changed, 31 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.AspNetCore.NodeServices/HostingModels/NodeInvocationException.cs b/src/Microsoft.AspNetCore.NodeServices/HostingModels/NodeInvocationException.cs index c793ae0..dea3c2c 100644 --- a/src/Microsoft.AspNetCore.NodeServices/HostingModels/NodeInvocationException.cs +++ b/src/Microsoft.AspNetCore.NodeServices/HostingModels/NodeInvocationException.cs @@ -13,6 +13,13 @@ namespace Microsoft.AspNetCore.NodeServices.HostingModels /// public bool NodeInstanceUnavailable { get; private set; } + /// + /// If true, indicates that even though the invocation failed because the Node.js instance could not be reached + /// or needs to be restarted, that Node.js instance may remain alive for a period in order to complete any + /// outstanding requests. + /// + public bool AllowConnectionDraining { get; private set;} + /// /// Creates a new instance of . /// @@ -29,10 +36,20 @@ namespace Microsoft.AspNetCore.NodeServices.HostingModels /// A description of the exception. /// Additional information, such as a Node.js stack trace, representing the exception. /// Specifies a value for the flag. - public NodeInvocationException(string message, string details, bool nodeInstanceUnavailable) + /// Specifies a value for the flag. + public NodeInvocationException(string message, string details, bool nodeInstanceUnavailable, bool allowConnectionDraining) : this(message, details) { + // Reject a meaningless combination of flags + if (allowConnectionDraining && !nodeInstanceUnavailable) + { + throw new ArgumentException( + $"The '${ nameof(allowConnectionDraining) }' parameter cannot be true " + + $"unless the '${ nameof(nodeInstanceUnavailable) }' parameter is also true."); + } + NodeInstanceUnavailable = nodeInstanceUnavailable; + AllowConnectionDraining = allowConnectionDraining; } } } \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.NodeServices/HostingModels/OutOfProcessNodeInstance.cs b/src/Microsoft.AspNetCore.NodeServices/HostingModels/OutOfProcessNodeInstance.cs index 5c20738..f58374a 100644 --- a/src/Microsoft.AspNetCore.NodeServices/HostingModels/OutOfProcessNodeInstance.cs +++ b/src/Microsoft.AspNetCore.NodeServices/HostingModels/OutOfProcessNodeInstance.cs @@ -42,6 +42,7 @@ If you haven't yet installed node-inspector, you can do so as follows: private readonly StringAsTempFile _entryPointScript; private FileSystemWatcher _fileSystemWatcher; private int _invocationTimeoutMilliseconds; + private bool _launchWithDebugging; private readonly Process _nodeProcess; private int? _nodeDebuggingPort; private bool _nodeProcessNeedsRestart; @@ -78,9 +79,10 @@ If you haven't yet installed node-inspector, you can do so as follows: OutputLogger = nodeOutputLogger; _entryPointScript = new StringAsTempFile(entryPointScript); _invocationTimeoutMilliseconds = invocationTimeoutMilliseconds; + _launchWithDebugging = launchWithDebugging; var startInfo = PrepareNodeProcessStartInfo(_entryPointScript.FileName, projectPath, commandLineArguments, - environmentVars, launchWithDebugging, debuggingPort); + environmentVars, _launchWithDebugging, debuggingPort); _nodeProcess = LaunchNodeProcess(startInfo); _watchFileExtensions = watchFileExtensions; _fileSystemWatcher = BeginFileWatcher(projectPath); @@ -103,10 +105,17 @@ If you haven't yet installed node-inspector, you can do so as follows: { // This special kind of exception triggers a transparent retry - NodeServicesImpl will launch // a new Node instance and pass the invocation to that one instead. + // Note that if the Node process is listening for debugger connections, then we need it to shut + // down immediately and not stay open for connection draining (because if it did, the new Node + // instance wouldn't able to start, because the old one would still hold the debugging port). var message = _nodeProcess.HasExited ? "The Node process has exited" : "The Node process needs to restart"; - throw new NodeInvocationException(message, null, nodeInstanceUnavailable: true); + throw new NodeInvocationException( + message, + details: null, + nodeInstanceUnavailable: true, + allowConnectionDraining: !_launchWithDebugging); } // Construct a new cancellation token that combines the supplied token with the configured invocation diff --git a/src/Microsoft.AspNetCore.NodeServices/NodeServicesImpl.cs b/src/Microsoft.AspNetCore.NodeServices/NodeServicesImpl.cs index cc68448..8dd2044 100644 --- a/src/Microsoft.AspNetCore.NodeServices/NodeServicesImpl.cs +++ b/src/Microsoft.AspNetCore.NodeServices/NodeServicesImpl.cs @@ -72,7 +72,8 @@ namespace Microsoft.AspNetCore.NodeServices { if (_currentNodeInstance == nodeInstance) { - DisposeNodeInstance(_currentNodeInstance, delay: ConnectionDrainingTimespan); + var disposalDelay = ex.AllowConnectionDraining ? ConnectionDrainingTimespan : TimeSpan.Zero; + DisposeNodeInstance(_currentNodeInstance, disposalDelay); _currentNodeInstance = null; } }