debugAdapter: Fix bugs that cause remote debug to hang
The issues being fixed in this PR:
1. There are instances where we are issuing a blocking call to Delve while it is running. That blocks subsequent requests to Delve and can cause the debugger to appear as hanged. Hence, we will need to check Delve's debug state before issuing blocking request in `stacktraceRequest` and `threadsRequest` (fixes golang/vscode-go#740).
2. When disconnecting, multiple requests `disconnectRequest` can be received. Subsequent `disconnectRequest` should not invoke Delve through the JSON RPC connection again as this may already be closed so we need to guard against that (fixes golang/vscode-go#766).
3. Also, even if Delve is blocked in `disconnectRequest`, `disconnectResponse` should always be sent back to VSCode after a period of time. Otherwise, in the case of Theia in particular, users will not be able to reset (fixes golang/vscode-go#761).
4. In `close` function of `Delve` class, we are not resolving a promise in the case of remote debugging and this causes the `close` function to hang and not send the `disconnectResponse` back (fixes golang/vscode-go#764).
5. Add more logging to help us easily debug similar scenarios in the future (fixes golang/vscode-go#763).
## Repro
1. Clones a Go program (for example: https://github.com/GoogleCloudPlatform/cloud-code-samples/tree/master/golang/go-hello-world).
2. Runs the program in Delve through a terminal:
`dlv debug --continue --accept-multiclient --api-version=2 --headless --listen=:3456 --log`
3. Connect to it by creating a configuration:
```
{
"name": "Connect to server",
"type": "go",
"request": "attach",
"mode": "remote",
"remotePath": "${workspaceFolder}",
"port": 3456,
"host": "127.0.0.1",
"cwd": "${workspaceFolder}"
}
```
4. Sets a breakpoint and sees that it's not working.
5. Disconnecting also does not work.
Change-Id: Iaa385c392326f489a9ec0e804d2c2bce6cc3defe
GitHub-Last-Rev: e052642274a8551560c129605fbb69d47727eb24
GitHub-Pull-Request: golang/vscode-go#686
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/257337
Trust: Quoc Truong <quoct@google.com>
Reviewed-by: Polina Sokolova <polina@google.com>
diff --git a/src/debugAdapter/goDebug.ts b/src/debugAdapter/goDebug.ts
index bb8c1dd..5e50dc6 100644
--- a/src/debugAdapter/goDebug.ts
+++ b/src/debugAdapter/goDebug.ts
@@ -343,6 +343,16 @@
return filePath.includes('/') ? '/' : '\\';
}
+// Comparing two different file paths while ignoring any different path separators.
+function compareFilePathIgnoreSeparator(firstFilePath: string, secondFilePath: string): boolean {
+ const firstSeparator = findPathSeparator(firstFilePath);
+ const secondSeparator = findPathSeparator(secondFilePath);
+ if (firstSeparator === secondSeparator) {
+ return firstFilePath === secondFilePath;
+ }
+ return firstFilePath === secondFilePath.split(secondSeparator).join(firstSeparator);
+}
+
export function escapeGoModPath(filePath: string) {
return filePath.replace(/[A-Z]/g, (match: string) => `!${match.toLocaleLowerCase()}`);
}
@@ -376,6 +386,7 @@
public stackTraceDepth: number;
public isRemoteDebugging: boolean;
public goroot: string;
+ public delveConnectionClosed = false;
private localDebugeePath: string | undefined;
private debugProcess: ChildProcess;
private request: 'attach' | 'launch';
@@ -695,9 +706,10 @@
*
* For attach debugging there are two scenarios; attaching to a local process by ID or connecting to a
* remote delve server. For attach-local we start the delve process so will also terminate it however we
- * detach from the debugee without killing it. For attach-remote we only detach from delve.
+ * detach from the debugee without killing it. For attach-remote we only close the client connection,
+ * but do not terminate the remote server.
*
- * The only way to detach from delve when it is running a program is to send a Halt request first.
+ * For local debugging, the only way to detach from delve when it is running a program is to send a Halt request first.
* Since the Halt request might sometimes take too long to complete, we have a timer in place to forcefully kill
* the debug process and clean up the assets in case of local debugging
*/
@@ -713,18 +725,19 @@
await forceCleanup();
return Promise.resolve();
}
- log('HaltRequest');
const isLocalDebugging: boolean = this.request === 'launch' && !!this.debugProcess;
return new Promise(async (resolve) => {
- // For remote debugging, closing the connection would terminate the
- // program as well so we just want to disconnect.
+ // For remote debugging, we want to leave the remote dlv server running,
+ // so instead of killing it via halt+detach, we just close the network connection.
// See https://www.github.com/go-delve/delve/issues/1587
if (this.isRemoteDebugging) {
+ log('Remote Debugging: close dlv connection.');
const rpcConnection = await this.connection;
// tslint:disable-next-line no-any
(rpcConnection as any)['conn']['end']();
- return;
+ this.delveConnectionClosed = true;
+ return resolve();
}
const timeoutToken: NodeJS.Timer =
isLocalDebugging &&
@@ -736,6 +749,7 @@
let haltErrMsg: string;
try {
+ log('HaltRequest');
await this.callPromise('Command', [{ name: 'halt' }]);
} catch (err) {
log('HaltResponse');
@@ -854,30 +868,44 @@
args: DebugProtocol.DisconnectArguments
): Promise<void> {
log('DisconnectRequest');
- if (!this.delve) {
- log('DisconnectRequest to parent');
- super.disconnectRequest(response, args);
- log('DisconnectResponse');
- return;
+ if (this.delve) {
+ // Since users want to reset when they issue a disconnect request,
+ // we should have a timeout in case disconnectRequestHelper hangs.
+ await Promise.race([
+ this.disconnectRequestHelper(response, args),
+ new Promise((resolve) => setTimeout(() => {
+ log('DisconnectRequestHelper timed out after 5s.');
+ resolve();
+ }, 5_000))
+ ]);
}
+ this.shutdownProtocolServer(response, args);
+ log('DisconnectResponse');
+ }
+
+ protected async disconnectRequestHelper(
+ response: DebugProtocol.DisconnectResponse,
+ args: DebugProtocol.DisconnectArguments
+ ): Promise<void> {
// For remote process, we have to issue a continue request
// before disconnecting.
if (this.delve.isRemoteDebugging) {
- // We don't have to wait for continue call
- // because we are not doing anything with the result.
- // Also, DisconnectRequest will return before
- // we get the result back from delve.
- this.debugState = await this.delve.getDebugState();
- if (!this.debugState.Running) {
+ // There is a chance that a second disconnectRequest can come through
+ // if users click detach multiple times. In that case, we want to
+ // guard against talking to the closed Delve connection.
+ if (this.delve.delveConnectionClosed) {
+ log(`Skip disconnectRequestHelper as Delve's connection is already closed.`);
+ return;
+ }
+
+ if (!(await this.isDebuggeeRunning())) {
+ log(`Issuing a continue command before closing Delve's connection as the debuggee is not running.`);
this.continue();
}
}
- this.delve.close().then(() => {
- log('DisconnectRequest to parent');
- super.disconnectRequest(response, args);
- log('DisconnectResponse');
- });
+ log('Closing Delve.');
+ await this.delve.close();
}
protected async configurationDoneRequest(
@@ -888,12 +916,9 @@
if (this.stopOnEntry) {
this.sendEvent(new StoppedEvent('entry', 1));
log('StoppedEvent("entry")');
- } else {
- this.debugState = await this.delve.getDebugState();
- if (!this.debugState.Running) {
- log('Changing DebugState from Halted to Running');
- this.continue();
- }
+ } else if (!await this.isDebuggeeRunning()) {
+ log('Changing DebugState from Halted to Running');
+ this.continue();
}
this.sendResponse(response);
log('ConfigurationDoneResponse', response);
@@ -1168,19 +1193,12 @@
args: DebugProtocol.SetBreakpointsArguments
): Promise<void> {
log('SetBreakPointsRequest');
- try {
- // If a program is launched with --continue, the program is running
- // before we can run attach. So we would need to check the state.
- // We use NonBlocking so the call would return immediately.
- this.debugState = await this.delve.getDebugState();
- } catch (error) {
- this.logDelveError(error, 'Failed to get state');
- }
-
- if (!this.debugState.Running && !this.continueRequestRunning) {
+ if (!(await this.isDebuggeeRunning())) {
+ log('Debuggee is not running. Setting breakpoints without halting.');
await this.setBreakPoints(response, args);
} else {
this.skipStopEventOnce = this.continueRequestRunning;
+ log(`Halting before setting breakpoints. SkipStopEventOnce is ${this.skipStopEventOnce}.`);
this.delve.callPromise('Command', [{ name: 'halt' }]).then(
() => {
return this.setBreakPoints(response, args).then(() => {
@@ -1203,9 +1221,9 @@
}
}
- protected threadsRequest(response: DebugProtocol.ThreadsResponse): void {
- if (this.continueRequestRunning) {
- // Thread request to delve is syncronous and will block if a previous async continue request didn't return
+ protected async threadsRequest(response: DebugProtocol.ThreadsResponse): Promise<void> {
+ if ((await this.isDebuggeeRunning())) {
+ // Thread request to delve is synchronous and will block if a previous async continue request didn't return
response.body = { threads: [new Thread(1, 'Dummy')] };
return this.sendResponse(response);
}
@@ -1244,11 +1262,20 @@
});
}
- protected stackTraceRequest(
+ protected async stackTraceRequest(
response: DebugProtocol.StackTraceResponse,
args: DebugProtocol.StackTraceArguments
- ): void {
+ ): Promise<void> {
log('StackTraceRequest');
+ // For normal VSCode, this request doesn't get invoked when we send a Dummy thread
+ // in the scenario where the debuggee is running.
+ // For Theia, however, this does get invoked and so we should just send an error
+ // response that we cannot get the stack trace at this point since the debugggee is running.
+ if (await this.isDebuggeeRunning()) {
+ this.sendErrorResponse(response, 2004, 'Unable to produce stack trace as the debugger is running');
+ return;
+ }
+
// delve does not support frame paging, so we ask for a large depth
const goroutineId = args.threadId;
const stackTraceIn = { id: goroutineId, depth: this.delve.stackTraceDepth };
@@ -1919,11 +1946,14 @@
return null;
}
}
+
+ // Make sure that we compare the file names with the same separators.
const matchedBreakpoint = existingBreakpoints.find(
(existingBreakpoint) =>
- existingBreakpoint.line === breakpointIn.line &&
- existingBreakpoint.file === breakpointIn.file
+ existingBreakpoint.line === breakpointIn.line
+ && compareFilePathIgnoreSeparator(existingBreakpoint.file, breakpointIn.file)
);
+
if (!matchedBreakpoint) {
log(`Cannot match breakpoint ${breakpointIn} with existing breakpoints.`);
return null;
@@ -2166,6 +2196,7 @@
}
private handleReenterDebug(reason: string): void {
+ log(`handleReenterDebug(${reason}).`);
this.cleanupHandles();
if (this.debugState.exited) {
@@ -2190,6 +2221,7 @@
}
if (this.skipStopEventOnce) {
+ log(`Skipping stop event for ${reason}. The current Go routines is ${this.debugState?.currentGoroutine}.`);
this.skipStopEventOnce = false;
return;
}
@@ -2201,6 +2233,34 @@
});
}
}
+
+ // Returns true if the debuggee is running.
+ // The call getDebugState is non-blocking so it should return
+ // almost instantaneously. However, if we run into some errors,
+ // we will fall back to the internal tracking of the debug state.
+ // TODO: If Delve is not in multi-client state, we can simply
+ // track the running state with continueRequestRunning internally
+ // instead of issuing a getDebugState call to Delve. Perhaps we want to
+ // do that to improve performance in the future.
+ private async isDebuggeeRunning(): Promise<boolean> {
+ try {
+ this.debugState = await this.delve.getDebugState();
+ return this.debugState.Running;
+ } catch (error) {
+ this.logDelveError(error, 'Failed to get state');
+ // Fall back to the internal tracking.
+ return this.continueRequestRunning;
+ }
+ }
+
+ private shutdownProtocolServer(
+ response: DebugProtocol.DisconnectResponse,
+ args: DebugProtocol.DisconnectArguments
+ ): void {
+ log('DisconnectRequest to parent to shut down protocol server.');
+ super.disconnectRequest(response, args);
+ }
+
private continue(calledWhenSettingBreakpoint?: boolean): Thenable<void> {
this.continueEpoch++;
const closureEpoch = this.continueEpoch;
@@ -2364,13 +2424,13 @@
// Debugger may be stopped at this point but we still can (and need) to obtain state and stacktrace
let goroutineId = 0;
try {
- const stateCallResult = await this.delve.getDebugState();
+ this.debugState = await this.delve.getDebugState();
// In some fault scenarios there may not be a currentGoroutine available from the debugger state
// Use the current thread
- if (!stateCallResult.currentGoroutine) {
- goroutineId = stateCallResult.currentThread.goroutineID;
+ if (!this.debugState.currentGoroutine) {
+ goroutineId = this.debugState.currentThread.goroutineID;
} else {
- goroutineId = stateCallResult.currentGoroutine.id;
+ goroutineId = this.debugState.currentGoroutine.id;
}
} catch (error) {
logError('dumpStacktrace - Failed to get debugger state ' + error);
