Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed Selenium test run hang after stopping the debugger #4013

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion eng/Versions.props
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<Project>
<PropertyGroup>
<!-- This repo version -->
<VersionPrefix>17.3.1</VersionPrefix>
<VersionPrefix>17.3.2</VersionPrefix>
<PreReleaseVersionLabel>release</PreReleaseVersionLabel>
<!-- Opt-out repo features -->
<UsingToolXliff>false</UsingToolXliff>
Expand Down
2 changes: 1 addition & 1 deletion scripts/build/TestPlatform.Settings.targets
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
<!-- This version is read by vsts-prebuild.ps1 and is a base for the current version, this should be updated
at the start of new iteration to the goal number. This is also used to version the local packages. This version needs to be statically
readable when we read the file as xml, don't move it to a .props file, unless you change the build server process -->
<TPVersionPrefix>17.3.1</TPVersionPrefix>
<TPVersionPrefix>17.3.2</TPVersionPrefix>
<LangVersion>preview</LangVersion>
<EnableNETAnalyzers>true</EnableNETAnalyzers>
<AnalysisLevel>preview</AnalysisLevel>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,20 @@ public partial class ProcessHelper : IProcessHelper
private static readonly string Arm = "arm";
private readonly Process _currentProcess = Process.GetCurrentProcess();

private IEnvironment _environment;

/// <summary>
/// Default constructor.
/// </summary>
public ProcessHelper() : this(new PlatformEnvironment())
{
}

internal ProcessHelper(IEnvironment environment)
{
_environment = environment;
}

/// <inheritdoc/>
public object LaunchProcess(string processPath, string? arguments, string? workingDirectory, IDictionary<string, string?>? envVariables, Action<object?, string?>? errorCallback, Action<object?>? exitCallBack, Action<object?, string?>? outputCallBack)
{
Expand Down Expand Up @@ -85,6 +99,8 @@ void InitializeAndStart()
{
process.Exited += async (sender, args) =>
{
const int timeout = 500;

if (sender is Process p)
{
try
Expand All @@ -101,11 +117,47 @@ 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);
// 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);
}
else
{
cts.Token.Register(() =>
{
try
{
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);
}
#endif
}
catch
Expand Down