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

First Draft for the Protocol tool. #306

Merged
merged 6 commits into from
Jan 3, 2017
Merged

First Draft for the Protocol tool. #306

merged 6 commits into from
Jan 3, 2017

Conversation

singhsarab
Copy link
Contributor

Emits the json sent and recieved for Discovery, RunAll, RunSelected scenarios.

Emits the json sent and recieved for Discovery, RunAll, RunSelected scenarios.
@singhsarab singhsarab requested a review from codito December 26, 2016 15:39

process.Start();
process.EnableRaisingEvents = true;
process.Exited += Process_Exited;
Copy link
Contributor

Choose a reason for hiding this comment

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

process.EnableRaisingEvents and process.Exited should be invoked before process.Start()

Reference : https://msdn.microsoft.com/en-us/library/system.diagnostics.process.exited(v=vs.110).aspx

processManager.StartProcess(new string[2] { parentProcessIdArgs, portArgs });

communicationManager.AcceptClientAsync();
communicationManager.WaitForClientConnection(Timeout.Infinite);
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why communicationManager.AcceptClientAsync method is async? Just after calling this method, you are making a blocking call to communicationManager.WaitForClientConnection(Timeout.Infinite).

Suggestion : It can be changed to something like this:
communicationManager.AcceptClient();
(Make relevant changes in implementaion of AcceptClient method)
And there is no need to have this call :
communicationManager.WaitForClientConnection(Timeout.Infinite);

private const string PORT_ARGUMENT = "/port:{0}";
private const string PARENT_PROCESSID_ARGUMENT = "/parentprocessid:{0}";

private static SocketCommunicationManager communicationManager;
Copy link
Contributor

Choose a reason for hiding this comment

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

Any specific reason for making this variable as static?

@codito codito merged commit 230c966 into microsoft:master Jan 3, 2017
@codito codito deleted the IDEProtocolSample branch January 3, 2017 12:46
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.

3 participants