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

Cancellation timing issue fix. #1398

Merged
merged 4 commits into from
Jan 31, 2018

Conversation

AbhitejJohn
Copy link
Contributor

Fix for a timing issue where a cancel request before test run request has been started is being ignored.

We have the right plumbing in place but just seem to have missed initializing a var. Put that in place.
All interface changes are internal only.

Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/559064

From testhost diag logs, this is the stack trace:
TpTrace Error: 0 : 17096, 8, 2018/01/30, 15:50:44.560, 1284210642450, testhost.x86.exe, LengthPrefixCommunicationChannel: MessageReceived: Exception occurred while calling handler of type Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.TestRequestHandler for MessageReceivedEventArgs: System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.NullReferenceException: Object reference not set to an instance of an object.
at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Execution.ExecutionManager.Cancel()
at Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.TestRequestHandler.OnMessageReceived(Object sender, MessageReceivedEventArgs messageReceivedArgs)
--- End of inner exception stack trace ---
at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor)
at System.Reflection.RuntimeMethodInfo.UnsafeInvokeInternal(Object obj, Object[] parameters, Object[] arguments)
at System.Delegate.DynamicInvokeImpl(Object[] args)
at Microsoft.VisualStudio.TestPlatform.Utilities.MulticastDelegateUtilities.SafeInvoke(Delegate delegates, Object sender, EventArgs args, String traceDisplayName)

… has been started is being ignored.

We have the right plumbing in place but just seem to have missed initializing a var. Put that in place.
All interface changes are internal only.
@singhsarab singhsarab changed the title Cancallation timing issue fix. Cancellation timing issue fix. Jan 31, 2018
@mayankbansal018
Copy link
Contributor

If the cancel request is coming before run request, then is it possible that IDE is doing something wrong in initiating these request?

{
if (this.activeTestRun == null)
{
var testRunCompleteEventArgs = new TestRunCompleteEventArgs(null, true, false, null, null, TimeSpan.Zero);
this.testRunEventsHandler.HandleTestRunComplete(testRunCompleteEventArgs, null, null, null);
testRunEventsHandler.HandleTestRunComplete(testRunCompleteEventArgs, null, null, null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Who was handling NRE? Why there is no report to vstest.console/crash?

Copy link
Contributor

@smadala smadala left a comment

Choose a reason for hiding this comment

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

Looks like this fixes the issue introduced in 15.6 but not the customer issue. @singhsarab will confirm it.

@singhsarab singhsarab merged commit f471cc4 into microsoft:rel/15.6 Jan 31, 2018
singhsarab pushed a commit to singhsarab/vstest that referenced this pull request Feb 19, 2018
* Fix for a timing issue where a cancel request before test run request has been started is being ignored.
We have the right plumbing in place but just seem to have missed initializing a var. Put that in place.
All interface changes are internal only.
singhsarab added a commit that referenced this pull request Feb 19, 2018
* Fix for a timing issue where a cancel request before test run request has been started is being ignored.
We have the right plumbing in place but just seem to have missed initializing a var. Put that in place.
All interface changes are internal only.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants