From ad79cdd1239b8f50ea81de41a52dbd1229b47267 Mon Sep 17 00:00:00 2001 From: Andrew Stanton-Nurse Date: Thu, 6 Apr 2017 11:15:45 -0700 Subject: [PATCH] fix hangs due to uncleared TCS in IntegrationTesting (#1008) --- .../Common/TaskTimeoutExtensions.cs | 35 ------------------- .../Deployers/IISExpressDeployer.cs | 14 ++++++-- .../Deployers/SelfHostDeployer.cs | 12 +++++-- 3 files changed, 21 insertions(+), 40 deletions(-) delete mode 100644 src/Microsoft.AspNetCore.Server.IntegrationTesting/Common/TaskTimeoutExtensions.cs diff --git a/src/Microsoft.AspNetCore.Server.IntegrationTesting/Common/TaskTimeoutExtensions.cs b/src/Microsoft.AspNetCore.Server.IntegrationTesting/Common/TaskTimeoutExtensions.cs deleted file mode 100644 index 2045f47f9a..0000000000 --- a/src/Microsoft.AspNetCore.Server.IntegrationTesting/Common/TaskTimeoutExtensions.cs +++ /dev/null @@ -1,35 +0,0 @@ -// 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. - -namespace System.Threading.Tasks -{ - internal static class TaskTimeoutExtensions - { - public static async Task OrTimeout(this Task task, TimeSpan timeout) - { - var completed = await Task.WhenAny(task, Task.Delay(timeout)); - if (completed == task) - { - // Manifest any exception - task.GetAwaiter().GetResult(); - } - else - { - throw new TimeoutException(); - } - } - - public static async Task OrTimeout(this Task task, TimeSpan timeout) - { - var completed = await Task.WhenAny(task, Task.Delay(timeout)); - if (completed == task) - { - return await task; - } - else - { - throw new TimeoutException(); - } - } - } -} diff --git a/src/Microsoft.AspNetCore.Server.IntegrationTesting/Deployers/IISExpressDeployer.cs b/src/Microsoft.AspNetCore.Server.IntegrationTesting/Deployers/IISExpressDeployer.cs index 3c8311f391..b1220198aa 100644 --- a/src/Microsoft.AspNetCore.Server.IntegrationTesting/Deployers/IISExpressDeployer.cs +++ b/src/Microsoft.AspNetCore.Server.IntegrationTesting/Deployers/IISExpressDeployer.cs @@ -9,6 +9,7 @@ using System.Text.RegularExpressions; using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Server.IntegrationTesting.Common; +using Microsoft.AspNetCore.Testing; using Microsoft.Extensions.Logging; namespace Microsoft.AspNetCore.Server.IntegrationTesting @@ -213,6 +214,10 @@ namespace Microsoft.AspNetCore.Server.IntegrationTesting process.Exited += (sender, e) => { Logger.LogInformation("iisexpress Process {pid} shut down", process.Id); + + // If TrySetResult was called above, this will just silently fail to set the new state, which is what we want + started.TrySetException(new Exception($"Command exited unexpectedly with exit code: {process.ExitCode}")); + TriggerHostShutdown(hostExitTokenSource); }; process.StartAndCaptureOutAndErrToLogger("iisexpress", Logger); @@ -225,7 +230,10 @@ namespace Microsoft.AspNetCore.Server.IntegrationTesting } // Wait for the app to start - if (!await started.Task) + // The timeout here is large, because we don't know how long the test could need + // We cover a lot of error cases above, but I want to make sure we eventually give up and don't hang the build + // just in case we missed one -anurse + if (!await started.Task.TimeoutAfter(TimeSpan.FromMinutes(10))) { Logger.LogInformation("iisexpress Process {pid} failed to bind to port {port}, trying again", _hostProcess.Id, port); @@ -294,10 +302,10 @@ namespace Microsoft.AspNetCore.Server.IntegrationTesting // If by this point, the host process is still running (somehow), throw an error. // A test failure is better than a silent hang and unknown failure later on - if(!_hostProcess.HasExited) + if (!_hostProcess.HasExited) { throw new Exception($"iisexpress Process {_hostProcess.Id} failed to shutdown"); } } } -} \ No newline at end of file +} diff --git a/src/Microsoft.AspNetCore.Server.IntegrationTesting/Deployers/SelfHostDeployer.cs b/src/Microsoft.AspNetCore.Server.IntegrationTesting/Deployers/SelfHostDeployer.cs index 03600cce47..34e9b5cdb3 100644 --- a/src/Microsoft.AspNetCore.Server.IntegrationTesting/Deployers/SelfHostDeployer.cs +++ b/src/Microsoft.AspNetCore.Server.IntegrationTesting/Deployers/SelfHostDeployer.cs @@ -9,6 +9,7 @@ using System.Text.RegularExpressions; using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Server.IntegrationTesting.Common; +using Microsoft.AspNetCore.Testing; using Microsoft.Extensions.Logging; namespace Microsoft.AspNetCore.Server.IntegrationTesting @@ -140,6 +141,10 @@ namespace Microsoft.AspNetCore.Server.IntegrationTesting HostProcess.Exited += (sender, e) => { Logger.LogInformation("host process ID {pid} shut down", HostProcess.Id); + + // If TrySetResult was called above, this will just silently fail to set the new state, which is what we want + started.TrySetException(new Exception($"Command exited unexpectedly with exit code: {HostProcess.ExitCode}")); + TriggerHostShutdown(hostExitTokenSource); }; @@ -160,7 +165,10 @@ namespace Microsoft.AspNetCore.Server.IntegrationTesting Logger.LogInformation("Started {fileName}. Process Id : {processId}", startInfo.FileName, HostProcess.Id); - await started.Task; + // The timeout here is large, because we don't know how long the test could need + // We cover a lot of error cases above, but I want to make sure we eventually give up and don't hang the build + // just in case we missed one -anurse + await started.Task.TimeoutAfter(TimeSpan.FromMinutes(10)); return (url: actualUrl ?? hintUrl, hostExitToken: hostExitTokenSource.Token); } @@ -183,4 +191,4 @@ namespace Microsoft.AspNetCore.Server.IntegrationTesting } } } -} \ No newline at end of file +}