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

Cancel/abort not honored when sent before custom host launch #1543

Merged
merged 20 commits into from
Apr 23, 2018
Merged
Show file tree
Hide file tree
Changes from 4 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
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ public CustomTestHostLauncher(Action callback)

public bool IsDebug => false;

public int LaunchTestHost(TestProcessStartInfo defaultTestHostStartInfo)
public int LaunchTestHost(TestProcessStartInfo defaultTestHostStartInfo, CancellationToken cancellationToken)
{
var processInfo = new ProcessStartInfo(
defaultTestHostStartInfo.FileName,
Expand All @@ -192,6 +192,11 @@ public int LaunchTestHost(TestProcessStartInfo defaultTestHostStartInfo)

throw new Exception("Process in invalid state.");
}

public int LaunchTestHost(TestProcessStartInfo defaultTestHostStartInfo)
{
return this.LaunchTestHost(defaultTestHostStartInfo, CancellationToken.None);
}
}

public class DiscoveryEventHandler : ITestDiscoveryEventsHandler
Expand Down
12 changes: 11 additions & 1 deletion src/Microsoft.TestPlatform.Client/DesignMode/DesignModeClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -235,10 +235,13 @@ private void ProcessRequests(ITestRequestManager testRequestManager)
/// <param name="testProcessStartInfo">
/// The test Process Start Info.
/// </param>
/// <param name="cancellationToken">
/// The cancellation token.
/// </param>
/// <returns>
/// The <see cref="int"/>.
/// </returns>
public int LaunchCustomHost(TestProcessStartInfo testProcessStartInfo)
public int LaunchCustomHost(TestProcessStartInfo testProcessStartInfo, CancellationToken cancellationToken)
{
lock (ackLockObject)
{
Expand All @@ -250,6 +253,9 @@ public int LaunchCustomHost(TestProcessStartInfo testProcessStartInfo)
waitHandle.Set();
};

// Registering cancellationToken to set waitHandle (whenever request is cancelled).
var cancellationTokenRegistration = cancellationToken.Register(() => waitHandle.Set());
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: would something like this be more direct? Msdn usage here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.


this.communicationManager.SendMessage(MessageType.CustomTestHostLaunch, testProcessStartInfo);

// LifeCycle of the TP through DesignModeClient is maintained by the IDEs or user-facing-clients like LUTs, who call TestPlatform
Expand All @@ -258,6 +264,10 @@ public int LaunchCustomHost(TestProcessStartInfo testProcessStartInfo)
// Even if TP can abort the API somehow, TP is essentially putting IDEs or Clients in inconsistent state without having info on
// Since the IDEs own user-UI-experience here, TP will let the custom host launch as much time as IDEs define it for their users
waitHandle.WaitOne();

cancellationTokenRegistration.Dispose();
cancellationToken.ThrowIfCancellationRequested();

this.onAckMessageReceived = null;

var ackPayload = this.dataSerializer.DeserializePayload<CustomHostLaunchAckPayload>(ackMessage);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ namespace Microsoft.VisualStudio.TestPlatform.Client.DesignMode
{
using Microsoft.VisualStudio.TestPlatform.ObjectModel;
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.Interfaces;
using System.Threading;
Copy link
Contributor

Choose a reason for hiding this comment

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

Threading [](start = 17, length = 9)

nitpick: sort using..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


/// <summary>
/// DesignMode TestHost Launcher for hosting of test process
Expand All @@ -28,7 +29,13 @@ public DesignModeTestHostLauncher(IDesignModeClient designModeClient)
/// <inheritdoc/>
public int LaunchTestHost(TestProcessStartInfo defaultTestHostStartInfo)
{
return this.designModeClient.LaunchCustomHost(defaultTestHostStartInfo);
return this.designModeClient.LaunchCustomHost(defaultTestHostStartInfo, CancellationToken.None);
}

/// <inheritdoc/>
public int LaunchTestHost(TestProcessStartInfo defaultTestHostStartInfo, CancellationToken cancellationToken)
{
return this.designModeClient.LaunchCustomHost(defaultTestHostStartInfo, cancellationToken);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ namespace Microsoft.VisualStudio.TestPlatform.Client.DesignMode
using Microsoft.VisualStudio.TestPlatform.Client.RequestHelper;
using Microsoft.VisualStudio.TestPlatform.ObjectModel;
using System;
using System.Threading;

/// <summary>
/// The interface for design mode client.
Expand All @@ -28,7 +29,9 @@ public interface IDesignModeClient : IDisposable
/// Send a custom host launch message to IDE
/// </summary>
/// <param name="defaultTestHostStartInfo">Default TestHost Start Info</param>
int LaunchCustomHost(TestProcessStartInfo defaultTestHostStartInfo);
/// <param name="cancellationToken">The cancellation Token.</param>
/// <returns>Process id of the launched test host.</returns>
int LaunchCustomHost(TestProcessStartInfo defaultTestHostStartInfo, CancellationToken cancellationToken);

/// <summary>
/// Handles parent process exit
Expand Down
14 changes: 12 additions & 2 deletions src/Microsoft.TestPlatform.Client/Execution/TestRunRequest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,16 @@ public class TestRunRequest : ITestRunRequest, ITestRunEventsHandler
/// </summary>
private object syncObject = new Object();

/// <summary>
/// Sync object for cancel operation
/// </summary>
private object cancelSyncObject = new Object();

/// <summary>
/// Sync object for abort operation
/// </summary>
private object abortSyncObject = new Object();

/// <summary>
/// The run completion event which will be signalled on completion of test run.
/// </summary>
Expand Down Expand Up @@ -236,7 +246,7 @@ public void CancelAsync()
{
EqtTrace.Verbose("TestRunRequest.CancelAsync: Canceling.");

lock (this.syncObject)
lock (this.cancelSyncObject)
{
if (this.disposed)
{
Expand Down Expand Up @@ -265,7 +275,7 @@ public void Abort()
{
EqtTrace.Verbose("TestRunRequest.Abort: Aborting.");

lock (this.syncObject)
lock (this.abortSyncObject)
Copy link
Contributor

@cltshivash cltshivash Apr 16, 2018

Choose a reason for hiding this comment

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

abortSyncObject [](start = 23, length = 15)

isn't the usage of the sync object to synchronize the access to the executionmanager ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All execution related requests, like ExecuteAsync, HandleTestRunComplete, HandleTestRunStatsChange, HandleLogMessage are under same sync object.

But we want to respect cancel, abort and don't want to wait if some of the above request are going on. Thus separate lock objects for them

Copy link
Contributor

Choose a reason for hiding this comment

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

Potentially reuse the same object for cancel and abort then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

{
if (this.disposed)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ public Collection<AttachmentSet> SendAfterTestRunStartAndGetResult(ITestMessageE

// Cycle through the messages that the datacollector sends.
// Currently each of the operations are not separate tasks since they should not each take much time. This is just a notification.
while (!isDataCollectionComplete)
while (!isDataCollectionComplete && !isCancelled)
Copy link
Contributor

Choose a reason for hiding this comment

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

isCancelled [](start = 49, length = 11)

won't we fail to receive data (testruncomplete )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case of cancellation and abort, we dont get any attachments from data collectors and simply cancels the run. So adding isCancelled will do the same thing. For non-cancelled run, we will be able to successfully able to receive data

{
var message = this.communicationManager.ReceiveMessage();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ namespace Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.Interfaces
{
using System;
using System.Collections.Generic;
using System.Threading;

using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.ObjectModel;
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Client;
Expand Down Expand Up @@ -32,6 +33,14 @@ public interface ITestRequestSender : IDisposable
/// <returns>True, if Handler is connected</returns>
bool WaitForRequestHandlerConnection(int connectionTimeout);

/// <summary>
/// Waits for Request Handler to be connected
/// </summary>
/// <param name="connectionTimeout">Time to wait for connection</param>
/// <param name="cancellationToken">Cancellation token</param>
/// <returns>True, if Handler is connected</returns>
bool WaitForRequestHandlerConnection(int connectionTimeout, CancellationToken cancellationToken);

/// <summary>
/// Close the Sender
/// </summary>
Expand Down Expand Up @@ -63,13 +72,29 @@ public interface ITestRequestSender : IDisposable
/// <param name="eventHandler">EventHandler for test run events</param>
void StartTestRun(TestRunCriteriaWithSources runCriteria, ITestRunEventsHandler eventHandler);

/// <summary>
/// Starts the TestRun with given sources and criteria
/// </summary>
/// <param name="runCriteria">RunCriteria for test run</param>
/// <param name="eventHandler">EventHandler for test run events</param>
/// <param name="cancellationToken">Cancellation token</param>
void StartTestRun(TestRunCriteriaWithSources runCriteria, ITestRunEventsHandler eventHandler, CancellationToken cancellationToken);

/// <summary>
/// Starts the TestRun with given test cases and criteria
/// </summary>
/// <param name="runCriteria">RunCriteria for test run</param>
/// <param name="eventHandler">EventHandler for test run events</param>
void StartTestRun(TestRunCriteriaWithTests runCriteria, ITestRunEventsHandler eventHandler);

/// <summary>
/// Starts the TestRun with given test cases and criteria
/// </summary>
/// <param name="runCriteria">RunCriteria for test run</param>
/// <param name="eventHandler">EventHandler for test run events</param>
/// <param name="cancellationToken">Cancellation token</param>
void StartTestRun(TestRunCriteriaWithTests runCriteria, ITestRunEventsHandler eventHandler, 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.

StartTestRun [](start = 13, length = 12)

same question.. is this a public interface ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Modified existing methods.


/// <summary>
/// Ends the Session
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,12 +137,22 @@ public int InitializeCommunication()
/// <inheritdoc />
public bool WaitForRequestHandlerConnection(int connectionTimeout)
{
return this.WaitForRequestHandlerConnection(connectionTimeout, CancellationToken.None);
}

/// <inheritdoc />
public bool WaitForRequestHandlerConnection(int connectionTimeout, CancellationToken cancellationToken)
{
var cancellationTokenRegistration = cancellationToken.Register(() => this.connected.Set());
if (EqtTrace.IsVerboseEnabled)
{
EqtTrace.Verbose("TestRequestSender.WaitForRequestHandlerConnection: waiting for connection with timeout: {0}", connectionTimeout);
}

return this.connected.Wait(connectionTimeout);
var waitSuccess = this.connected.Wait(connectionTimeout) && !cancellationToken.IsCancellationRequested;
cancellationTokenRegistration.Dispose();

return waitSuccess;
}

/// <inheritdoc />
Expand Down Expand Up @@ -260,13 +270,19 @@ public void InitializeExecution(IEnumerable<string> pathToAdditionalExtensions)

/// <inheritdoc />
public void StartTestRun(TestRunCriteriaWithSources runCriteria, ITestRunEventsHandler eventHandler)
Copy link
Contributor

Choose a reason for hiding this comment

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

StartTestRun [](start = 20, length = 12)

why do we need two signatures..? TP is an internal interface..?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

{
this.StartTestRun(runCriteria, eventHandler, CancellationToken.None);
}

/// <inheritdoc />
public void StartTestRun(TestRunCriteriaWithSources runCriteria, ITestRunEventsHandler eventHandler, CancellationToken cancellationToken)
{
this.messageEventHandler = eventHandler;
this.onDisconnected = (disconnectedEventArgs) =>
{
this.OnTestRunAbort(eventHandler, disconnectedEventArgs.Error, true);
this.OnTestRunAbort(eventHandler, disconnectedEventArgs.Error, true, cancellationToken);
};
this.onMessageReceived = (sender, args) => this.OnExecutionMessageReceived(sender, args, eventHandler);
this.onMessageReceived = (sender, args) => this.OnExecutionMessageReceived(sender, args, eventHandler, cancellationToken);
this.channel.MessageReceived += this.onMessageReceived;

var message = this.dataSerializer.SerializePayload(
Expand All @@ -284,13 +300,19 @@ public void StartTestRun(TestRunCriteriaWithSources runCriteria, ITestRunEventsH

/// <inheritdoc />
public void StartTestRun(TestRunCriteriaWithTests runCriteria, ITestRunEventsHandler eventHandler)
{
this.StartTestRun(runCriteria, eventHandler, CancellationToken.None);
}

/// <inheritdoc />
public void StartTestRun(TestRunCriteriaWithTests runCriteria, ITestRunEventsHandler eventHandler, CancellationToken cancellationToken)
{
this.messageEventHandler = eventHandler;
this.onDisconnected = (disconnectedEventArgs) =>
{
this.OnTestRunAbort(eventHandler, disconnectedEventArgs.Error, true);
this.OnTestRunAbort(eventHandler, disconnectedEventArgs.Error, true, cancellationToken);
};
this.onMessageReceived = (sender, args) => this.OnExecutionMessageReceived(sender, args, eventHandler);
this.onMessageReceived = (sender, args) => this.OnExecutionMessageReceived(sender, args, eventHandler, cancellationToken);
this.channel.MessageReceived += this.onMessageReceived;

var message = this.dataSerializer.SerializePayload(
Expand Down Expand Up @@ -379,7 +401,7 @@ public void Dispose()
this.communicationEndpoint.Stop();
}

private void OnExecutionMessageReceived(object sender, MessageReceivedEventArgs messageReceived, ITestRunEventsHandler testRunEventsHandler)
private void OnExecutionMessageReceived(object sender, MessageReceivedEventArgs messageReceived, ITestRunEventsHandler testRunEventsHandler, CancellationToken cancellationToken)
{
try
{
Expand Down Expand Up @@ -434,7 +456,7 @@ private void OnExecutionMessageReceived(object sender, MessageReceivedEventArgs
}
catch (Exception exception)
{
this.OnTestRunAbort(testRunEventsHandler, exception, false);
this.OnTestRunAbort(testRunEventsHandler, exception, false, cancellationToken);
}
}

Expand Down Expand Up @@ -485,7 +507,7 @@ private void OnDiscoveryMessageReceived(ITestDiscoveryEventsHandler2 discoveryEv
}
}

private void OnTestRunAbort(ITestRunEventsHandler testRunEventsHandler, Exception exception, bool getClientError)
private void OnTestRunAbort(ITestRunEventsHandler testRunEventsHandler, Exception exception, bool getClientError, CancellationToken cancellationToken)
{
if (this.IsOperationComplete())
{
Expand All @@ -496,7 +518,7 @@ private void OnTestRunAbort(ITestRunEventsHandler testRunEventsHandler, Exceptio
EqtTrace.Verbose("TestRequestSender: OnTestRunAbort: Set operation complete.");
this.SetOperationComplete();

var reason = this.GetAbortErrorMessage(exception, getClientError);
var reason = this.GetAbortErrorMessage(exception, getClientError, cancellationToken);
EqtTrace.Error("TestRequestSender: Aborting test run because {0}", reason);
this.LogErrorMessage(string.Format(CommonResources.AbortedTestRun, reason));

Expand All @@ -522,7 +544,7 @@ private void OnDiscoveryAbort(ITestDiscoveryEventsHandler2 eventHandler, Excepti
this.SetOperationComplete();

var discoveryCompleteEventArgs = new DiscoveryCompleteEventArgs(-1, true);
var reason = this.GetAbortErrorMessage(exception, getClientError);
var reason = this.GetAbortErrorMessage(exception, getClientError, CancellationToken.None);
EqtTrace.Error("TestRequestSender: Aborting test discovery because {0}", reason);
this.LogErrorMessage(string.Format(CommonResources.AbortedTestDiscovery, reason));

Expand All @@ -540,8 +562,9 @@ private void OnDiscoveryAbort(ITestDiscoveryEventsHandler2 eventHandler, Excepti
eventHandler.HandleDiscoveryComplete(discoveryCompleteEventArgs, null);
}

private string GetAbortErrorMessage(Exception exception, bool getClientError)
private string GetAbortErrorMessage(Exception exception, bool getClientError, CancellationToken cancellationToken)
{
var cancellationTokenRegistration = cancellationToken.Register(() => this.clientExited.Set());
EqtTrace.Verbose("TestRequestSender: GetAbortErrorMessage: Exception: " + exception);

// It is also possible for an operation to abort even if client has not
Expand All @@ -554,15 +577,18 @@ private string GetAbortErrorMessage(Exception exception, bool getClientError)

// Set a default message and wait for test host to exit for a moment
reason = CommonResources.UnableToCommunicateToTestHost;
if (this.clientExited.Wait(this.clientExitedWaitTime))
if (this.clientExited.Wait(this.clientExitedWaitTime) && !cancellationToken.IsCancellationRequested)
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, should this be the first condition then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. If cancellation flag is checked first and it returns false, and then while waiting cancellation occurs, then waitHandle will be set because of cancellation and thus enter in if condition (which is wrong as wait is set by cancel)

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking the other way round where we actually end up waiting for a while, figure out its cancelled and head to the else part which seems like a wasted wait...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are already doing that in this PR. Earlier I did a CancellationToken.Register(() => this.clientExited) which I latter changed to this.clientExited.Wait(this.clientExitedWaitTime, CancellationToken) on your recommendation. In either case, clientExited will return immediately if cancellation happened. Let me know if i am understanding it incorrectly.

{
EqtTrace.Info("TestRequestSender: GetAbortErrorMessage: Received test host error message.");
reason = this.clientExitErrorMessage;
}

EqtTrace.Info("TestRequestSender: GetAbortErrorMessage: Timed out waiting for test host error message.");
else
{
EqtTrace.Info("TestRequestSender: GetAbortErrorMessage: Timed out waiting for test host error message.");
}
}

cancellationTokenRegistration.Dispose();
return reason;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ public class ProxyDiscoveryManager : ProxyOperationManager, IProxyDiscoveryManag
{
private readonly ITestRuntimeProvider testHostManager;
private IDataSerializer dataSerializer;
private CancellationTokenSource cancellationTokenSource;
private bool isCommunicationEstablished;
private IRequestData requestData;
private ITestDiscoveryEventsHandler2 baseTestDiscoveryEventsHandler;
Expand Down Expand Up @@ -71,7 +70,6 @@ internal ProxyDiscoveryManager(
{
this.dataSerializer = dataSerializer;
this.testHostManager = testHostManager;
this.cancellationTokenSource = new CancellationTokenSource();
this.isCommunicationEstablished = false;
this.requestData = requestData;
}
Expand Down Expand Up @@ -99,7 +97,7 @@ public void DiscoverTests(DiscoveryCriteria discoveryCriteria, ITestDiscoveryEve
this.baseTestDiscoveryEventsHandler = eventHandler;
try
{
this.isCommunicationEstablished = this.SetupChannel(discoveryCriteria.Sources, this.cancellationTokenSource.Token);
this.isCommunicationEstablished = this.SetupChannel(discoveryCriteria.Sources);

if (this.isCommunicationEstablished)
{
Expand Down
Loading