Polish remote debug experience (#2685)

src/debugAdapter/goDebug.ts

@@ -8,7 +8,7 @@ import * as os from 'os';
 import * as fs from 'fs';
 import * as util from 'util';
 import { DebugProtocol } from 'vscode-debugprotocol';
-import { DebugSession, InitializedEvent, TerminatedEvent, ThreadEvent, StoppedEvent, OutputEvent, Thread, StackFrame, Scope, Source, Handles, LoggingDebugSession, Logger, logger } from 'vscode-debugadapter';
+import { DebugSession, InitializedEvent, TerminatedEvent, ThreadEvent, StoppedEvent, OutputEvent, Thread, StackFrame, Scope, Source, Handles, LoggingDebugSession, Logger, logger, Breakpoint } from 'vscode-debugadapter';
 import { existsSync, lstatSync } from 'fs';
 import { basename, dirname, extname } from 'path';
 import { spawn, ChildProcess, execSync, spawnSync, execFile } from 'child_process';
@@ -64,14 +64,11 @@ interface DebuggerState {
 	breakPointInfo: {};
 	currentThread: DebugThread;
 	currentGoroutine: DebugGoroutine;
-}
-
-interface ClearBreakpointOut {
-	breakpoint: DebugBreakpoint;
+	Running: boolean;
 }
 
 interface CreateBreakpointOut {
-	breakpoint: DebugBreakpoint;
+	Breakpoint: DebugBreakpoint;
 }
 
 interface GetVersionOut {
@@ -302,6 +299,7 @@ class Delve {
 	isApiV1: boolean;
 	dlvEnv: any;
 	stackTraceDepth: number;
+	isRemoteDebugging: boolean;
 	request: 'attach' | 'launch';
 
 	constructor(launchArgs: LaunchRequestArguments | AttachRequestArguments, program: string) {
@@ -332,10 +330,12 @@ class Delve {
 
 			if (mode === 'remote') {
 				this.debugProcess = null;
+				this.isRemoteDebugging = true;
 				serverRunning = true;  // assume server is running when in remote mode
 				connectClient(launchArgs.port, launchArgs.host);
 				return;
 			}
+			this.isRemoteDebugging = false;
 			let env: NodeJS.ProcessEnv;
 			if (launchArgs.request === 'launch') {
 				let isProgramDirectory = false;
@@ -555,6 +555,18 @@ class Delve {
 		});
 	}
 
+	/**
+	 * Returns the current state of the delve debugger.
+	 * This method does not block delve and should return immediately.
+	 */
+	public async getDebugState(): Promise<DebuggerState> {
+		// 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.
+		const callResult = await this.callPromise<DebuggerState | CommandOut>('State', [{NonBlocking: true}]);
+		return this.isApiV1 ? <DebuggerState>callResult : (<CommandOut>callResult).State;
+	}
+
 	/**
 	 * Closing a debugging session follows different approaches for launch vs attach debugging.
 	 *
@@ -582,6 +594,15 @@ class Delve {
 			await removeFile(this.localDebugeePath);
 		};
 		return new Promise(async resolve => {
+			// For remote debugging, closing the connection would terminate the
+			// program as well so we just want to disconnect.
+			// See https://www.github.com/go-delve/delve/issues/1587
+			if (this.isRemoteDebugging) {
+				const rpcConnection = await this.connection;
+				// tslint:disable-next-line no-any
+				(rpcConnection as any)['conn']['end']();
+				return;
+			}
 			const timeoutToken: NodeJS.Timer = isLocalDebugging && setTimeout(async () => {
 				log('Killing debug process manually as we could not halt delve in time');
 				await forceCleanup();
@@ -779,24 +800,39 @@ class GoDebugSession extends LoggingDebugSession {
 		this.initLaunchAttachRequest(response, args);
 	}
 
-	protected disconnectRequest(response: DebugProtocol.DisconnectResponse, args: DebugProtocol.DisconnectArguments): void {
+	protected async disconnectRequest(response: DebugProtocol.DisconnectResponse, args: DebugProtocol.DisconnectArguments): Promise<void> {
 		log('DisconnectRequest');
+		// 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) {
+				this.continue();
+			}
+		}
 		this.delve.close().then(() => {
 			log('DisconnectRequest to parent');
 			super.disconnectRequest(response, args);
 			log('DisconnectResponse');
 		});
 	}
 
-	protected configurationDoneRequest(response: DebugProtocol.ConfigurationDoneResponse, args: DebugProtocol.ConfigurationDoneArguments): void {
+	protected async configurationDoneRequest(response: DebugProtocol.ConfigurationDoneResponse, args: DebugProtocol.ConfigurationDoneArguments): Promise<void> {
 		log('ConfigurationDoneRequest');
 
 		if (this.stopOnEntry) {
 			this.sendEvent(new StoppedEvent('breakpoint', 0));
 			log('StoppedEvent("breakpoint")');
 			this.sendResponse(response);
 		} else {
-			this.continueRequest(<DebugProtocol.ContinueResponse>response);
+			this.debugState = await this.delve.getDebugState();
+			if (!this.debugState.Running) {
+				this.continueRequest(<DebugProtocol.ContinueResponse>response);
+			}
 		}
 	}
 
@@ -869,26 +905,36 @@ class GoDebugSession extends LoggingDebugSession {
 				breakpointIn.loadLocals = this.delve.loadConfig;
 				breakpointIn.cond = breakpoint.condition;
 				return this.delve.callPromise('CreateBreakpoint', [this.delve.isApiV1 ? breakpointIn : { Breakpoint: breakpointIn }]).then(null, err => {
+					// Delve does not seem to support error code at this time.
+					// TODO(quoct): Follow up with delve team.
+					if (err.toString().startsWith('Breakpoint exists at')) {
+						log('Encounter existing breakpoint: ' + breakpointIn);
+						return this.delve.isApiV1 ? breakpointIn : {Breakpoint: breakpointIn};
+					}
 					log('Error on CreateBreakpoint: ' + err.toString());
 					return null;
 				});
 			}));
 		}).then(newBreakpoints => {
+			let convertedBreakpoints: DebugBreakpoint[];
 			if (!this.delve.isApiV1) {
 				// Unwrap breakpoints from v2 apicall
-				newBreakpoints = newBreakpoints.map((bp, i) => {
-					return bp ? bp.Breakpoint : null;
+				convertedBreakpoints = newBreakpoints.map((bp, i) => {
+					return bp ? (bp as CreateBreakpointOut).Breakpoint : null;
 				});
+			} else {
+				convertedBreakpoints = newBreakpoints as DebugBreakpoint[];
 			}
+
 			log('All set:' + JSON.stringify(newBreakpoints));
-			const breakpoints = newBreakpoints.map((bp, i) => {
+			const breakpoints = convertedBreakpoints.map((bp, i) => {
 				if (bp) {
 					return { verified: true, line: bp.line };
 				} else {
 					return { verified: false, line: args.lines[i] };
 				}
 			});
-			this.breakpoints.set(file, newBreakpoints.filter(x => !!x));
+			this.breakpoints.set(file, convertedBreakpoints.filter(x => !!x));
 			return breakpoints;
 		}).then(breakpoints => {
 			response.body = { breakpoints };
@@ -900,12 +946,21 @@ class GoDebugSession extends LoggingDebugSession {
 		});
 	}
 
-	protected setBreakPointsRequest(response: DebugProtocol.SetBreakpointsResponse, args: DebugProtocol.SetBreakpointsArguments): void {
+	protected async setBreakPointsRequest(response: DebugProtocol.SetBreakpointsResponse, args: DebugProtocol.SetBreakpointsArguments): Promise<void> {
 		log('SetBreakPointsRequest');
-		if (!this.continueRequestRunning) {
-			this.setBreakPoints(response, args);
+		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) {
+			logError(`Failed to get state ${String(error)}`);
+		}
+
+		if (!this.debugState.Running && !this.continueRequestRunning) {
+			await this.setBreakPoints(response, args);
 		} else {
-			this.skipStopEventOnce = true;
+			this.skipStopEventOnce = this.continueRequestRunning;
 			this.delve.callPromise('Command', [{ name: 'halt' }]).then(() => {
 				return this.setBreakPoints(response, args).then(() => {
 					return this.continue(true).then(null, err => {