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
Show file tree
Hide file tree
Changes from 5 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
12 changes: 10 additions & 2 deletions src/Microsoft.TestPlatform.Client/DesignMode/DesignModeClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public class DesignModeClient : IDesignModeClient

private readonly IDataSerializer dataSerializer;

private Action<Message> onAckMessageReceived;
protected Action<Message> onAckMessageReceived;

private object ackLockObject = new object();

Expand Down Expand Up @@ -178,11 +178,13 @@ private void ProcessRequests(ITestRequestManager testRequestManager)
testRequestManager.AbortTestRun();
break;
}

case MessageType.CustomTestHostLaunchCallback:
{
this.onAckMessageReceived?.Invoke(message);
break;
}

case MessageType.SessionEnd:
{
EqtTrace.Info("DesignModeClient: Session End message received from server. Closing the connection.");
Expand Down Expand Up @@ -227,9 +229,15 @@ public int LaunchCustomHost(TestProcessStartInfo testProcessStartInfo)
};

this.communicationManager.SendMessage(MessageType.CustomTestHostLaunch, testProcessStartInfo);
waitHandle.WaitOne(ClientListenTimeOut);

// LifeCycle of the TP through DesignModeClient is maintained by the IDEs or user-facing-clients like LUTs, who call TestPlatform
// TP is handing over the control of launch to these IDEs and so, TP has to wait indefinite
// 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.

this.onAckMessageReceived = null;

return this.dataSerializer.DeserializePayload<int>(ackMessage);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ namespace Microsoft.VisualStudio.TestPlatform.Client.UnitTests.DesignMode
using Moq;

using Newtonsoft.Json.Linq;
using System.Threading.Tasks;

[TestClass]
public class DesignModeClientTests
Expand Down Expand Up @@ -222,5 +223,42 @@ public void DesignModeClientShouldStopCommunicationOnParentProcessExit()

this.mockCommunicationManager.Verify(cm => cm.StopClient(), Times.Once);
}

[TestMethod]
public void DesignModeClientLaunchCustomHostMustReturnIfAckComes()
{
var testableDesignModeClient = new TestableDesignModeClient(this.mockCommunicationManager.Object, JsonDataSerializer.Instance);

this.mockCommunicationManager.Setup(cm => cm.WaitForServerConnection(It.IsAny<int>())).Returns(true);

var expectedProcessId = 1234;
Action sendMessageAction = () =>
{
testableDesignModeClient.InvokeCustomHostLaunchAckCallback(expectedProcessId);
};

this.mockCommunicationManager.Setup(cm => cm.SendMessage(MessageType.CustomTestHostLaunch, It.IsAny<object>())).
Callback(() => Task.Run(sendMessageAction));

var info = new TestProcessStartInfo();
var processId = testableDesignModeClient.LaunchCustomHost(info);

Assert.AreEqual(expectedProcessId, processId);
}

private class TestableDesignModeClient : DesignModeClient
{
internal TestableDesignModeClient(ICommunicationManager communicationManager,
IDataSerializer dataSerializer)
: base(communicationManager, dataSerializer)
{
}

public void InvokeCustomHostLaunchAckCallback(int processId)
{
this.onAckMessageReceived?.Invoke(
new Message() { MessageType = MessageType.CustomTestHostLaunchCallback, Payload = JToken.FromObject(processId) });
}
}
}
}