From 5d4ba2a81001481ddc8c871428f74bb86dd7a3d2 Mon Sep 17 00:00:00 2001 From: Codrin Poienaru Date: Wed, 7 Sep 2022 10:20:21 +0200 Subject: [PATCH 1/4] Fixed Selenium test hang --- .../common/System/ProcessHelper.cs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs index ef7e1a97db..7ed171c32f 100644 --- a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs +++ b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs @@ -84,6 +84,8 @@ void InitializeAndStart() if (exitCallBack != null) { + const int timeout = 500; + process.Exited += async (sender, args) => { if (sender is Process p) @@ -102,11 +104,20 @@ void InitializeAndStart() // // For older frameworks, the solution is more tricky but it seems we can get the expected // behavior using the parameterless 'WaitForExit()' combined with an awaited Task.Run call. - var cts = new CancellationTokenSource(500); + var cts = new CancellationTokenSource(timeout); #if NET5_0_OR_GREATER await p.WaitForExitAsync(cts.Token); #else - await Task.Run(() => p.WaitForExit(), cts.Token); + var os = new PlatformEnvironment().OperatingSystem; + if (os is PlatformOperatingSystem.Windows) + { + p.WaitForExit(timeout); + } + else + { + cts.Token.Register(() => p.Kill()); + await Task.Run(() => p.WaitForExit(), cts.Token).ConfigureAwait(false); + } #endif } catch From e541166ef9064a110809d59f5ea4ee77a0bf734f Mon Sep 17 00:00:00 2001 From: Codrin Poienaru Date: Wed, 7 Sep 2022 13:28:00 +0200 Subject: [PATCH 2/4] Fixed code review comments --- .../common/System/ProcessHelper.cs | 34 ++++++++++++++++--- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs index 7ed171c32f..c331f26bc8 100644 --- a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs +++ b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs @@ -26,6 +26,20 @@ public partial class ProcessHelper : IProcessHelper private static readonly string Arm = "arm"; private readonly Process _currentProcess = Process.GetCurrentProcess(); + private IEnvironment _environment; + + /// + /// Default constructor. + /// + public ProcessHelper() : this(new PlatformEnvironment()) + { + } + + internal ProcessHelper(IEnvironment environment) + { + _environment = environment; + } + /// public object LaunchProcess(string processPath, string? arguments, string? workingDirectory, IDictionary? envVariables, Action? errorCallback, Action? exitCallBack, Action? outputCallBack) { @@ -84,10 +98,10 @@ void InitializeAndStart() if (exitCallBack != null) { - const int timeout = 500; - process.Exited += async (sender, args) => { + const int timeout = 500; + if (sender is Process p) { try @@ -108,8 +122,20 @@ void InitializeAndStart() #if NET5_0_OR_GREATER await p.WaitForExitAsync(cts.Token); #else - var os = new PlatformEnvironment().OperatingSystem; - if (os is PlatformOperatingSystem.Windows) + // NOTE: In case we run on Windows we must call 'WaitForExit(timeout)' instead of calling + // the parameterless overload. The reason for this requirement stems from the behavior of + // the Selenium WebDriver when debugging a test. If the debugger is detached, the default + // action is to kill the testhost process that it was attached to, but for some reason we + // end up with a zombie process that would make us wait indefinitely with a simple + // 'WaitForExit()' call. This in turn causes the vstest.console to block waiting for the + // test request to finish and this behavior will be visible to the user since TW will + // show the Selenium test as still running. Only killing the Edge Driver process would help + // unblock vstest.console, but this is not a reasonable ask to our users. + // + // TODO: This fix is not ideal, it's only a workaround to make Selenium tests usable again. + // Ideally, we should spend some more time here in order to better understand what causes + // the testhost to become a zombie process in the first place. + if (_environment.OperatingSystem is PlatformOperatingSystem.Windows) { p.WaitForExit(timeout); } From aabe00f101f9e4aa4c59f2a7f71147dbca001479 Mon Sep 17 00:00:00 2001 From: Codrin Poienaru Date: Wed, 7 Sep 2022 14:48:50 +0200 Subject: [PATCH 3/4] Fixed pipeline fail on Ubuntu/macOS --- .../common/System/ProcessHelper.cs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs index c331f26bc8..037f2d266c 100644 --- a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs +++ b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs @@ -141,7 +141,13 @@ void InitializeAndStart() } else { - cts.Token.Register(() => p.Kill()); + cts.Token.Register(() => + { + if (!p.HasExited) + { + p.Kill(); + } + }); await Task.Run(() => p.WaitForExit(), cts.Token).ConfigureAwait(false); } #endif From ab015f87f8eae01d64e17df59c8e14789b6ff56b Mon Sep 17 00:00:00 2001 From: Codrin Poienaru Date: Wed, 7 Sep 2022 15:16:15 +0200 Subject: [PATCH 4/4] Fixed pipeline for Ubuntu/macOS attempt #2 --- .../common/System/ProcessHelper.cs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs index 037f2d266c..f1aa22de84 100644 --- a/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs +++ b/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs @@ -143,9 +143,18 @@ void InitializeAndStart() { cts.Token.Register(() => { - if (!p.HasExited) + try { - p.Kill(); + if (!p.HasExited) + { + p.Kill(); + } + } + catch + { + // Ignore all exceptions thrown when trying to kill a process that may be + // left hanging. This is a best effort to kill it, but should we fail for + // any reason we'd probably block on 'WaitForExit()' anyway. } }); await Task.Run(() => p.WaitForExit(), cts.Token).ConfigureAwait(false);