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.

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

Debugger connection retry logic #406

Merged
merged 2 commits into from
Sep 2, 2015
Merged

Debugger connection retry logic #406

Show file tree
Hide file tree
Changes from 1 commit
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
45 changes: 44 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 @@ -112,10 +112,53 @@ 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)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

braces on same line here and everywhere else.

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I am using it in the WriteLine call immediately below.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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 )

Copy link
Contributor

Choose a reason for hiding this comment

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

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

{
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

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

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

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
8 changes: 7 additions & 1 deletion 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 @@ -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");
Copy link
Contributor

Choose a reason for hiding this comment

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

We included the "ping timed out" before to make it clearer what the core issue actually was. Don't mind including the exception as well, but we should also include the ping timeout bit.

} 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.");
Copy link
Contributor

Choose a reason for hiding this comment

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

PlatformNotSupportedException doesn't have anything to do with the ping timing out, does it?

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