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

Fix socket exception on datacollection in parallel #1505

Merged
merged 9 commits into from
Mar 26, 2018

Conversation

smadala
Copy link
Contributor

@smadala smadala commented Mar 26, 2018

Description

Fix

  • Stop the Server/Client on TestExecution.Completed/TestDiscovery.Completed received.
  • Make EqtTrace logging friendly to parallel scenario.

Related issue

#1472

@@ -596,6 +596,7 @@ private void SetOperationComplete()
EqtTrace.Verbose("TestRequestSender.SetOperationComplete: Setting operation complete.");
}

this.communicationEndpoint.Stop();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need lock here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not required.

@@ -25,14 +25,19 @@ internal static class TcpClientExtensions
CancellationToken cancellationToken)
{
Exception error = null;
var remoteEndPoint = ((IPEndPoint)client.Client.RemoteEndPoint).ToString();
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need typecast?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

try
{
if (client.Client.Poll(STREAMREADTIMEOUT, SelectMode.SelectRead))
{
EqtTrace.Verbose("TcpClientExtensions.MessageLoopAsync: NotifyDataAvailable remoteendPoint: {0} localEndPoint: {1}", remoteEndPoint, localEndPoint);
Copy link
Contributor

Choose a reason for hiding this comment

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

MessageLoopAsync [](start = 62, length = 16)

Lets see logs for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

var connected = this.dataCollectionRequestSender.WaitForRequestHandlerConnection(this.connectionTimeout);
if (connected == false)
{
EqtTrace.Error("ProxyDataCollectionManager.Initialize: failed to connect to datacollector process.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Add datacollector process ID we could not connect to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@mayankbansal018 mayankbansal018 left a comment

Choose a reason for hiding this comment

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

:shipit:

@smadala smadala merged commit d872914 into rel/15.6 Mar 26, 2018
smadala added a commit to smadala/vstest that referenced this pull request Mar 27, 2018
* Add more logging for datacollection manager

* Skip additional messages

* Add more logging

* Add logging for MessageLoopAsync

* Add logging for MessageLoopAsync 2

* Stop the communication server on operation complete

* Add tests and remove not required logging

* Address review comments
smadala added a commit that referenced this pull request Apr 2, 2018
* Fix socket exception on datacollection in parallel (#1505)

* Add more logging for datacollection manager

* Skip additional messages

* Add more logging

* Add logging for MessageLoopAsync

* Add logging for MessageLoopAsync 2

* Stop the communication server on operation complete

* Add tests and remove not required logging

* Address review comments

* Remove IPEndpoint typecast
@smadala smadala deleted the datacollector-socket-exception branch April 18, 2018 13:33
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.

2 participants