From cc9367a966455b19333b3553bbc1778200017252 Mon Sep 17 00:00:00 2001 From: Pratik Sanglikar Date: Mon, 26 Jul 2021 15:10:27 -0700 Subject: [PATCH 1/3] Add timeout for Process.WaitForExit() --- src/Microsoft.Tye.Core/ProcessUtil.cs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.Tye.Core/ProcessUtil.cs b/src/Microsoft.Tye.Core/ProcessUtil.cs index a2d65090..01270a45 100644 --- a/src/Microsoft.Tye.Core/ProcessUtil.cs +++ b/src/Microsoft.Tye.Core/ProcessUtil.cs @@ -23,6 +23,8 @@ namespace Microsoft.Tye private static readonly bool IsWindows = RuntimeInformation.IsOSPlatform(OSPlatform.Windows); + private const int ProcessExitTimeoutMs = 60 * 1000; // 1 minute timeout for the process to exit. + public static Task ExecuteAsync( string command, string args, @@ -118,7 +120,12 @@ namespace Microsoft.Tye { // Even though the Exited event has been raised, WaitForExit() must still be called to ensure the output buffers // have been flushed before the process is considered completely done. - process.WaitForExit(); + // Because of the bug in the dotnet runtime https://github.com/dotnet/runtime/issues/29232, Process.WaitForExit() + // hangs for processes that spawn another long-running processes. + // Since these are expected to be long running processes and we're typically not concerned with capturing all of its output + // i.e. it's probably ok for some output to be lost on shutdown, since Tye is shutting down anyway, + // we call Process.WaitForProcessExit(ProcessExitTimeoutMs). + process.WaitForExit(ProcessExitTimeoutMs); } if (throwOnError && process.ExitCode != 0) From 227906b7b88c1025824b9845e6d536ff8655cf1a Mon Sep 17 00:00:00 2001 From: Pratik Sanglikar Date: Tue, 27 Jul 2021 09:36:57 -0700 Subject: [PATCH 2/3] Reduce timeout and fix broken test --- src/Microsoft.Tye.Core/ProcessUtil.cs | 2 +- test/E2ETest/ReplicaStoppingTests.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.Tye.Core/ProcessUtil.cs b/src/Microsoft.Tye.Core/ProcessUtil.cs index 01270a45..d0c8ad1b 100644 --- a/src/Microsoft.Tye.Core/ProcessUtil.cs +++ b/src/Microsoft.Tye.Core/ProcessUtil.cs @@ -23,7 +23,7 @@ namespace Microsoft.Tye private static readonly bool IsWindows = RuntimeInformation.IsOSPlatform(OSPlatform.Windows); - private const int ProcessExitTimeoutMs = 60 * 1000; // 1 minute timeout for the process to exit. + private const int ProcessExitTimeoutMs = 30 * 1000; // 30 seconds timeout for the process to exit. public static Task ExecuteAsync( string command, diff --git a/test/E2ETest/ReplicaStoppingTests.cs b/test/E2ETest/ReplicaStoppingTests.cs index 6ba8aa4e..91275181 100644 --- a/test/E2ETest/ReplicaStoppingTests.cs +++ b/test/E2ETest/ReplicaStoppingTests.cs @@ -61,7 +61,7 @@ namespace E2ETest var replicasToRestart = new[] { replicaToStop.Key }; var restOfReplicas = host.Application.Services.SelectMany(s => s.Value.Replicas).Select(r => r.Value.Name).Where(r => r != replicaToStop.Key).ToArray(); - Assert.True(await DoOperationAndWaitForReplicasToRestart(host, replicasToRestart.ToHashSet(), restOfReplicas.ToHashSet(), TimeSpan.FromSeconds(1), _ => + Assert.True(await DoOperationAndWaitForReplicasToRestart(host, replicasToRestart.ToHashSet(), restOfReplicas.ToHashSet(), TimeSpan.FromSeconds(30), _ => { replicaToStop.Value.StoppingTokenSource!.Cancel(); return Task.CompletedTask; From 79dd9690d11313793b0f8071c8c7a8eddf52b51b Mon Sep 17 00:00:00 2001 From: Pratik Sanglikar Date: Tue, 27 Jul 2021 15:58:11 -0700 Subject: [PATCH 3/3] Add supporting comments --- src/Microsoft.Tye.Core/ProcessUtil.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Microsoft.Tye.Core/ProcessUtil.cs b/src/Microsoft.Tye.Core/ProcessUtil.cs index d0c8ad1b..c89c4943 100644 --- a/src/Microsoft.Tye.Core/ProcessUtil.cs +++ b/src/Microsoft.Tye.Core/ProcessUtil.cs @@ -125,6 +125,7 @@ namespace Microsoft.Tye // Since these are expected to be long running processes and we're typically not concerned with capturing all of its output // i.e. it's probably ok for some output to be lost on shutdown, since Tye is shutting down anyway, // we call Process.WaitForProcessExit(ProcessExitTimeoutMs). + // Also, since this is a process.Exited event, process.ExitCode is valid even if WaitForExit() times out. process.WaitForExit(ProcessExitTimeoutMs); } @@ -134,6 +135,7 @@ namespace Microsoft.Tye } else { + // Since the process has exited, no additional data will be written to either output buffer or error buffer, it's thread-safe to call ToString() on both outputBuilder and errorBuilder. processLifetimeTask.TrySetResult(new ProcessResult(outputBuilder.ToString(), errorBuilder.ToString(), process.ExitCode)); } };