Skip to content

Debugger connection retry logic #406

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Sep 2, 2015
Merged

Debugger connection retry logic #406

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7285903
Debugger connection retry logic
billti Sep 2, 2015
543de5a
Code review feedback to formatting and exception logging
billti Sep 2, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 41 additions & 1 deletion Nodejs/Product/Nodejs/Debugger/Communication/DebuggerConnection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -112,10 +113,49 @@ public Version NodeVersion {
/// <param name="uri">URI identifying the endpoint to connect to.</param>
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) {
if (ex.IsCriticalException()) {
throw;
}
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);
Copy link
Contributor

@mousetraps mousetraps Sep 2, 2015

Choose a reason for hiding this comment

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

I'm fine taking this for RC2, but let's file an issue on this since we really shouldn't be blocking.

Copy link
Member Author

@billti billti Sep 2, 2015

Choose a reason for hiding this comment

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

Yeah, but as noted above, just the existing TCP connection call blocks for 2 to 3 seconds already trying to connect. Getting this whole logic off the UI thread, or moving to async, seems desirable, but that's a much more invasive change at this point. (My point being the whole refactor should be the opened issue, not just this 200ms delay)

Copy link
Contributor

@mousetraps mousetraps Sep 2, 2015

Choose a reason for hiding this comment

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

gotcha, makes sense

}
}
}
}

Task.Factory.StartNew(ReceiveAndDispatchMessagesWorker);
Expand Down
6 changes: 6 additions & 0 deletions Nodejs/Product/Nodejs/Debugger/DebugEngine/Remote/NodeRemoteEnumDebugProcesses.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,16 +105,22 @@ private static NodeRemoteDebugProcess Connect(NodeRemoteDebugPort port, INetwork
} catch (OperationCanceledException) {
LiveLogger.WriteLine("NodeRemoteEnumDebugProcesses ping timed out.");
} 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("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);
Expand Down