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.

Sign up for GitHub

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

Jump to bottom

Adding CancellationToken to Testhost launch async #917

Merged
merged 9 commits into from Jul 11, 2017
Merged

Adding CancellationToken to Testhost launch async #917

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7d07627
Adding CancellationToken to Testhost launch async
mayankbansal018 Jul 6, 2017
f164a45
LaunchTestHost need not always throw
mayankbansal018 Jul 7, 2017
dcdd23b
Adding test for Cancellation Token
mayankbansal018 Jul 7, 2017
4abac27
Adding testhostlaunch failure event in case operation was cancelled b…
mayankbansal018 Jul 10, 2017
6798fa3
Clean up of ITestRuntimeProvider
mayankbansal018 Jul 10, 2017
6c78e6b
PR Comments
mayankbansal018 Jul 11, 2017
c7cc72f
Merge branch 'master' into runtime-launchhostcancellable
mayankbansal018 Jul 11, 2017
cb512af
Updated Comments
mayankbansal018 Jul 11, 2017
6e54fb6
PR comments
mayankbansal018 Jul 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
69 changes: 34 additions & 35 deletions src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyOperationManager.cs
View file
Open in desktop
Expand Up @@ -31,11 +31,11 @@ public abstract class ProxyOperationManager
private readonly int connectionTimeout;
private readonly string versionCheckPropertyName = "IsVersionCheckRequired";
private readonly ManualResetEventSlim testHostExited = new ManualResetEventSlim(false);
private readonly ManualResetEventSlim testHostLaunchError = new ManualResetEventSlim(false);
private readonly ManualResetEventSlim testHostConnectionTimeOut = new ManualResetEventSlim(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: timeout is one word

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at usage, suggest to name it as channelConnected.


private bool initialized;
private string testHostProcessStdError;
private int testHostProcessId;
private bool testHostLaunchSuccess;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: testHostLaunched


#region Constructors

Expand All @@ -52,7 +52,7 @@ protected ProxyOperationManager(ITestRequestSender requestSender, ITestRuntimePr
this.testHostManager = testHostManager;
this.processHelper = new ProcessHelper();
this.initialized = false;
this.testHostProcessId = -1;
this.testHostLaunchSuccess = false;
}

#endregion
Expand All @@ -70,6 +70,8 @@ protected ProxyOperationManager(ITestRequestSender requestSender, ITestRuntimePr

#region IProxyOperationManager implementation.

// ToDo: change SetupChannel to return bool, which will specify that a successfull connection with testhost was established or not
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: inside SetupChannel method body.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved


/// <summary>
/// Ensure that the engine is ready for test operations.
/// Usually includes starting up the test host process.
Expand All @@ -78,8 +80,6 @@ protected ProxyOperationManager(ITestRequestSender requestSender, ITestRuntimePr
/// <param name="cancellationToken"></param>
public virtual void SetupChannel(IEnumerable<string> sources, CancellationToken cancellationToken)
{
var connTimeout = this.connectionTimeout;

if (!this.initialized)
{
this.testHostProcessStdError = string.Empty;
Expand All @@ -92,36 +92,23 @@ public virtual void SetupChannel(IEnumerable<string> sources, CancellationToken
// Subscribe to TestHost Event
this.testHostManager.HostLaunched += this.TestHostManagerHostLaunched;
this.testHostManager.HostExited += this.TestHostManagerHostExited;
this.testHostManager.HostLaunchFailure += this.TestHostManagerHostLaunchFailure;


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove extra space

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved

// Get the test process start info
var testHostStartInfo = this.UpdateTestProcessStartInfo(this.testHostManager.GetTestHostProcessStartInfo(sources, null, connectionInfo));
try
{
// Launch the test host.
var hostLaunchedTask = this.testHostManager.LaunchTestHostAsync(testHostStartInfo, cancellationToken);
this.testHostProcessId = hostLaunchedTask.Result;
this.testHostLaunchSuccess = hostLaunchedTask.Result;
}
catch (Exception ex)
{
throw new TestPlatformException(string.Format(CultureInfo.CurrentUICulture, ex.Message));
}

// Warn the user that execution will wait for debugger attach.
var hostDebugEnabled = Environment.GetEnvironmentVariable("VSTEST_HOST_DEBUG");
if (!string.IsNullOrEmpty(hostDebugEnabled) && hostDebugEnabled.Equals("1", StringComparison.Ordinal))
{
ConsoleOutput.Instance.WriteLine(CrossPlatEngineResources.HostDebuggerWarning, OutputLevel.Warning);
ConsoleOutput.Instance.WriteLine(
string.Format("Process Id: {0}, Name: {1}", this.testHostProcessId, this.processHelper.GetProcessName(this.testHostProcessId)),
OutputLevel.Information);

// Increase connection timeout when debugging is enabled.
connTimeout = 5 * this.connectionTimeout;
}

// Wait for a timeout for the client to connect.
if (!this.RequestSender.WaitForRequestHandlerConnection(connTimeout))
// test host launched failed because of user cancellation or any other runtime exception
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: comments start with Caps

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved

// or connection with test host did not happen
if (!this.testHostLaunchSuccess || this.testHostConnectionTimeOut.Wait(0))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ManualResetEventSlim.Wait == a bool type?

{
var errorMsg = CrossPlatEngineResources.InitializationFailed;

Expand Down Expand Up @@ -161,7 +148,7 @@ public virtual void Close()
try
{
// do not send message if host did not launch
if (this.testHostProcessId != -1)
if (this.testHostLaunchSuccess)
{
this.RequestSender.EndSession();
}
Expand All @@ -179,15 +166,14 @@ public virtual void Close()
// We don't need to terminate if the test host has already terminated.
// If the host launch fails, please clean test host.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the pattern be simple?

  • If the consumer is exiting, always call CleanTestHostAsync. It is the responsibility of Clean to ensure that there are no lingering child processes

If there were no test host, it is up to CleanTestHostAsync to do what it chooses.

Thinking aloud, does IDisposable or Close make more sense than CleanXyz?

// The upper bound for wait should be 100ms.
if ((this.testHostProcessId != -1 && !this.testHostExited.Wait(100)) || this.testHostLaunchError.Wait(0))
if (!this.testHostExited.Wait(100))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean should be called irrespective of whether the test host has exited or not.

{
EqtTrace.Warning("ProxyOperationManager: Timed out waiting for test host to exit. Will terminate process.");
this.testHostManager.CleanTestHostAsync(this.testHostProcessId, CancellationToken.None).Wait();
this.testHostManager.CleanTestHostAsync(CancellationToken.None).Wait();
}

this.testHostManager.HostExited -= this.TestHostManagerHostExited;
this.testHostManager.HostLaunched -= this.TestHostManagerHostLaunched;
this.testHostManager.HostLaunchFailure -= this.TestHostManagerHostLaunchFailure;
}
}

Expand Down Expand Up @@ -224,9 +210,28 @@ protected string GetTimestampedLogFile(string logFile)
Path.GetExtension(logFile))).AddDoubleQuote();
}

private void TestHostManagerHostLaunched(object sender, HostProviderEventArgs e)
public void TestHostManagerHostLaunched(object sender, HostProviderEventArgs e)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this public?

{
EqtTrace.Verbose(e.Data);
var connTimeout = this.connectionTimeout;

// Warn the user that execution will wait for debugger attach.
var hostDebugEnabled = Environment.GetEnvironmentVariable("VSTEST_HOST_DEBUG");
if (!string.IsNullOrEmpty(hostDebugEnabled) && hostDebugEnabled.Equals("1", StringComparison.Ordinal))
{
ConsoleOutput.Instance.WriteLine(CrossPlatEngineResources.HostDebuggerWarning, OutputLevel.Warning);
ConsoleOutput.Instance.WriteLine(
string.Format("Process Id: {0}, Name: {1}", e.ProcessId, this.processHelper.GetProcessName(e.ProcessId)),
OutputLevel.Information);

// Increase connection timeout when debugging is enabled.
connTimeout = 5 * this.connectionTimeout;
}

// Wait for a timeout for the client to connect.
if (!this.RequestSender.WaitForRequestHandlerConnection(connTimeout))
{
this.testHostConnectionTimeOut.Set();
}
}

private void TestHostManagerHostExited(object sender, HostProviderEventArgs e)
Expand All @@ -237,11 +242,5 @@ private void TestHostManagerHostExited(object sender, HostProviderEventArgs e)

this.testHostExited.Set();
}

private void TestHostManagerHostLaunchFailure(object sender, HostProviderEventArgs e)
{
EqtTrace.Verbose(e.Data);
this.testHostLaunchError.Set();
}
}
}
13 changes: 3 additions & 10 deletions src/Microsoft.TestPlatform.ObjectModel/Host/ITestRunTimeProvider.cs
View file
Open in desktop
Expand Up @@ -20,14 +20,10 @@ public interface ITestRuntimeProvider
#region events
/// <summary>
/// Raised when host is launched successfully
/// Consumed by TestPlatform to initialize connection b/w test host and testplatform
/// </summary>
event EventHandler<HostProviderEventArgs> HostLaunched;

/// <summary>
/// Raised when host launch reports an Error
/// </summary>
event EventHandler<HostProviderEventArgs> HostLaunchFailure;

/// <summary>
/// Raised when host is cleaned up and removes all it's dependecies
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: spell check

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved

/// </summary>
Expand Down Expand Up @@ -72,7 +68,7 @@ public interface ITestRuntimeProvider
/// <param name="testHostStartInfo">Start parameters for the test host.</param>
/// <param name="cancellationToken"></param>
/// <returns>ProcessId of launched Process. 0 means not launched.</returns>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to update return string as the signature has been updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved

Task<int> LaunchTestHostAsync(TestProcessStartInfo testHostStartInfo, CancellationToken cancellationToken);
Task<bool> LaunchTestHostAsync(TestProcessStartInfo testHostStartInfo, CancellationToken cancellationToken);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update documentation


/// <summary>
/// Gets the start parameters for the test host.
Expand All @@ -96,16 +92,13 @@ public interface ITestRuntimeProvider
/// <summary>
/// Terminate the test host process.
/// </summary>
/// <param name="processId">
/// Process identifier for the test host.
/// </param>
/// <param name="cancellationToken">
/// Cancellation token.
/// </param>
/// <returns>
/// The <see cref="Task"/>.
/// </returns>
Task CleanTestHostAsync(int processId, CancellationToken cancellationToken);
Task CleanTestHostAsync(CancellationToken cancellationToken);
}

/// <summary>
Expand Down
29 changes: 10 additions & 19 deletions src/Microsoft.TestPlatform.TestHostProvider/Hosting/DefaultTestHostManager.cs
View file
Open in desktop
Expand Up @@ -78,9 +78,6 @@ internal DefaultTestHostManager(IProcessHelper processHelper, IEnvironment envir
/// <inheritdoc/>
public event EventHandler<HostProviderEventArgs> HostExited;

/// <inheritdoc/>
public event EventHandler<HostProviderEventArgs> HostLaunchFailure;

/// <inheritdoc/>
public bool Shared { get; private set; }

Expand Down Expand Up @@ -117,7 +114,7 @@ public void SetCustomLauncher(ITestHostLauncher customLauncher)
}

/// <inheritdoc/>
public async Task<int> LaunchTestHostAsync(TestProcessStartInfo testHostStartInfo, CancellationToken cancellationToken)
public virtual async Task<bool> LaunchTestHostAsync(TestProcessStartInfo testHostStartInfo, CancellationToken cancellationToken)
{
return await Task.Run(() => this.LaunchHost(testHostStartInfo, cancellationToken), cancellationToken);
}
Expand Down Expand Up @@ -205,11 +202,11 @@ public void Initialize(IMessageLogger logger, string runsettingsXml)
}

/// <inheritdoc/>
public Task CleanTestHostAsync(int processId, CancellationToken cancellationToken)
public Task CleanTestHostAsync(CancellationToken cancellationToken)
{
try
{
this.processHelper.TerminateProcess(processId);
this.processHelper.TerminateProcess(this.testHostProcess.Id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can testHostProcess be null?

}
catch (Exception ex)
{
Expand All @@ -225,7 +222,7 @@ public Task CleanTestHostAsync(int processId, CancellationToken cancellationToke
/// Raises HostLaunched event
/// </summary>
/// <param name="e">hostprovider event args</param>
private void OnHostLaunched(HostProviderEventArgs e)
protected virtual void OnHostLaunched(HostProviderEventArgs e)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if making a event handler virtual is a good idea.

{
this.HostLaunched.SafeInvoke(this, e, "HostProviderEvents.OnHostLaunched");
}
Expand All @@ -234,7 +231,7 @@ private void OnHostLaunched(HostProviderEventArgs e)
/// Raises HostExited event
/// </summary>
/// <param name="e">hostprovider event args</param>
private void OnHostExited(HostProviderEventArgs e)
protected virtual void OnHostExited(HostProviderEventArgs e)
{
if (!this.hostExitedEventRaised)
{
Expand All @@ -243,12 +240,7 @@ private void OnHostExited(HostProviderEventArgs e)
}
}

private void OnHostLaunchFailure(HostProviderEventArgs e)
{
this.HostLaunchFailure.SafeInvoke(this, e, "HostProviderEvents.HostLaunchError");
}

private int LaunchHost(TestProcessStartInfo testHostStartInfo, CancellationToken cancellationToken)
private bool LaunchHost(TestProcessStartInfo testHostStartInfo, CancellationToken cancellationToken)
{
try
{
Expand All @@ -270,13 +262,12 @@ private int LaunchHost(TestProcessStartInfo testHostStartInfo, CancellationToken
}
catch (OperationCanceledException ex)
{
this.OnHostLaunchFailure(new HostProviderEventArgs(ex.Message, -1, 0));
return -1;
this.messageLogger.SendMessage(TestMessageLevel.Error, ex.Message);
return false;
}

var pId = this.testHostProcess != null ? this.testHostProcess.Id : 0;
this.OnHostLaunched(new HostProviderEventArgs("Test Runtime launched with Pid: " + pId));
return pId;
this.OnHostLaunched(new HostProviderEventArgs("Test Runtime launched", 0, this.testHostProcess.Id));
return this.testHostProcess != null;
}
}
}
25 changes: 8 additions & 17 deletions src/Microsoft.TestPlatform.TestHostProvider/Hosting/DotnetTestHostManager.cs
View file
Open in desktop
Expand Up @@ -93,9 +93,6 @@ public DotnetTestHostManager()
/// <inheritdoc />
public event EventHandler<HostProviderEventArgs> HostExited;

/// <inheritdoc />
public event EventHandler<HostProviderEventArgs> HostLaunchFailure;

/// <summary>
/// Gets a value indicating whether gets a value indicating if the test host can be shared for multiple sources.
/// </summary>
Expand Down Expand Up @@ -145,7 +142,7 @@ public void SetCustomLauncher(ITestHostLauncher customLauncher)
}

/// <inheritdoc/>
public virtual async Task<int> LaunchTestHostAsync(TestProcessStartInfo testHostStartInfo, CancellationToken cancellationToken)
public virtual async Task<bool> LaunchTestHostAsync(TestProcessStartInfo testHostStartInfo, CancellationToken cancellationToken)
{
return await Task.Run(() => this.LaunchHost(testHostStartInfo, cancellationToken), cancellationToken);
}
Expand Down Expand Up @@ -270,11 +267,11 @@ public bool CanExecuteCurrentRunConfiguration(string runsettingsXml)
}

/// <inheritdoc/>
public Task CleanTestHostAsync(int processId, CancellationToken cancellationToken)
public Task CleanTestHostAsync(CancellationToken cancellationToken)
{
try
{
this.processHelper.TerminateProcess(processId);
this.processHelper.TerminateProcess(this.testHostProcess.Id);
}
catch (Exception ex)
{
Expand Down Expand Up @@ -308,12 +305,7 @@ private void OnHostExited(HostProviderEventArgs e)
}
}

private void OnHostLaunchFailure(HostProviderEventArgs e)
{
this.HostLaunchFailure.SafeInvoke(this, e, "HostProviderEvents.HostLaunchFailure");
}

private int LaunchHost(TestProcessStartInfo testHostStartInfo, CancellationToken cancellationToken)
private bool LaunchHost(TestProcessStartInfo testHostStartInfo, CancellationToken cancellationToken)
{
try
{
Expand All @@ -333,14 +325,13 @@ private int LaunchHost(TestProcessStartInfo testHostStartInfo, CancellationToken
}
catch (OperationCanceledException ex)
{
this.OnHostLaunchFailure(new HostProviderEventArgs(ex.Message, -1, 0));
return -1;
this.messageLogger.SendMessage(TestMessageLevel.Error, ex.Message);
return false;
}

var pId = this.testHostProcess != null ? this.testHostProcess.Id : 0;
this.OnHostLaunched(new HostProviderEventArgs("Test Runtime launched with Pid: " + pId));
this.OnHostLaunched(new HostProviderEventArgs("Test Runtime launched", 0, this.testHostProcess.Id));

return pId;
return this.testHostProcess != null;
}

private string GetTestHostPath(string runtimeConfigDevPath, string depsFilePath, string sourceDirectory)
Expand Down