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

CustomHost Launch Timeout fix #265

Merged
merged 16 commits into from
Dec 14, 2016
Merged

CustomHost Launch Timeout fix #265

merged 16 commits into from
Dec 14, 2016

Conversation

saikrishnav
Copy link
Member

CustomHost Launch Timeout fix

// Even if TP has a timeout here, there is no way TP can abort or stop the thread/task that is hung in IDE or LUT
// 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();
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of infinite wait, should we provide the caller an ability to pass in a timeout?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also what is the way for IDEs to cancel or terminate the infinite wait?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since IDE's are driving the test platform - would prefer a way for IDEs to cancel this operation instead. Just in case the process the IDE has launched has crashed before it could connect to the test platform, it would be good to have the IDEs notify the test platform so it can abort this request and process more.

Copy link
Contributor

@codito codito Dec 13, 2016

Choose a reason for hiding this comment

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

Closure: we decided to ensure that IDE callback to launch test host (in vstestconsolewrapper) always returns a pid. This will guarantee that a stale thread will never lie around in case testhost launch fails.

@saikrishnav will probably update this PR with the change.

Copy link
Contributor

Choose a reason for hiding this comment

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

@codito : We should document this in the translation layer wiki we were planning.

@harshjain2
Copy link
Contributor

@saikrishnav @codito Suggestion: Seems like the documentation for this class is auto-generated. Can we improve it a bit?
/// <summary> /// The design mode client. /// </summary>

@codito
Copy link
Contributor

codito commented Dec 10, 2016

@harshjain2 definitely. #237 tracks it as well.

testRequestManager.ResetOptions();
try
{
testRequestManager.ResetOptions();
Copy link
Contributor

Choose a reason for hiding this comment

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

We need new tests for these?

@saikrishnav saikrishnav merged commit 74f6557 into microsoft:master Dec 14, 2016
@saikrishnav saikrishnav deleted the customhosttimeoutfix branch December 14, 2016 05:45
@codito codito added this to the Preview 111.1 - 20161217 milestone Dec 19, 2016
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