From 7285903785c6208689d68a98f8ec108c1b32ba21 Mon Sep 17 00:00:00 2001 From: Bill Ticehurst Date: Wed, 2 Sep 2015 10:22:52 -0700 Subject: [PATCH 1/2] Debugger connection retry logic --- .../Communication/DebuggerConnection.cs | 45 ++++++++++++++++++- .../Remote/NodeRemoteEnumDebugProcesses.cs | 8 +++- 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/Nodejs/Product/Nodejs/Debugger/Communication/DebuggerConnection.cs b/Nodejs/Product/Nodejs/Debugger/Communication/DebuggerConnection.cs index 6e8615874..2992fa088 100644 --- a/Nodejs/Product/Nodejs/Debugger/Communication/DebuggerConnection.cs +++ b/Nodejs/Product/Nodejs/Debugger/Communication/DebuggerConnection.cs @@ -112,10 +112,53 @@ public Version NodeVersion { /// URI identifying the endpoint to connect to. public void Connect(Uri uri) { Utilities.ArgumentNotNull("uri", uri); + LiveLogger.WriteLine("Debugger connecting to URI: {0}", uri); Close(); lock (_networkClientLock) { - _networkClient = _networkClientFactory.CreateNetworkClient(uri); + int connection_attempts = 0; + const int MAX_ATTEMPTS = 5; + while (true) + { + connection_attempts++; + try + { + // TODO: This currently results in a call to the synchronous TcpClient + // constructor, which is a blocking call, and can take a couple of seconds + // to connect (with timeouts and retries). This code is running on the UI + // thread. Ideally this should be connecting async, or moved off the UI thread. + _networkClient = _networkClientFactory.CreateNetworkClient(uri); + + // Unclear if the above can succeed and not be connected, but check for safety. + // The code needs to either break out the while loop, or hit the retry logic + // in the exception handler. + if (_networkClient.Connected) + { + LiveLogger.WriteLine("Debugger connected successfully"); + break; + } + else + { + throw new SocketException(); + } + } + catch (Exception ex) + { + LiveLogger.WriteLine("Connection attempt {0} failed with: {1}", connection_attempts, ex); + if (connection_attempts >= MAX_ATTEMPTS) + { + throw; + } + else + { + // See above TODO. This should be moved off the UI thread or posted to retry + // without blocking in the meantime. For now, this seems the lesser of two + // evils. (The other being the debugger failing to attach on launch if the + // debuggee socket wasn't open quickly enough). + System.Threading.Thread.Sleep(200); + } + } + } } Task.Factory.StartNew(ReceiveAndDispatchMessagesWorker); diff --git a/Nodejs/Product/Nodejs/Debugger/DebugEngine/Remote/NodeRemoteEnumDebugProcesses.cs b/Nodejs/Product/Nodejs/Debugger/DebugEngine/Remote/NodeRemoteEnumDebugProcesses.cs index 61fa740b0..b173f4350 100644 --- a/Nodejs/Product/Nodejs/Debugger/DebugEngine/Remote/NodeRemoteEnumDebugProcesses.cs +++ b/Nodejs/Product/Nodejs/Debugger/DebugEngine/Remote/NodeRemoteEnumDebugProcesses.cs @@ -103,18 +103,24 @@ private static NodeRemoteDebugProcess Connect(NodeRemoteDebugPort port, INetwork break; } } catch (OperationCanceledException) { - LiveLogger.WriteLine("NodeRemoteEnumDebugProcesses ping timed out."); + LiveLogger.WriteLine("OperationCanceledException connecting to remote debugger"); } catch (DebuggerAlreadyAttachedException ex) { + LiveLogger.WriteLine("DebuggerAlreadyAttachedException connecting to remote debugger"); exception = ex; } catch (AggregateException ex) { + LiveLogger.WriteLine("AggregateException connecting to remote debugger"); exception = ex; } catch (IOException ex) { + LiveLogger.WriteLine("IOException connecting to remote debugger"); exception = ex; } catch (SocketException ex) { + LiveLogger.WriteLine("SocketException connecting to remote debugger"); exception = ex; } catch (WebSocketException ex) { + LiveLogger.WriteLine("WebSocketException connecting to remote debugger"); exception = ex; } catch (PlatformNotSupportedException) { + LiveLogger.WriteLine("NodeRemoteEnumDebugProcesses ping timed out."); MessageBox.Show( "Remote debugging of node.js Microsoft Azure applications is only supported on Windows 8 and above.", null, MessageBoxButtons.OK, MessageBoxIcon.Error); From 543de5a60c9acd6729015a6deb2171aec2b9cf67 Mon Sep 17 00:00:00 2001 From: Bill Ticehurst Date: Wed, 2 Sep 2015 14:52:17 -0700 Subject: [PATCH 2/2] Code review feedback to formatting and exception logging --- .../Communication/DebuggerConnection.cs | 25 ++++++++----------- .../Remote/NodeRemoteEnumDebugProcesses.cs | 4 +-- 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/Nodejs/Product/Nodejs/Debugger/Communication/DebuggerConnection.cs b/Nodejs/Product/Nodejs/Debugger/Communication/DebuggerConnection.cs index 2992fa088..1130e425e 100644 --- a/Nodejs/Product/Nodejs/Debugger/Communication/DebuggerConnection.cs +++ b/Nodejs/Product/Nodejs/Debugger/Communication/DebuggerConnection.cs @@ -22,6 +22,7 @@ using System.Text.RegularExpressions; using System.Threading.Tasks; using Microsoft.NodejsTools.Logging; +using Microsoft.VisualStudioTools; using Microsoft.VisualStudioTools.Project; using Newtonsoft.Json; @@ -118,11 +119,9 @@ public void Connect(Uri uri) { lock (_networkClientLock) { int connection_attempts = 0; const int MAX_ATTEMPTS = 5; - while (true) - { + while (true) { connection_attempts++; - try - { + try { // TODO: This currently results in a call to the synchronous TcpClient // constructor, which is a blocking call, and can take a couple of seconds // to connect (with timeouts and retries). This code is running on the UI @@ -132,25 +131,23 @@ public void Connect(Uri uri) { // Unclear if the above can succeed and not be connected, but check for safety. // The code needs to either break out the while loop, or hit the retry logic // in the exception handler. - if (_networkClient.Connected) - { + if (_networkClient.Connected) { LiveLogger.WriteLine("Debugger connected successfully"); break; } - else - { + else { throw new SocketException(); } } - catch (Exception ex) - { + catch (Exception ex) { + if (ex.IsCriticalException()) { + throw; + } LiveLogger.WriteLine("Connection attempt {0} failed with: {1}", connection_attempts, ex); - if (connection_attempts >= MAX_ATTEMPTS) - { + if (connection_attempts >= MAX_ATTEMPTS) { throw; } - else - { + else { // See above TODO. This should be moved off the UI thread or posted to retry // without blocking in the meantime. For now, this seems the lesser of two // evils. (The other being the debugger failing to attach on launch if the diff --git a/Nodejs/Product/Nodejs/Debugger/DebugEngine/Remote/NodeRemoteEnumDebugProcesses.cs b/Nodejs/Product/Nodejs/Debugger/DebugEngine/Remote/NodeRemoteEnumDebugProcesses.cs index b173f4350..985f6fa09 100644 --- a/Nodejs/Product/Nodejs/Debugger/DebugEngine/Remote/NodeRemoteEnumDebugProcesses.cs +++ b/Nodejs/Product/Nodejs/Debugger/DebugEngine/Remote/NodeRemoteEnumDebugProcesses.cs @@ -103,7 +103,7 @@ private static NodeRemoteDebugProcess Connect(NodeRemoteDebugPort port, INetwork break; } } catch (OperationCanceledException) { - LiveLogger.WriteLine("OperationCanceledException connecting to remote debugger"); + LiveLogger.WriteLine("NodeRemoteEnumDebugProcesses ping timed out."); } catch (DebuggerAlreadyAttachedException ex) { LiveLogger.WriteLine("DebuggerAlreadyAttachedException connecting to remote debugger"); exception = ex; @@ -120,7 +120,7 @@ private static NodeRemoteDebugProcess Connect(NodeRemoteDebugPort port, INetwork LiveLogger.WriteLine("WebSocketException connecting to remote debugger"); exception = ex; } catch (PlatformNotSupportedException) { - LiveLogger.WriteLine("NodeRemoteEnumDebugProcesses ping timed out."); + LiveLogger.WriteLine("PlatformNotSupportedException connecting to remote debugger"); MessageBox.Show( "Remote debugging of node.js Microsoft Azure applications is only supported on Windows 8 and above.", null, MessageBoxButtons.OK, MessageBoxIcon.Error);