Event based communication over sockets. #1294

navin22 · 2017-11-13T10:59:18Z

Description

Using Event based communication over sockets using LengthPrefix communication channel.
Channel will now notify when the message is received.
Socket client and server and use one thread each, instead of using new thread at every read.
Related tests and added additional tests.
ToDo : Only main flow is converted to the socket client and server. Other flows like DataCollection is not on this.

…d implementation). Update test request handler to use the new socket implementation.

# Conflicts: # src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs # src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender2.cs # test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/TestRequestSenderTests.cs # test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/TestRequestSenderTests2.cs

# Conflicts: # src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs # src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender2.cs # test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/TestRequestSenderTests.cs # test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/TestRequestSenderTests2.cs Added debug launches to debug tests.

mayankbansal018

not taken a look at tests yet, will do it later

mayankbansal018 · 2017-11-15T05:55:07Z

src/Microsoft.TestPlatform.CommunicationUtilities/SocketClient.cs


        /// <inheritdoc />
-        public void Start(string connectionInfo)
+        // public void Start(string connectionInfo)


nit: remove this

mayankbansal018 · 2017-11-15T05:59:03Z

src/Microsoft.TestPlatform.CommunicationUtilities/SocketClient.cs

+            // {
+            //    throw connectAsyncTask.Exception;
+            // }
+            if (this.Connected != null)


Can Connected ever be null in this class?

It's an event so it can be null. But in our case it will not be null.

mayankbansal018 · 2017-11-15T06:04:16Z

src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs

        {
+            if (this.connectionInfo.Role == ConnectionRole.Client)


The connectioninfo here is that of the RuntimeProvider, which tells how the testhost will act, so we should reverse the role the of vstest.console right?

Yeah, it happens in another constructor. Ln 92-94.

mayankbansal018 · 2017-11-15T06:18:28Z

src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelProxyExecutionManager.cs

-            // In case of DataCollection we only start dc.exe on initialize, & close once the TestRun is complete,
-            // So instead of resuing ProxyExecutionManager, we will close it here, & create new PEMWDC
-            // In Case of Abort, clean old one and create new proxyExecutionManager in place of old one.
-            if (!this.SharedHosts || testRunCompleteArgs.IsAborted || (proxyExecutionManager is ProxyExecutionManagerWithDataCollection))


Why did we remove these checks? were they bogus?

Earlier they were needed only for those scenarios, and in other scenarios they were reusing the execution manager. But since the test host dies it's not good to have that around. and was observing an exception, which tries to send message over a channel that's already closed.

mayankbansal018 · 2017-11-15T09:28:45Z

src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs

        {
+            if (this.connectionInfo.Role == ConnectionRole.Client)


Please put a comment to move this to Communication factory, which returns ICommEndPoint based on Role, & ConnectionType (Socket, NamedPipe etc..) Once another communication mechanism is implemented

navin22 · 2017-11-15T14:20:20Z

@dotnet-bot test Windows_NT / Release Build

navin22 · 2017-11-15T14:51:51Z

@dotnet-bot test

smadala · 2017-11-16T06:14:53Z

src/Microsoft.TestPlatform.Common/Interfaces/Communication/ConnectedEventArgs.cs

@@ -24,11 +24,28 @@ public ConnectedEventArgs()
        public ConnectedEventArgs(ICommunicationChannel channel)
        {
            this.Channel = channel;
+            this.Connected = true;


Why is communicationutilities namespace in common assembly?

Moved the files.

smadala · 2017-11-16T06:16:36Z

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

+{
+    using System;
+
+    public interface ICommunicationEndPoint


Can be put it in vstest-docs, once refined.

smadala · 2017-11-16T06:40:45Z

src/Microsoft.TestPlatform.CommunicationUtilities/LengthPrefixCommunicationChannel.cs

@@ -57,10 +57,7 @@ public Task NotifyDataAvailable()
            // connection is closed.
            var data = this.reader.ReadString();


ReadString can throw the exception.

It's caught in MessageLoopAsync method.

smadala · 2017-11-16T06:50:58Z

src/Microsoft.TestPlatform.CommunicationUtilities/SocketServer.cs

@@ -17,7 +17,7 @@ namespace Microsoft.VisualStudio.TestPlatform.CommunicationUtilities
    /// <summary>
    /// Communication server implementation over sockets.
    /// </summary>
-    public class SocketServer : ICommunicationServer
+    public class SocketServer : ICommunicationEndPoint


Delete ICommunicationServer and ICommunicationClient.

smadala · 2017-11-16T07:01:14Z

src/Microsoft.TestPlatform.CommunicationUtilities/SocketClient.cs

+            var ipEndPoint = endpoint.GetIPEndPoint();
+
+            // Don't start if the endpoint port is zero
+            this.tcpClient.ConnectAsync(ipEndPoint.Address, ipEndPoint.Port).ContinueWith(this.OnServerConnected);


Add EqtTrace here saying that "Waiting for connecting to server".

smadala · 2017-11-16T07:05:05Z

src/Microsoft.TestPlatform.CommunicationUtilities/SocketClient.cs

+
+            // Don't start if the endpoint port is zero
+            this.tcpClient.ConnectAsync(ipEndPoint.Address, ipEndPoint.Port).ContinueWith(this.OnServerConnected);
+            return endpoint;


Should return endpoint or ipEndPoint?

smadala · 2017-11-16T07:06:21Z

src/Microsoft.TestPlatform.CommunicationUtilities/SocketClient.cs


        /// <inheritdoc />
-        public void Start(string connectionInfo)
+        public string Start(string endpoint)


Nit: endPoint.

smadala · 2017-11-16T08:34:01Z

src/Microsoft.TestPlatform.CommunicationUtilities/SocketClient.cs

            {
-                this.ServerConnected.SafeInvoke(this, new ConnectedEventArgs(this.channel), "SocketClient: ServerConnected");
+                if (connectAsyncTask.IsFaulted)
+                {


Add EqtTrace on failure and success scenarios.

smadala · 2017-11-16T08:44:34Z

src/Microsoft.TestPlatform.CommunicationUtilities/SocketServer.cs

            {
                this.channel = this.channelFactory(this.tcpClient.GetStream());
-                this.ClientConnected.SafeInvoke(this, new ConnectedEventArgs(this.channel), "SocketServer: ClientConnected");
+                this.Connected.SafeInvoke(this, new ConnectedEventArgs(this.channel), "SocketServer: ClientConnected");


Add EqtTrace on failure and success scenarios.

smadala · 2017-11-16T09:01:57Z

src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs

    {
-        private ICommunicationManager communicationManager;
+        // Time to wait for test host exit (in seconds)
+        private const int CLIENTPROCESSEXITWAIT = 10 * 1000;


Nit: ClientProcessExitWaitTimeout?(Change it in other places too.)
In seconds?

smadala · 2017-11-16T09:14:36Z

src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs

+                };
+            this.communicationEndpoint.Disconnected += (sender, args) =>
+                {
+                    // If there's an disconnected event handler, call it


On disconnect should set this.connected.Set()?

smadala · 2017-11-16T09:27:41Z

src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs

+                else if (message.MessageType == MessageType.TestMessage)
+                {
+                    // There are some test hosts which sends log messages, hence logging and ignoring.
+                    var testMessage = this.dataSerializer.DeserializePayload<TestMessagePayload>(message);


Why is TestMessage expected? Should this log to client?

smadala · 2017-11-16T09:56:10Z

src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs

            {
-                throw new TestPlatformException(string.Format(CultureInfo.CurrentUICulture, CommonResources.UnexpectedMessage, MessageType.VersionCheck, message.MessageType));
+                this.channel.MessageReceived -= this.onMessageReceived;


What will happens messages sent after version check message type, Like this

Reading from binary reader only when there's a corresponding event handler.

smadala · 2017-11-16T11:13:00Z

test/Microsoft.TestPlatform.Common.UnitTests/Microsoft.TestPlatform.Common.UnitTests.csproj

@@ -10,31 +10,20 @@
    <TargetFrameworks>netcoreapp1.0;net451</TargetFrameworks>
    <AssemblyName>Microsoft.TestPlatform.Common.UnitTests</AssemblyName>
  </PropertyGroup>
-  <ItemGroup>
+  <!--<ItemGroup>


Nit: Remove comment.

smadala · 2017-11-16T11:13:36Z

...onUtilities.PlatformTests/Microsoft.TestPlatform.CommunicationUtilities.PlatformTests.csproj

@@ -7,12 +7,12 @@
  <Import Project="$(TestPlatformRoot)scripts/build/TestPlatform.Settings.targets" />
  <PropertyGroup>
    <OutputType Condition=" '$(TargetFramework)' == 'netcoreapp1.0' ">Exe</OutputType>
-    <TargetFrameworks>netcoreapp1.0;net451</TargetFrameworks>
+    <TargetFrameworks>net451;netcoreapp1.0</TargetFrameworks>


Revert the change.

smadala · 2017-11-16T11:13:52Z

...onUtilities.PlatformTests/Microsoft.TestPlatform.CommunicationUtilities.PlatformTests.csproj

    <AssemblyName>Microsoft.TestPlatform.CommunicationUtilities.PlatformTests</AssemblyName>
    <EnableCodeAnalysis>true</EnableCodeAnalysis>
  </PropertyGroup>
  <ItemGroup>
-    <ProjectReference Include="..\..\src\Microsoft.TestPlatform.CommunicationUtilities\Microsoft.TestPlatform.CommunicationUtilities.csproj" />
+    <!--<ProjectReference Include="..\..\src\Microsoft.TestPlatform.CommunicationUtilities\Microsoft.TestPlatform.CommunicationUtilities.csproj" />


Remove all the commented code.

smadala · 2017-11-16T11:15:46Z

test/Microsoft.TestPlatform.CommunicationUtilities.PlatformTests/SocketTestsBase.cs

+
+        protected IPEndPoint GetIpEndPoint(string value)
+        {
+            if (Uri.TryCreate(string.Concat("tcp://", value), UriKind.Absolute, out Uri uri))


smadala · 2017-11-16T11:16:21Z

...municationUtilities.UnitTests/Microsoft.TestPlatform.CommunicationUtilities.UnitTests.csproj

@@ -7,12 +7,12 @@
  <Import Project="$(TestPlatformRoot)scripts/build/TestPlatform.Settings.targets" />
  <PropertyGroup>
    <OutputType Condition=" '$(TargetFramework)' == 'netcoreapp1.0' ">Exe</OutputType>
-    <TargetFrameworks>netcoreapp1.0;net451</TargetFrameworks>
+    <TargetFrameworks>net451;netcoreapp1.0</TargetFrameworks>


Revert this change(And other places too).

…ventcomm

mayankbansal018 · 2017-11-17T13:10:42Z

src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs

+                {
+                    throw new TestPlatformException(string.Format(CultureInfo.CurrentUICulture, CommonResources.VersionCheckFailed));
+                }
+                else


Some other message type can also come if the communication is very fast, for e.g. version check can also send https://github.com/Microsoft/vstest/blob/a32cf559805a61cd491b5574bd3f365df8943878/src/Microsoft.TestPlatform.CrossPlatEngine/EventHandlers/TestRequestHandler.cs#L102

which is Log message, & this will throw exception.

Ideal state would be to have one EventHandler method, which handles all possible message type, & throw if some garbage message comes.
Each time we are attaching an EventHandler, we can point to same method, & then detach once it is done

added handler for message type MessageType.TestMessage

mayankbansal018 · 2017-11-17T13:33:58Z

src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs

+        private void SetOperationComplete()
+        {
+            // Complete the currently ongoing operation (Discovery/Execution)
+            Interlocked.CompareExchange(ref this.operationCompleted, 1, 0);


Why are we using a Thread safe operation, are we expecting that this can hit from multiple threads?

mayankbansal018 · 2017-11-17T14:17:08Z

src/Microsoft.TestPlatform.CrossPlatEngine/EventHandlers/TestRequestHandler.cs

                }
+                this.channel = connectedArgs.Channel;
+                this.channel.MessageReceived += this.OnMessageReceived;


As soon as we set handler to receive messages, we can get a Test Discovery/Execution request before ProcessRequest which supply testHostManagerFactory.

So move attaching to event handler in ProcessRequest

Again this is a corner scenario, but can happen

You are right, there some unit test failing randomly because of this.I am handling this in the latest one. Please take a look.

navin22 · 2017-11-17T15:21:42Z

@dotnet-bot test Windows_NT / Debug Build

navin22 · 2017-11-19T15:46:12Z

@dotnet-bot test Windows_NT / Release Build

cltshivash · 2017-11-21T08:18:26Z

src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs

+            testRunEventsHandler.HandleTestRunComplete(completeArgs, null, null, null);
+        }
+
+        private void OnDiscoveryAbort(ITestDiscoveryEventsHandler2 eventHandler, Exception exception, bool getClientError)


OnDiscoveryAbort [](start = 21, length = 16)

exception handling has been removed.. any impact due to this ?

cltshivash · 2017-11-21T08:21:28Z

src/Microsoft.TestPlatform.CrossPlatEngine/EventHandlers/TestRequestHandler.cs

                }
+                this.channel = connectedArgs.Channel;
+                this.channel.MessageReceived += this.OnMessageReceived;


MessageReceived [](start = 29, length = 15)

can the processing of a second message begin before the completion of an earlier one..? what is the impact..?

mayankbansal018 · 2017-12-07T15:03:22Z

@dotnet-bot test Windows_NT / Release Build

mayankbansal018 · 2017-12-08T05:32:52Z

@dotnet-bot test Windows_NT / Release Build

…ntcomm

Arun Mahapatra and others added 11 commits May 8, 2017 08:12

Update test request sender to use socket server and client (remove ol…

a6105f1

…d implementation). Update test request handler to use the new socket implementation.

Merge branch 'master' into request-handler

14a3693

Added few tests for test request handler.

8e985ce

Pushing changes.

48c04da

Refactoring test code.

ebf7546

Added some more tests.

8d60783

Fixed tests.

06a759d

removed unused method.

c75ea37

Merge remote-tracking branch 'upstream/master' into eventcomm

6189bc0

navin22 requested review from codito, smadala and mayankbansal018 November 13, 2017 10:59

Addressed some PR comments.

3d5db37

mayankbansal018 reviewed Nov 15, 2017

View reviewed changes

navin22 added 2 commits November 15, 2017 15:17

fixed some PR comments.

070ffa8

Merge branch 'master' into eventcomm

5ed4c8e

smadala reviewed Nov 16, 2017

View reviewed changes

smadala suggested changes Nov 16, 2017

View reviewed changes

navin22 added 4 commits November 16, 2017 20:32

Addressed PR Comments.

38a5e20

Merge branch 'eventcomm' of https://github.com/navin22/nvstest into e…

2fe9b5f

…ventcomm

Fixed PR comments.

6341cf6

Merge branch 'master' into eventcomm

e4886e0

mayankbansal018 suggested changes Nov 17, 2017

View reviewed changes

navin22 added 3 commits November 18, 2017 12:08

Trying to check if unit tests will pass in CI.

d066305

handling testhost manager factory sync.

1dbd500

Fixing inconsistent tests.

00f71c8

cltshivash reviewed Nov 21, 2017

View reviewed changes

mayankbansal018 and others added 3 commits November 27, 2017 17:54

Merge branch 'master' into eventcomm

a464726

Merge branch 'master' into eventcomm

5c41c0d

Merge branch 'master' into eventcomm

1fe7cec

mayankbansal018 approved these changes Dec 7, 2017

View reviewed changes

Mayank Bansal and others added 3 commits December 8, 2017 12:30

Merge branch 'master' of https://github.com/Microsoft/vstest into eve…

0db8785

…ntcomm

minor update

79bd89c

add service guid

fbb4922

mayankbansal018 force-pushed the eventcomm branch from d2329b2 to fbb4922 Compare December 11, 2017 06:50

smadala approved these changes Dec 11, 2017

View reviewed changes

mayankbansal018 merged commit a89c524 into microsoft:master Dec 11, 2017

		@@ -57,10 +57,7 @@ public Task NotifyDataAvailable()
		// connection is closed.
		var data = this.reader.ReadString();

Event based communication over sockets. #1294

Event based communication over sockets. #1294

Conversation

navin22 commented Nov 13, 2017

Description

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

navin22 commented Nov 15, 2017

navin22 commented Nov 15, 2017

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

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

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

navin22 commented Nov 17, 2017

navin22 commented Nov 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayankbansal018 commented Dec 7, 2017

mayankbansal018 commented Dec 8, 2017