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

abhishkk · 2018-04-16T03:47:36Z

Issue:
#1540

Changes done:

If cancellation/abort happens before custom host launch, TestPlatform will cancel waiting for IDE. In this case, IDE will get an error level message that Could not connect to test host process.
Cancel and abort request will not keep on waiting on same lock as of execution request, so as to respect cancel/abort quickly.
On cancellation, data collector will not send any attachments (that happens today also) and quickly cancel.
Methods are updated in ITestRequestSender API so that we can cancel waiting for test host to get connected to vstest.console.
With every request sent by console to test host, now has a pre-check if cancellation requested or not.
AfterTestRunEnd request to data collectors will be called only once when test run completes. (Currently in case of cancellation, we were sending AfterTestRunEnd 2 times, once just as soon as cancellation comes and second when test run completes.)
ITestHostLauncher API now has one more overriden LaunchTestHost method which takes CancellationToken. Any custom runtime provider needs to call this overriden method to respect cancellation request.
TestRequestManager is no longer holding cancel and abort request. Till now they were holding cancel/abort request for five seconds in case execution request is active.

@AbhitejJohn Please verify IDE side changes considering above changes. I have done validations from my end. Let me know if there are any issues.

@mayankbansal018 Regarding # 7, do we need make any changes in UWP runtime provider to respect cancellation?

…ication

abhishkk · 2018-04-16T03:50:32Z

src/Microsoft.TestPlatform.TestHostProvider/Hosting/DefaultTestHostManager.cs

@@ -383,7 +383,7 @@ private bool LaunchHost(TestProcessStartInfo testHostStartInfo, CancellationToke
                }
                else
                {
-                    int processId = this.customTestHostLauncher.LaunchTestHost(testHostStartInfo);
+                    int processId = this.customTestHostLauncher.LaunchTestHost(testHostStartInfo, cancellationToken);


We are breaking the API here. Is customTestHostLauncher used by others also? Interface ITesHostLauncher is in ObjectModel.
If its used by others, we need to introduce something called IsCancellable in ITestHostLauncher which will be used here to switch between overriden calls of customTestHostLauncher.LaunchTestHost.

will it be used in UWP sceanrio ..?

In reply to: 181616862 [](ancestors = 181616862)

I confirmed from Mayank. It will not be breaking change.

cltshivash · 2018-04-16T07:34:42Z

src/Microsoft.TestPlatform.Client/DesignMode/DesignModeTestHostLauncher.cs

@@ -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;


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

nitpick: sort using..

cltshivash · 2018-04-16T07:36:31Z

src/Microsoft.TestPlatform.Client/Execution/TestRunRequest.cs

@@ -265,7 +275,7 @@ public void Abort()
        {
            EqtTrace.Verbose("TestRunRequest.Abort: Aborting.");

-            lock (this.syncObject)
+            lock (this.abortSyncObject)


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

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

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

Potentially reuse the same object for cancel and abort then?

cltshivash · 2018-04-16T07:37:41Z

src/Microsoft.TestPlatform.CommunicationUtilities/DataCollectionRequestSender.cs

@@ -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)


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

won't we fail to receive data (testruncomplete )

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

cltshivash · 2018-04-16T07:38:43Z

src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs

@@ -260,13 +270,19 @@ public void InitializeExecution(IEnumerable<string> pathToAdditionalExtensions)

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


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

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

cltshivash · 2018-04-16T07:39:14Z

src/Microsoft.TestPlatform.CommunicationUtilities/Interfaces/ITestRequestSender.cs

+        /// <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);


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

same question.. is this a public interface ?

Done. Modified existing methods.

cltshivash

🕐

mayankbansal018 · 2018-04-18T09:29:41Z

src/Microsoft.TestPlatform.TestHostProvider/Hosting/DefaultTestHostManager.cs

@@ -383,7 +383,7 @@ private bool LaunchHost(TestProcessStartInfo testHostStartInfo, CancellationToke
                }
                else
                {
-                    int processId = this.customTestHostLauncher.LaunchTestHost(testHostStartInfo);
+                    int processId = this.customTestHostLauncher.LaunchTestHost(testHostStartInfo, cancellationToken);


Add this is dotnet runtime

mayankbansal018

AbhitejJohn · 2018-04-19T07:13:51Z

src/Microsoft.TestPlatform.Client/DesignMode/DesignModeClient.cs

@@ -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());


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

AbhitejJohn · 2018-04-19T07:22:05Z

src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs

@@ -554,15 +559,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)


hmm, should this be the first condition then?

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)

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...

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.

AbhitejJohn · 2018-04-19T07:28:14Z

src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyOperationManager.cs

@@ -184,6 +189,24 @@ public virtual bool SetupChannel(IEnumerable<string> sources, CancellationToken
            return true;
        }

+        private bool LaunchTestHostAsync(TestProcessStartInfo testHostStartInfo, CancellationToken token)
+        {
+            this.CancellationTokenSource.Token.ThrowIfCancellationRequested();


should this be called out in the protocol doc? Looks like a cancel is going to be an abrupt stop to a request no matter where in the sequence we are currently.

We are cancelling abruptly only before the test execution is actually started and then sending HandleTestRunComplete.
Once test execution is started by test host, we send cancel request to test host and which passes the cancel request to adapters.
@cltshivash @singhsarab Should this be documented?

yup, got that but most clients would not expect a TestRunComplete when negotiating protocol unless told otherwise.... Was a separate message for cancellation considered for scenarios like these? Might be a bigger change but worth a fluid protocol.

TestRequestManager today waits for completion in all scenarios. I will check if this is already documented and if not, will discuss with the team and document it.

AbhitejJohn · 2018-04-19T08:35:52Z

src/Microsoft.TestPlatform.ObjectModel/Client/Interfaces/ITestHostLauncher.cs

+        /// <param name="defaultTestHostStartInfo">Default TestHost Process Info</param>
+        /// <param name="cancellationToken">The cancellation Token.</param>
+        /// <returns>Process id of the launched test host</returns>
+        int LaunchTestHost(TestProcessStartInfo defaultTestHostStartInfo, CancellationToken cancellationToken);


Will require a corresponding change in TPV1 OM. TE implements this for debugging which would need a change as well.

This interface is used by run time provider only for launching the custom host via custom host launcher.
Why changes are required in TPV1 OM?

Even though if TE uses it as an interface for their internal purpose, there will no breaking change here (as TPv1 OM is used both while build and run time. TPV2 OM is not loaded.) Please correct me if i am missing something here.

Fare, but shouldn't we be consistent? Bare in mind that the Console wrapper and interfaces assemblies are loaded in VS and this could result in inadvertent runtime failures.

@cltshivash @singhsarab @AbhitejJohn
Am I missing something here? AFIAK ITestHostLauncher is meant only for runtime provider and it is loaded in vstest console only. Neither translation layer, nor test host or any other process from TP side uses this except vstest.console.

If TE is using this interface for their internal purpose, then it is using TPV1 OM and loading TPV1 OM. So how the behavior is inconsistent? I don't feel comfortable changing TPV1 OM unless it's a breaking change.

mayankbansal018 · 2018-04-19T11:46:08Z

Do validate that UWP does work with these changes

…slation layer.

…into cancelNotHonored

abhishkk added 4 commits April 14, 2018 11:59

Initial commit

1f41e27

Cancellation in custom host launcher, test host, while ongoing commun…

1b09512

…ication

passing cancellation to data collectors.

60b74d8

build issues fix

51bcc10

abhishkk requested review from acesiddhu, smadala, cltshivash, singhsarab and mayankbansal018 April 16, 2018 03:47

abhishkk commented Apr 16, 2018

View reviewed changes

cltshivash reviewed Apr 16, 2018

View reviewed changes

cltshivash suggested changes Apr 16, 2018

View reviewed changes

review comments

c3dc452

abhishkk requested a review from AbhitejJohn April 18, 2018 03:38

Unit tests

7674f35

abhishkk self-assigned this Apr 18, 2018

mayankbansal018 reviewed Apr 18, 2018

View reviewed changes

mayankbansal018 approved these changes Apr 18, 2018

View reviewed changes

AbhitejJohn reviewed Apr 19, 2018

View reviewed changes

abhishkk added 4 commits April 19, 2018 15:56

Merge branch 'master' into cancelNotHonored

eb079c6

review comments

046c86a

resolve conflict

e206964

Merge branch 'master' into cancelNotHonored

653047a

Passing operation canceled instead of test platform exception to tran…

3015b3e

…slation layer.

cltshivash approved these changes Apr 20, 2018

View reviewed changes

abhishkk added 9 commits April 20, 2018 20:00

review comments

4513b14

review comments

3a77114

review comments

07f8abd

review comments

b11ac54

Merge branch 'master' into cancelNotHonored

145ef38

unit tests fix

93923a2

Merge branch 'cancelNotHonored' of https://github.com/abhishkk/vstest …

10fa006

…into cancelNotHonored

unit tests fail

24bd933

unit tests

08975c6

abhishkk merged commit a8b1e78 into microsoft:master Apr 23, 2018

singhsarab mentioned this pull request Jun 6, 2018

[C#][.Net Core][UT&Desktop] Cancel "Run All" failed and throw 2 errors #1568

Closed

		@@ -260,13 +270,19 @@ public void InitializeExecution(IEnumerable<string> pathToAdditionalExtensions)

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

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

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

Conversation

abhishkk commented Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cltshivash Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cltshivash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayankbansal018 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhishkk Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

mayankbansal018 commented Apr 19, 2018

abhishkk commented Apr 16, 2018 •

edited

Loading

cltshivash Apr 16, 2018 •

edited

Loading

abhishkk Apr 20, 2018 •

edited

Loading