From e024e5bdcd2859abb4b9ae28a9c4472c96b14e31 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Tue, 2 Jan 2018 14:02:10 +0000 Subject: [PATCH] When a SPA dev server (or prerendering build) takes too long to start up, only fail current request, not future requests. Fixes #1447 --- .../AngularCli/AngularCliBuilder.cs | 4 +-- .../AngularCli/AngularCliMiddleware.cs | 21 +++++------ .../Prerendering/SpaPrerenderingExtensions.cs | 13 +++++-- .../Proxying/SpaProxyingExtensions.cs | 10 +++--- .../ReactDevelopmentServerMiddleware.cs | 21 +++++------ .../Util/EventedStreamReader.cs | 11 +----- .../Util/TaskTimeoutExtensions.cs | 35 +++++++++++++++++++ 7 files changed, 75 insertions(+), 40 deletions(-) create mode 100644 src/Microsoft.AspNetCore.SpaServices.Extensions/Util/TaskTimeoutExtensions.cs diff --git a/src/Microsoft.AspNetCore.SpaServices.Extensions/AngularCli/AngularCliBuilder.cs b/src/Microsoft.AspNetCore.SpaServices.Extensions/AngularCli/AngularCliBuilder.cs index 2d2b1b2..61dedd3 100644 --- a/src/Microsoft.AspNetCore.SpaServices.Extensions/AngularCli/AngularCliBuilder.cs +++ b/src/Microsoft.AspNetCore.SpaServices.Extensions/AngularCli/AngularCliBuilder.cs @@ -20,7 +20,6 @@ namespace Microsoft.AspNetCore.SpaServices.AngularCli public class AngularCliBuilder : ISpaPrerendererBuilder { private static TimeSpan RegexMatchTimeout = TimeSpan.FromSeconds(5); // This is a development-time only feature, so a very long timeout is fine - private static TimeSpan BuildTimeout = TimeSpan.FromSeconds(50); // Note that the HTTP request itself by default times out after 60s, so you only get useful error information if this is shorter private readonly string _npmScriptName; @@ -63,8 +62,7 @@ namespace Microsoft.AspNetCore.SpaServices.AngularCli try { await npmScriptRunner.StdOut.WaitForMatch( - new Regex("Date", RegexOptions.None, RegexMatchTimeout), - BuildTimeout); + new Regex("Date", RegexOptions.None, RegexMatchTimeout)); } catch (EndOfStreamException ex) { diff --git a/src/Microsoft.AspNetCore.SpaServices.Extensions/AngularCli/AngularCliMiddleware.cs b/src/Microsoft.AspNetCore.SpaServices.Extensions/AngularCli/AngularCliMiddleware.cs index e3f6925..c6aa54c 100644 --- a/src/Microsoft.AspNetCore.SpaServices.Extensions/AngularCli/AngularCliMiddleware.cs +++ b/src/Microsoft.AspNetCore.SpaServices.Extensions/AngularCli/AngularCliMiddleware.cs @@ -12,6 +12,7 @@ using System.Text.RegularExpressions; using System.Threading.Tasks; using System.Threading; using System.Net.Http; +using Microsoft.AspNetCore.SpaServices.Extensions.Util; namespace Microsoft.AspNetCore.SpaServices.AngularCli { @@ -49,7 +50,15 @@ namespace Microsoft.AspNetCore.SpaServices.AngularCli var targetUriTask = angularCliServerInfoTask.ContinueWith( task => new UriBuilder("http", "localhost", task.Result.Port).Uri); - SpaProxyingExtensions.UseProxyToSpaDevelopmentServer(spaBuilder, targetUriTask); + SpaProxyingExtensions.UseProxyToSpaDevelopmentServer(spaBuilder, () => + { + // On each request, we create a separate startup task with its own timeout. That way, even if + // the first request times out, subsequent requests could still work. + return targetUriTask.WithTimeout(StartupTimeout, + $"The Angular CLI process did not start listening for requests " + + $"within the timeout period of {StartupTimeout.Seconds} seconds. " + + $"Check the log output for error information."); + }); } private static async Task StartAngularCliServerAsync( @@ -68,8 +77,7 @@ namespace Microsoft.AspNetCore.SpaServices.AngularCli try { openBrowserLine = await npmScriptRunner.StdOut.WaitForMatch( - new Regex("open your browser on (http\\S+)", RegexOptions.None, RegexMatchTimeout), - StartupTimeout); + new Regex("open your browser on (http\\S+)", RegexOptions.None, RegexMatchTimeout)); } catch (EndOfStreamException ex) { @@ -78,13 +86,6 @@ namespace Microsoft.AspNetCore.SpaServices.AngularCli $"Angular CLI was listening for requests. The error output was: " + $"{stdErrReader.ReadAsString()}", ex); } - catch (TaskCanceledException ex) - { - throw new InvalidOperationException( - $"The Angular CLI process did not start listening for requests " + - $"within the timeout period of {StartupTimeout.Seconds} seconds. " + - $"Check the log output for error information.", ex); - } } var uri = new Uri(openBrowserLine.Groups[1].Value); diff --git a/src/Microsoft.AspNetCore.SpaServices.Extensions/Prerendering/SpaPrerenderingExtensions.cs b/src/Microsoft.AspNetCore.SpaServices.Extensions/Prerendering/SpaPrerenderingExtensions.cs index fea3bfb..ec41090 100644 --- a/src/Microsoft.AspNetCore.SpaServices.Extensions/Prerendering/SpaPrerenderingExtensions.cs +++ b/src/Microsoft.AspNetCore.SpaServices.Extensions/Prerendering/SpaPrerenderingExtensions.cs @@ -6,6 +6,7 @@ using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.NodeServices; using Microsoft.AspNetCore.SpaServices; +using Microsoft.AspNetCore.SpaServices.Extensions.Util; using Microsoft.AspNetCore.SpaServices.Prerendering; using Microsoft.Extensions.DependencyInjection; using Microsoft.Net.Http.Headers; @@ -23,6 +24,8 @@ namespace Microsoft.AspNetCore.Builder /// public static class SpaPrerenderingExtensions { + private static TimeSpan BuildTimeout = TimeSpan.FromSeconds(50); // Note that the HTTP request itself by default times out after 60s, so you only get useful error information if this is shorter + /// /// Enables server-side prerendering middleware for a Single Page Application. /// @@ -85,9 +88,15 @@ namespace Microsoft.AspNetCore.Builder } // If we're building on demand, wait for that to finish, or raise any build errors - if (buildOnDemandTask != null) + if (buildOnDemandTask != null && !buildOnDemandTask.IsCompleted) { - await buildOnDemandTask; + // For better debuggability, create a per-request timeout that makes it clear if the + // prerendering builder took too long for this request, but without aborting the + // underlying build task so that subsequent requests could still work. + await buildOnDemandTask.WithTimeout(BuildTimeout, + $"The prerendering build process did not complete within the " + + $"timeout period of {BuildTimeout.Seconds} seconds. " + + $"Check the log output for error information."); } // It's no good if we try to return a 304. We need to capture the actual diff --git a/src/Microsoft.AspNetCore.SpaServices.Extensions/Proxying/SpaProxyingExtensions.cs b/src/Microsoft.AspNetCore.SpaServices.Extensions/Proxying/SpaProxyingExtensions.cs index 066e581..3270c1b 100644 --- a/src/Microsoft.AspNetCore.SpaServices.Extensions/Proxying/SpaProxyingExtensions.cs +++ b/src/Microsoft.AspNetCore.SpaServices.Extensions/Proxying/SpaProxyingExtensions.cs @@ -45,7 +45,7 @@ namespace Microsoft.AspNetCore.Builder { UseProxyToSpaDevelopmentServer( spaBuilder, - Task.FromResult(baseUri)); + () => Task.FromResult(baseUri)); } /// @@ -54,10 +54,10 @@ namespace Microsoft.AspNetCore.Builder /// development. Do not enable this middleware in production applications. /// /// The . - /// A that resolves with the target base URI to which requests should be proxied. + /// A callback that will be invoked on each request to supply a that resolves with the target base URI to which requests should be proxied. public static void UseProxyToSpaDevelopmentServer( this ISpaBuilder spaBuilder, - Task baseUriTask) + Func> baseUriTaskFactory) { var applicationBuilder = spaBuilder.ApplicationBuilder; var applicationStoppingToken = GetStoppingToken(applicationBuilder); @@ -72,11 +72,11 @@ namespace Microsoft.AspNetCore.Builder var neverTimeOutHttpClient = SpaProxy.CreateHttpClientForProxy(Timeout.InfiniteTimeSpan); - // Proxy all requests into the Angular CLI server + // Proxy all requests to the SPA development server applicationBuilder.Use(async (context, next) => { var didProxyRequest = await SpaProxy.PerformProxyRequest( - context, neverTimeOutHttpClient, baseUriTask, applicationStoppingToken, + context, neverTimeOutHttpClient, baseUriTaskFactory(), applicationStoppingToken, proxy404s: true); }); } diff --git a/src/Microsoft.AspNetCore.SpaServices.Extensions/ReactDevelopmentServer/ReactDevelopmentServerMiddleware.cs b/src/Microsoft.AspNetCore.SpaServices.Extensions/ReactDevelopmentServer/ReactDevelopmentServerMiddleware.cs index 4a81562..146a648 100644 --- a/src/Microsoft.AspNetCore.SpaServices.Extensions/ReactDevelopmentServer/ReactDevelopmentServerMiddleware.cs +++ b/src/Microsoft.AspNetCore.SpaServices.Extensions/ReactDevelopmentServer/ReactDevelopmentServerMiddleware.cs @@ -11,6 +11,7 @@ using System.IO; using System.Collections.Generic; using System.Text.RegularExpressions; using System.Threading.Tasks; +using Microsoft.AspNetCore.SpaServices.Extensions.Util; namespace Microsoft.AspNetCore.SpaServices.ReactDevelopmentServer { @@ -48,7 +49,15 @@ namespace Microsoft.AspNetCore.SpaServices.ReactDevelopmentServer var targetUriTask = portTask.ContinueWith( task => new UriBuilder("http", "localhost", task.Result).Uri); - SpaProxyingExtensions.UseProxyToSpaDevelopmentServer(spaBuilder, targetUriTask); + SpaProxyingExtensions.UseProxyToSpaDevelopmentServer(spaBuilder, () => + { + // On each request, we create a separate startup task with its own timeout. That way, even if + // the first request times out, subsequent requests could still work. + return targetUriTask.WithTimeout(StartupTimeout, + $"The create-react-app server did not start listening for requests " + + $"within the timeout period of {StartupTimeout.Seconds} seconds. " + + $"Check the log output for error information."); + }); } private static async Task StartCreateReactAppServerAsync( @@ -75,8 +84,7 @@ namespace Microsoft.AspNetCore.SpaServices.ReactDevelopmentServer // no compiler warnings. So instead of waiting for that, consider it ready as soon // as it starts listening for requests. await npmScriptRunner.StdOut.WaitForMatch( - new Regex("Starting the development server", RegexOptions.None, RegexMatchTimeout), - StartupTimeout); + new Regex("Starting the development server", RegexOptions.None, RegexMatchTimeout)); } catch (EndOfStreamException ex) { @@ -85,13 +93,6 @@ namespace Microsoft.AspNetCore.SpaServices.ReactDevelopmentServer $"create-react-app server was listening for requests. The error output was: " + $"{stdErrReader.ReadAsString()}", ex); } - catch (TaskCanceledException ex) - { - throw new InvalidOperationException( - $"The create-react-app server did not start listening for requests " + - $"within the timeout period of {StartupTimeout.Seconds} seconds. " + - $"Check the log output for error information.", ex); - } } return portNumber; diff --git a/src/Microsoft.AspNetCore.SpaServices.Extensions/Util/EventedStreamReader.cs b/src/Microsoft.AspNetCore.SpaServices.Extensions/Util/EventedStreamReader.cs index 64a4f8c..95e018a 100644 --- a/src/Microsoft.AspNetCore.SpaServices.Extensions/Util/EventedStreamReader.cs +++ b/src/Microsoft.AspNetCore.SpaServices.Extensions/Util/EventedStreamReader.cs @@ -34,7 +34,7 @@ namespace Microsoft.AspNetCore.NodeServices.Util Task.Factory.StartNew(Run); } - public Task WaitForMatch(Regex regex, TimeSpan timeout = default) + public Task WaitForMatch(Regex regex) { var tcs = new TaskCompletionSource(); var completionLock = new object(); @@ -72,15 +72,6 @@ namespace Microsoft.AspNetCore.NodeServices.Util OnReceivedLine += onReceivedLineHandler; OnStreamClosed += onStreamClosedHandler; - if (timeout != default) - { - var timeoutToken = new CancellationTokenSource(timeout); - timeoutToken.Token.Register(() => - { - ResolveIfStillPending(() => tcs.SetCanceled()); - }); - } - return tcs.Task; } diff --git a/src/Microsoft.AspNetCore.SpaServices.Extensions/Util/TaskTimeoutExtensions.cs b/src/Microsoft.AspNetCore.SpaServices.Extensions/Util/TaskTimeoutExtensions.cs new file mode 100644 index 0000000..fe4d6b9 --- /dev/null +++ b/src/Microsoft.AspNetCore.SpaServices.Extensions/Util/TaskTimeoutExtensions.cs @@ -0,0 +1,35 @@ +// 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.Threading.Tasks; + +namespace Microsoft.AspNetCore.SpaServices.Extensions.Util +{ + internal static class TaskTimeoutExtensions + { + public static async Task WithTimeout(this Task task, TimeSpan timeoutDelay, string message) + { + if (task == await Task.WhenAny(task, Task.Delay(timeoutDelay))) + { + task.Wait(); // Allow any errors to propagate + } + else + { + throw new TimeoutException(message); + } + } + + public static async Task WithTimeout(this Task task, TimeSpan timeoutDelay, string message) + { + if (task == await Task.WhenAny(task, Task.Delay(timeoutDelay))) + { + return task.Result; + } + else + { + throw new TimeoutException(message); + } + } + } +}