diff --git a/src/Microsoft.AspNetCore.NodeServices/Configuration/NodeServicesFactory.cs b/src/Microsoft.AspNetCore.NodeServices/Configuration/NodeServicesFactory.cs index fc038f2..d6660c0 100644 --- a/src/Microsoft.AspNetCore.NodeServices/Configuration/NodeServicesFactory.cs +++ b/src/Microsoft.AspNetCore.NodeServices/Configuration/NodeServicesFactory.cs @@ -29,11 +29,11 @@ namespace Microsoft.AspNetCore.NodeServices { case NodeHostingModel.Http: return new HttpNodeInstance(options.ProjectPath, options.WatchFileExtensions, options.NodeInstanceOutputLogger, - options.EnvironmentVariables, options.LaunchWithDebugging, options.DebuggingPort, /* port */ 0); + options.EnvironmentVariables, options.InvocationTimeoutMilliseconds, options.LaunchWithDebugging, options.DebuggingPort, /* port */ 0); case NodeHostingModel.Socket: var pipeName = "pni-" + Guid.NewGuid().ToString("D"); // Arbitrary non-clashing string return new SocketNodeInstance(options.ProjectPath, options.WatchFileExtensions, pipeName, options.NodeInstanceOutputLogger, - options.EnvironmentVariables, options.LaunchWithDebugging, options.DebuggingPort); + options.EnvironmentVariables, options.InvocationTimeoutMilliseconds, options.LaunchWithDebugging, options.DebuggingPort); default: throw new ArgumentException("Unknown hosting model: " + options.HostingModel); } diff --git a/src/Microsoft.AspNetCore.NodeServices/Configuration/NodeServicesOptions.cs b/src/Microsoft.AspNetCore.NodeServices/Configuration/NodeServicesOptions.cs index edf9ae2..dbf1ce6 100644 --- a/src/Microsoft.AspNetCore.NodeServices/Configuration/NodeServicesOptions.cs +++ b/src/Microsoft.AspNetCore.NodeServices/Configuration/NodeServicesOptions.cs @@ -11,6 +11,8 @@ namespace Microsoft.AspNetCore.NodeServices public class NodeServicesOptions { public const NodeHostingModel DefaultNodeHostingModel = NodeHostingModel.Http; + internal const string TimeoutConfigPropertyName = nameof(InvocationTimeoutMilliseconds); + private const int DefaultInvocationTimeoutMilliseconds = 60 * 1000; private const string LogCategoryName = "Microsoft.AspNetCore.NodeServices"; private static readonly string[] DefaultWatchFileExtensions = { ".js", ".jsx", ".ts", ".tsx", ".json", ".html" }; @@ -22,6 +24,7 @@ namespace Microsoft.AspNetCore.NodeServices } EnvironmentVariables = new Dictionary(); + InvocationTimeoutMilliseconds = DefaultInvocationTimeoutMilliseconds; HostingModel = DefaultNodeHostingModel; WatchFileExtensions = (string[])DefaultWatchFileExtensions.Clone(); @@ -49,5 +52,6 @@ namespace Microsoft.AspNetCore.NodeServices public bool LaunchWithDebugging { get; set; } public IDictionary EnvironmentVariables { get; set; } public int DebuggingPort { get; set; } + public int InvocationTimeoutMilliseconds { get; set; } } } \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.NodeServices/HostingModels/HttpNodeInstance.cs b/src/Microsoft.AspNetCore.NodeServices/HostingModels/HttpNodeInstance.cs index 0c11371..1ab4c13 100644 --- a/src/Microsoft.AspNetCore.NodeServices/HostingModels/HttpNodeInstance.cs +++ b/src/Microsoft.AspNetCore.NodeServices/HostingModels/HttpNodeInstance.cs @@ -37,7 +37,8 @@ namespace Microsoft.AspNetCore.NodeServices.HostingModels private int _portNumber; public HttpNodeInstance(string projectPath, string[] watchFileExtensions, ILogger nodeInstanceOutputLogger, - IDictionary environmentVars, bool launchWithDebugging, int debuggingPort, int port = 0) + IDictionary environmentVars, int invocationTimeoutMilliseconds, bool launchWithDebugging, + int debuggingPort, int port = 0) : base( EmbeddedResourceReader.Read( typeof(HttpNodeInstance), @@ -47,6 +48,7 @@ namespace Microsoft.AspNetCore.NodeServices.HostingModels MakeCommandLineOptions(port), nodeInstanceOutputLogger, environmentVars, + invocationTimeoutMilliseconds, launchWithDebugging, debuggingPort) { diff --git a/src/Microsoft.AspNetCore.NodeServices/HostingModels/OutOfProcessNodeInstance.cs b/src/Microsoft.AspNetCore.NodeServices/HostingModels/OutOfProcessNodeInstance.cs index 9a583d3..3957cb0 100644 --- a/src/Microsoft.AspNetCore.NodeServices/HostingModels/OutOfProcessNodeInstance.cs +++ b/src/Microsoft.AspNetCore.NodeServices/HostingModels/OutOfProcessNodeInstance.cs @@ -37,6 +37,7 @@ If you haven't yet installed node-inspector, you can do so as follows: private bool _disposed; private readonly StringAsTempFile _entryPointScript; private FileSystemWatcher _fileSystemWatcher; + private int _invocationTimeoutMilliseconds; private readonly Process _nodeProcess; private int? _nodeDebuggingPort; private bool _nodeProcessNeedsRestart; @@ -49,6 +50,7 @@ If you haven't yet installed node-inspector, you can do so as follows: string commandLineArguments, ILogger nodeOutputLogger, IDictionary environmentVars, + int invocationTimeoutMilliseconds, bool launchWithDebugging, int debuggingPort) { @@ -59,6 +61,7 @@ If you haven't yet installed node-inspector, you can do so as follows: OutputLogger = nodeOutputLogger; _entryPointScript = new StringAsTempFile(entryPointScript); + _invocationTimeoutMilliseconds = invocationTimeoutMilliseconds; var startInfo = PrepareNodeProcessStartInfo(_entryPointScript.FileName, projectPath, commandLineArguments, environmentVars, launchWithDebugging, debuggingPort); @@ -81,17 +84,74 @@ If you haven't yet installed node-inspector, you can do so as follows: throw new NodeInvocationException(message, null, nodeInstanceUnavailable: true); } - // Wait until the connection is established. This will throw if the connection fails to initialize, - // or if cancellation is requested first. Note that we can't really cancel the "establishing connection" - // task because that's shared with all callers, but we can stop waiting for it if this call is cancelled. - await _connectionIsReadySource.Task.OrThrowOnCancellation(cancellationToken); - - return await InvokeExportAsync(new NodeInvocationInfo + // Construct a new cancellation token that combines the supplied token with the configured invocation + // timeout. Technically we could avoid wrapping the cancellationToken if no timeout is configured, + // but that's not really a major use case, since timeouts are enabled by default. + using (var timeoutSource = new CancellationTokenSource()) + using (var combinedCancellationTokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, timeoutSource.Token)) { - ModuleName = moduleName, - ExportedFunctionName = exportNameOrNull, - Args = args - }, cancellationToken); + if (_invocationTimeoutMilliseconds > 0) + { + timeoutSource.CancelAfter(_invocationTimeoutMilliseconds); + } + + // By overwriting the supplied cancellation token, we ensure that it isn't accidentally used + // below. We only want to pass through the token that respects timeouts. + cancellationToken = combinedCancellationTokenSource.Token; + var connectionDidSucceed = false; + + try + { + // Wait until the connection is established. This will throw if the connection fails to initialize, + // or if cancellation is requested first. Note that we can't really cancel the "establishing connection" + // task because that's shared with all callers, but we can stop waiting for it if this call is cancelled. + await _connectionIsReadySource.Task.OrThrowOnCancellation(cancellationToken); + connectionDidSucceed = true; + + return await InvokeExportAsync(new NodeInvocationInfo + { + ModuleName = moduleName, + ExportedFunctionName = exportNameOrNull, + Args = args + }, cancellationToken); + } + catch (TaskCanceledException) + { + if (timeoutSource.IsCancellationRequested) + { + // It was very common for developers to report 'TaskCanceledException' when encountering almost any + // trouble when using NodeServices. Now we have a default invocation timeout, and attempt to give + // a more descriptive exception message if it happens. + if (!connectionDidSucceed) + { + // This is very unlikely, but for debugging, it's still useful to differentiate it from the + // case below. + throw new NodeInvocationException( + $"Attempt to connect to Node timed out after {_invocationTimeoutMilliseconds}ms.", + string.Empty); + } + else + { + // Developers encounter this fairly often (if their Node code fails without invoking the callback, + // all that the .NET side knows is that the invocation eventually times out). Previously, this surfaced + // as a TaskCanceledException, but this led to a lot of issue reports. Now we throw the following + // descriptive error. + throw new NodeInvocationException( + $"The Node invocation timed out after {_invocationTimeoutMilliseconds}ms.", + $"You can change the timeout duration by setting the {NodeServicesOptions.TimeoutConfigPropertyName} " + + $"property on {nameof(NodeServicesOptions)}.\n\n" + + "The first debugging step is to ensure that your Node.js function always invokes the supplied " + + "callback (or throws an exception synchronously), even if it encounters an error. Otherwise, " + + "the .NET code has no way to know that it is finished or has failed." + ); + } + } + else + { + throw; + } + } + } } public void Dispose() diff --git a/src/Microsoft.AspNetCore.NodeServices/HostingModels/SocketNodeInstance.cs b/src/Microsoft.AspNetCore.NodeServices/HostingModels/SocketNodeInstance.cs index 2acfc0e..2ee2aa4 100644 --- a/src/Microsoft.AspNetCore.NodeServices/HostingModels/SocketNodeInstance.cs +++ b/src/Microsoft.AspNetCore.NodeServices/HostingModels/SocketNodeInstance.cs @@ -41,7 +41,7 @@ namespace Microsoft.AspNetCore.NodeServices.HostingModels public SocketNodeInstance(string projectPath, string[] watchFileExtensions, string socketAddress, ILogger nodeInstanceOutputLogger, IDictionary environmentVars, - bool launchWithDebugging, int debuggingPort) + int invocationTimeoutMilliseconds, bool launchWithDebugging, int debuggingPort) : base( EmbeddedResourceReader.Read( typeof(SocketNodeInstance), @@ -51,6 +51,7 @@ namespace Microsoft.AspNetCore.NodeServices.HostingModels MakeNewCommandLineOptions(socketAddress), nodeInstanceOutputLogger, environmentVars, + invocationTimeoutMilliseconds, launchWithDebugging, debuggingPort) {