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 4 commits
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
8 changes: 6 additions & 2 deletions src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyDiscoveryManager.cs
View file
Open in desktop
Expand Up @@ -7,6 +7,7 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client
using System.Collections.Generic;
using System.Linq;
using System.Text.RegularExpressions;
using System.Threading;

using Microsoft.VisualStudio.TestPlatform.Common;
using Microsoft.VisualStudio.TestPlatform.Common.ExtensionFramework;
Expand All @@ -26,6 +27,7 @@ public class ProxyDiscoveryManager : ProxyOperationManager, IProxyDiscoveryManag
{
private readonly ITestRuntimeProvider testHostManager;
private IDataSerializer dataSerializer;
private CancellationTokenSource cancellationTokenSource;

#region Constructors

Expand All @@ -50,6 +52,7 @@ public ProxyDiscoveryManager(ITestRequestSender testRequestSender, ITestRuntimeP
/// <param name="testHostManager">
/// Test host Manager instance
/// </param>
/// <param name="dataSerializer"></param>
/// <param name="clientConnectionTimeout">
/// The client Connection Timeout
/// </param>
Expand All @@ -62,6 +65,7 @@ public ProxyDiscoveryManager(ITestRequestSender testRequestSender, ITestRuntimeP
{
this.dataSerializer = dataSerializer;
this.testHostManager = testHostManager;
this.cancellationTokenSource = new CancellationTokenSource();
}

#endregion
Expand Down Expand Up @@ -97,7 +101,7 @@ public void DiscoverTests(DiscoveryCriteria discoveryCriteria, ITestDiscoveryEve
this.InitializeExtensions(discoveryCriteria.Sources);
}

this.SetupChannel(discoveryCriteria.Sources);
this.SetupChannel(discoveryCriteria.Sources, this.cancellationTokenSource.Token);
this.RequestSender.DiscoverTests(discoveryCriteria, eventHandler);
}
catch (Exception exception)
Expand Down Expand Up @@ -146,7 +150,7 @@ private void InitializeExtensions(IEnumerable<string> sources)
// Only send this if needed.
if (platformExtensions.Any())
{
this.SetupChannel(sourceList);
this.SetupChannel(sourceList, this.cancellationTokenSource.Token);

this.RequestSender.InitializeDiscovery(platformExtensions, TestPluginCache.Instance.LoadOnlyWellKnownExtensions);
}
Expand Down
16 changes: 10 additions & 6 deletions src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyExecutionManager.cs
View file
Open in desktop
Expand Up @@ -7,7 +7,10 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Linq;
using System.Text.RegularExpressions;
using System.Threading;

using Microsoft.VisualStudio.TestPlatform.Common;
using Microsoft.VisualStudio.TestPlatform.Common.ExtensionFramework;
using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities;
using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.Interfaces;
Expand All @@ -20,8 +23,6 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Logging;

using Constants = Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Constants;
using System.Text.RegularExpressions;
using Microsoft.VisualStudio.TestPlatform.Common;

/// <summary>
/// Orchestrates test execution operations for the engine communicating with the client.
Expand All @@ -30,17 +31,17 @@ internal class ProxyExecutionManager : ProxyOperationManager, IProxyExecutionMan
{
private readonly ITestRuntimeProvider testHostManager;
private IDataSerializer dataSerializer;
private CancellationTokenSource cancellationTokenSource;

/// <inheritdoc/>
public bool IsInitialized { get; private set; } = false;


#region Constructors

/// <summary>
/// Initializes a new instance of the <see cref="ProxyExecutionManager"/> class.
/// </summary>
/// <param name="testRequestSender">Test request sender instance.</param>
/// <param name="requestSender">Test request sender instance.</param>
/// <param name="testHostManager">Test host manager for this proxy.</param>
public ProxyExecutionManager(ITestRequestSender requestSender, ITestRuntimeProvider testHostManager) : this(requestSender, testHostManager, JsonDataSerializer.Instance, Constants.ClientConnectionTimeout)
{
Expand All @@ -52,12 +53,14 @@ public ProxyExecutionManager(ITestRequestSender requestSender, ITestRuntimeProvi
/// </summary>
/// <param name="requestSender">Request Sender instance</param>
/// <param name="testHostManager">Test host manager instance</param>
/// <param name="dataSerializer"></param>
/// <param name="clientConnectionTimeout">The client Connection Timeout</param>
internal ProxyExecutionManager(ITestRequestSender requestSender, ITestRuntimeProvider testHostManager, IDataSerializer dataSerializer, int clientConnectionTimeout)
: base(requestSender, testHostManager, clientConnectionTimeout)
{
this.testHostManager = testHostManager;
this.dataSerializer = dataSerializer;
this.cancellationTokenSource = new CancellationTokenSource();
}

#endregion
Expand All @@ -76,6 +79,7 @@ public virtual void Initialize()
EqtTrace.Verbose("ProxyExecutionManager: Test host is shared. SetupChannel it early.");
this.InitializeExtensions(Enumerable.Empty<string>());
}

this.IsInitialized = true;
}

Expand Down Expand Up @@ -105,7 +109,7 @@ public virtual int StartTestRun(TestRunCriteria testRunCriteria, ITestRunEventsH
this.InitializeExtensions(testSources);
}

this.SetupChannel(testRunCriteria.Sources);
this.SetupChannel(testRunCriteria.Sources, this.cancellationTokenSource.Token);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider defining this CancellationTokenSource in ProxyOperationManager?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

next PR


var executionContext = new TestExecutionContext(
testRunCriteria.FrequencyOfRunStatsChangeEvent,
Expand Down Expand Up @@ -191,7 +195,7 @@ private void InitializeExtensions(IEnumerable<string> sources)
// Only send this if needed.
if (platformExtensions.Any())
{
this.SetupChannel(sourceList);
this.SetupChannel(sourceList, this.cancellationTokenSource.Token);

this.RequestSender.InitializeExecution(platformExtensions, TestPluginCache.Instance.LoadOnlyWellKnownExtensions);
}
Expand Down
50 changes: 32 additions & 18 deletions src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyOperationManager.cs
View file
Open in desktop
Expand Up @@ -7,20 +7,19 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client
using System.Collections.Generic;
using System.Globalization;
using System.IO;
using System.Linq;
using System.Reflection;
using System.Threading;
using System.Threading.Tasks;

using Microsoft.VisualStudio.TestPlatform.CoreUtilities.Extensions;
using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.Interfaces;
using Microsoft.VisualStudio.TestPlatform.CoreUtilities.Extensions;
using Microsoft.VisualStudio.TestPlatform.ObjectModel;
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Host;
using Microsoft.VisualStudio.TestPlatform.PlatformAbstractions;
using Microsoft.VisualStudio.TestPlatform.PlatformAbstractions.Interfaces;
using Microsoft.VisualStudio.TestPlatform.Utilities;

using CrossPlatEngineResources = Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Resources.Resources;
using System.Reflection;
using System.Linq;

/// <summary>
/// Base class for any operations that the client needs to drive through the engine.
Expand All @@ -32,6 +31,7 @@ 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 bool initialized;
private string testHostProcessStdError;
Expand Down Expand Up @@ -75,7 +75,8 @@ protected ProxyOperationManager(ITestRequestSender requestSender, ITestRuntimePr
/// Usually includes starting up the test host process.
/// </summary>
/// <param name="sources">List of test sources.</param>
public virtual void SetupChannel(IEnumerable<string> sources)
/// <param name="cancellationToken"></param>
public virtual void SetupChannel(IEnumerable<string> sources, CancellationToken cancellationToken)
{
var connTimeout = this.connectionTimeout;

Expand All @@ -91,29 +92,28 @@ public virtual void SetupChannel(IEnumerable<string> sources)
// 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.

Why not use the Exited event for error too?


// Get the test process start info
var testHostStartInfo = this.UpdateTestProcessStartInfo(this.testHostManager.GetTestHostProcessStartInfo(sources, null, connectionInfo));

// Launch the test host.
var hostLaunchedTask = this.testHostManager.LaunchTestHostAsync(testHostStartInfo);

try
{
// Launch the test host.
var hostLaunchedTask = this.testHostManager.LaunchTestHostAsync(testHostStartInfo, cancellationToken);
this.testHostProcessId = hostLaunchedTask.Result;
}
catch (OperationCanceledException ex)
catch (Exception ex)
{
throw new TestPlatformException(string.Format(CultureInfo.CurrentUICulture, ex.Message));
}

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

// 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}", hostLaunchedTask.Result, this.processHelper.GetProcessName(hostLaunchedTask.Result)),
string.Format("Process Id: {0}, Name: {1}", this.testHostProcessId, this.processHelper.GetProcessName(this.testHostProcessId)),
OutputLevel.Information);

// Increase connection timeout when debugging is enabled.
Expand All @@ -125,7 +125,7 @@ public virtual void SetupChannel(IEnumerable<string> sources)
{
var errorMsg = CrossPlatEngineResources.InitializationFailed;

if (!string.IsNullOrWhiteSpace(this.testHostProcessStdError.ToString()))
if (!string.IsNullOrWhiteSpace(this.testHostProcessStdError))
{
// Testhost failed with error
errorMsg = string.Format(CrossPlatEngineResources.TestHostExitedWithError, this.testHostProcessStdError);
Expand Down Expand Up @@ -160,7 +160,11 @@ public virtual void Close()
{
try
{
this.RequestSender.EndSession();
// do not send message if host did not launch
if (this.testHostProcessId != -1)
{
this.RequestSender.EndSession();
}
}
catch (Exception ex)
{
Expand All @@ -172,16 +176,18 @@ public virtual void Close()
{
this.initialized = false;

// We don't need to terminate if the test host has already terminated. The upper bound
// for wait should be 100ms.
if (this.testHostProcessId != -1 && !this.testHostExited.Wait(100))
// 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))
{
EqtTrace.Warning("ProxyOperationManager: Timed out waiting for test host to exit. Will terminate process.");
this.testHostManager.TerminateAsync(this.testHostProcessId, CancellationToken.None).Wait();
this.testHostManager.CleanTestHostAsync(this.testHostProcessId, CancellationToken.None).Wait();
}

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

Expand All @@ -205,7 +211,9 @@ protected virtual TestProcessStartInfo UpdateTestProcessStartInfo(TestProcessSta
protected string GetTimestampedLogFile(string logFile)
{
if (string.IsNullOrWhiteSpace(logFile))
{
return null;
}

return Path.ChangeExtension(
logFile,
Expand All @@ -229,5 +237,11 @@ private void TestHostManagerHostExited(object sender, HostProviderEventArgs e)

this.testHostExited.Set();
}

private void TestHostManagerHostLaunchFailure(object sender, HostProviderEventArgs e)
{
EqtTrace.Verbose(e.Data);
this.testHostLaunchError.Set();
}
}
}
2 changes: 1 addition & 1 deletion src/Microsoft.TestPlatform.CrossPlatEngine/TestEngine.cs
View file
Open in desktop
Expand Up @@ -9,6 +9,7 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine
using Microsoft.VisualStudio.TestPlatform.Common.Hosting;
using Microsoft.VisualStudio.TestPlatform.Common.Logging;
using Microsoft.VisualStudio.TestPlatform.Common.Utilities;
using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities;
using Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client;
using Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.Parallel;
using Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.DataCollection;
Expand All @@ -17,7 +18,6 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Engine;
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Host;
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Utilities;
using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities;

/// <summary>
/// Cross Platform test engine entry point for the client.
Expand Down
26 changes: 20 additions & 6 deletions src/Microsoft.TestPlatform.ObjectModel/Host/ITestRunTimeProvider.cs
View file
Open in desktop
Expand Up @@ -5,12 +5,12 @@ namespace Microsoft.VisualStudio.TestPlatform.ObjectModel.Host
{
using System;
using System.Collections.Generic;
using System.Threading;
using System.Threading.Tasks;

using Microsoft.VisualStudio.TestPlatform.ObjectModel;
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.Interfaces;
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Logging;
using System.Threading;

/// <summary>
/// Interface for TestRuntimeProvider which manages test host processes for test engine.
Expand All @@ -24,7 +24,12 @@ public interface ITestRuntimeProvider
event EventHandler<HostProviderEventArgs> HostLaunched;

/// <summary>
/// Raised when host is reports Error
/// 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>
event EventHandler<HostProviderEventArgs> HostExited;

Expand Down Expand Up @@ -65,8 +70,9 @@ public interface ITestRuntimeProvider
/// Launches the test host for discovery/execution.
/// </summary>
/// <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);
Task<int> 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.

I think the return code is used to terminate the host later.

Will the return type be always an int? Could there be a scenario where the TestRuntimeProvider requires more information (like the Emulator prodess id and the testhost process id)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If Runtime provider need to keep additional information, then it can store that info locally. As for lauchasync it's job it to return whether host was successfully launched 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.

Make this a bool or just Task then?


/// <summary>
/// Gets the start parameters for the test host.
Expand All @@ -83,15 +89,23 @@ public interface ITestRuntimeProvider
/// mechanism. E.g. for .net core, extensions are discovered from the <c>testproject.deps.json</c> file.
/// </summary>
/// <param name="sources">List of test sources.</param>
/// <param name="extensions"></param>
/// <returns>List of paths to extension assemblies.</returns>
IEnumerable<string> GetTestPlatformExtensions(IEnumerable<string> sources, IEnumerable<string> extensions);

/// <summary>
/// Terminate the test host process.
/// </summary>
/// <param name="processId">Process identifier for the test host.</param>
/// <param name="cancellationToken">Cancellation token.</param>
Task TerminateAsync(int processId, CancellationToken cancellationToken);
/// <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);
}

/// <summary>
Expand Down