Debugger connection retry logic

[billti]

This adds retry logic when attempting to connect to the debug port on Node, in an attempt to resolve #245 and #246.

In some scenarios, especially when using Debug builds of Node/io.js, the Node process doesn't open the debug port (5858 by default) in time. At the network level, this results in the connection being refused. This change adds retry logic if the connection fails (and additional logging).

On my machine, using the 'Debug' build of the latest io.js branch (as using 'Release' shows no issue at all), I can now see the connection retry and succeed, e.g.

[17:20:09.0841895] AD7Engine Created (63760528)
[17:20:09.0907085] AD7Engine Called SetRegistryRoot
[17:20:09.0912082] AD7Engine Called SetLocale
[17:20:09.0947085] AD7Engine LaunchSuspended Called with flags 'LAUNCH_ENABLE_ENC' (63760528)
[17:20:09.1791871] Debugger connecting to URI: tcp://localhost:5858/
[17:20:11.2412522] Connection attempt 1 failed with: System.Net.Sockets.SocketException (0x80004005): No connection could be made because the target machine actively refused it 127.0.0.1:5858
   at System.Net.Sockets.TcpClient..ctor(String hostname, Int32 port)
   at Microsoft.NodejsTools.Debugger.Communication.NetworkClientFactory.CreateNetworkClient(Uri uri) in C:\src\nodejstools\Nodejs\Product\Nodejs\Debugger\Communication\NetworkClientFactory.cs:line 32
   at Microsoft.NodejsTools.Debugger.Communication.DebuggerConnection.Connect(Uri uri) in C:\src\nodejstools\Nodejs\Product\Nodejs\Debugger\Communication\DebuggerConnection.cs:line 130
[17:20:12.9912323] Debugger connected successfully
[17:20:12.9912323] DebuggerConnection: Request: {"command":"scripts","seq":1,"type":"request","arguments":{"includeSource":false}}

Note that on a retry connect, sometime the debugger enters a state where it doesn't appear to be connected, but using "Debug / Break all" will then break on the first line and debugging can then continue. This needs to be looked at separately.

[paulvanbrenk]

Do you expect any other exceptions than the SocketException you throw? If not, just filter this on that type and let the other exceptions naturally bubble up, this will preserve the stack better.

[paulvanbrenk]

Since you're not using the exception you catch, you should remove the 'ex' variable from the catch clause.

[billti]

I am using it in the WriteLine call immediately below.

[billti]

And yes, I do expect other exceptions. In fact, per the comment above the condition, I don't expect to have to throw the exception myself, that's just for safety. In general any exception trying to connect should cause either a retry or a rethrow after the max retries.

[paulvanbrenk]

In that case, I would add a message to the SocketException you throw to indicate that a networkclient was successfully created but not connected. That way we can investigate further if that happens.

[billti]

Yup, was going to do that, but there is no SocketException constructor that takes a string, and the Message property is read-only. ( https://msdn.microsoft.com/en-us/library/system.net.sockets.socketexception.socketexception(v=vs.110).aspx )

[mousetraps]

I don't expect it to be an issue in ths case, but in general we don't want to swallow critical exceptions.
https://github.com/Microsoft/nodejstools/blob/35817ffdaa1797f95628a22f588fc3ed885f884b/Common/Product/SharedProject/ExceptionExtensions.cs#L23-L29

[mousetraps]

some minor comments and lgtm after that

[mousetraps]

👍