src/goDebugFactory: respond with error if dlv dap failed to start

DelveDAPOutputAdapter constructor begins spawning of dlv dap
process but it doesn't wait until the dlv dap process is up and
running. The success or failure of dlv dap process launch is
lazily detected when the client sends messages. Previously,
we just sent a TerminatedEvent and pop up an error notification.
This CL changes DelveDAPOutputAdapter to respond with
DebugProtocol.ErrorResponse, so the client can handle the error
 - displaying the error with a modal popup
 - changing focus so DEBUG CONSOLE becomes visible

And made startAndConnectToServer catches errors and never
throw an error (reject the promise). So, DelveDAPOutputAdapter's
'connected' will never be rejected and explicitly store the error
message. Previously this rejected promise caused warnings, i.e.,
rejected promise not handled within X seconds.

Did some clean up of unnecessary async or declaration.
And, fixed and reenabled logDest tests.

Change-Id: I57b7d52d83a82c506629e07b1dec65f9e02801bc
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/321472
Trust: Hyang-Ah Hana Kim <[email protected]>
Trust: Suzy Mueller <[email protected]>
Run-TryBot: Hyang-Ah Hana Kim <[email protected]>
TryBot-Result: kokoro <[email protected]>
Reviewed-by: Suzy Mueller <[email protected]>

Author: hyangah

src/goDebugFactory.ts

@@ -15,6 +15,7 @@ import * as fs from 'fs';
 import * as net from 'net';
 import { getTool } from './goTools';
 import { Logger, TimestampedLogger } from './goLogging';
+import { DebugProtocol } from 'vscode-debugprotocol';
 
 export class GoDebugAdapterDescriptorFactory implements vscode.DebugAdapterDescriptorFactory {
 	constructor(private outputChannel?: vscode.OutputChannel) {}
@@ -198,30 +199,35 @@ export class DelveDAPOutputAdapter extends ProxyDebugAdapter {
 		this.connected = this.startAndConnectToServer();
 	}
 
-	private connected: Promise<void>;
+	private connected: Promise<{ connected: boolean; reason?: any }>;
 	private dlvDapServer: ChildProcess;
 	private port: number;
 	private socket: net.Socket;
 	private terminatedOnError = false;
 
 	protected async sendMessageToServer(message: vscode.DebugProtocolMessage): Promise<void> {
-		try {
-			await this.connected;
+		const { connected, reason } = await this.connected;
+		if (connected) {
 			super.sendMessageToServer(message);
-		} catch (err) {
-			if (this.terminatedOnError) {
-				return;
-			}
+			return;
+		}
+		const errMsg = `Couldn't start dlv dap:\n${reason}`;
+		if (this.terminatedOnError) {
 			this.terminatedOnError = true;
-			// If there was an error connecting, show an error message and send a terminated event
-			// since we cannot start.
-			if (err) {
-				const errMsg = `Debug Error: ${err}`;
-				this.outputEvent('stderr', errMsg);
-				vscode.window.showErrorMessage(errMsg);
-			}
+			this.outputEvent('stderr', errMsg);
 			this.sendMessageToClient(new TerminatedEvent());
 		}
+		if ((message as any).type === 'request') {
+			const req = message as DebugProtocol.Request;
+			this.sendMessageToClient({
+				seq: 0,
+				type: 'response',
+				request_seq: req.seq,
+				success: false,
+				command: req.command,
+				message: errMsg
+			});
+		}
 	}
 
 	async dispose(timeoutMS?: number) {
@@ -268,48 +274,54 @@ export class DelveDAPOutputAdapter extends ProxyDebugAdapter {
 	}
 
 	private async startAndConnectToServer() {
-		const { port, host, dlvDapServer } = await startDapServer(
-			this.config,
-			(msg) => this.outputEvent('stdout', msg),
-			(msg) => this.outputEvent('stderr', msg),
-			(msg) => {
-				this.outputEvent('console', msg);
-				// Some log messages generated after vscode stops the debug session
-				// may not appear in the DEBUG CONSOLE. For easier debugging, log
-				// the messages through the logger that prints to Go Debug output
-				// channel.
-				this.logger?.info(msg);
-			}
-		);
-		const socket = await new Promise<net.Socket>((resolve, reject) => {
-			// eslint-disable-next-line prefer-const
-			let timer: NodeJS.Timeout;
-			const s = net.createConnection(port, host, () => {
-				clearTimeout(timer);
-				resolve(s);
+		try {
+			const { port, host, dlvDapServer } = await startDapServer(
+				this.config,
+				(msg) => this.outputEvent('stdout', msg),
+				(msg) => this.outputEvent('stderr', msg),
+				(msg) => {
+					this.outputEvent('console', msg);
+					// Some log messages generated after vscode stops the debug session
+					// may not appear in the DEBUG CONSOLE. For easier debugging, log
+					// the messages through the logger that prints to Go Debug output
+					// channel.
+					this.logger?.info(msg);
+				}
+			);
+			const socket = await new Promise<net.Socket>((resolve, reject) => {
+				// eslint-disable-next-line prefer-const
+				let timer: NodeJS.Timeout;
+				const s = net.createConnection(port, host, () => {
+					clearTimeout(timer);
+					resolve(s);
+				});
+				timer = setTimeout(() => {
+					reject('connection timeout');
+					s?.destroy();
+				}, 1000);
 			});
-			timer = setTimeout(() => {
-				reject('connection timeout');
-				s?.destroy();
-			}, 1000);
-		});
 
-		this.dlvDapServer = dlvDapServer;
-		this.port = port;
-		this.socket = socket;
-		this.start(this.socket, this.socket);
+			this.dlvDapServer = dlvDapServer;
+			this.port = port;
+			this.socket = socket;
+			this.start(this.socket, this.socket);
+		} catch (err) {
+			return { connected: false, reason: err };
+		}
+		this.logger?.debug(`Running dlv dap server: port=${this.port} pid=${this.dlvDapServer.pid}\n`);
+		return { connected: true };
 	}
 
 	private outputEvent(dest: string, output: string, data?: any) {
 		this.sendMessageToClient(new OutputEvent(output, dest, data));
 	}
 }
 
-export async function startDapServer(
+async function startDapServer(
 	configuration: vscode.DebugConfiguration,
-	log?: (msg: string) => void,
-	logErr?: (msg: string) => void,
-	logConsole?: (msg: string) => void
+	log: (msg: string) => void,
+	logErr: (msg: string) => void,
+	logConsole: (msg: string) => void
 ): Promise<{ port: number; host: string; dlvDapServer?: ChildProcessWithoutNullStreams }> {
 	const host = configuration.host || '127.0.0.1';
 
@@ -319,20 +331,11 @@ export async function startDapServer(
 		return { port: configuration.port, host };
 	}
 	const port = await getPort();
-	if (!log) {
-		log = appendToDebugConsole;
-	}
-	if (!logErr) {
-		logErr = appendToDebugConsole;
-	}
-	if (!logConsole) {
-		logConsole = appendToDebugConsole;
-	}
 	const dlvDapServer = await spawnDlvDapServerProcess(configuration, host, port, log, logErr, logConsole);
 	return { dlvDapServer, port, host };
 }
 
-async function spawnDlvDapServerProcess(
+function spawnDlvDapServerProcess(
 	launchArgs: vscode.DebugConfiguration,
 	host: string,
 	port: number,
@@ -350,12 +353,19 @@ async function spawnDlvDapServerProcess(
 		logErr(
 			`Couldn't find dlv-dap at the Go tools path, ${process.env['GOPATH']}${
 				env['GOPATH'] ? ', ' + env['GOPATH'] : ''
-			} or ${envPath}`
-		);
-		throw new Error(
-			'Cannot find Delve debugger. Install from https://github.com/go-delve/delve & ensure it is in your Go tools path, "GOPATH/bin" or "PATH".'
+			} or ${envPath}\n` +
+				'Follow the setup instruction in https://github.com/golang/vscode-go/blob/master/docs/dlv-dap.md#getting-started.\n'
 		);
+		throw new Error('Cannot find Delve debugger (dlv dap)');
 	}
+	let dir = '';
+	try {
+		dir = parseProgramArgSync(launchArgs).dirname;
+	} catch (err) {
+		logErr(`Program arg: ${launchArgs.program}\n${err}\n`);
+		throw err; // rethrow so the caller knows it failed.
+	}
+
 	const dlvArgs = new Array<string>();
 	dlvArgs.push('dap');
 	// add user-specified dlv flags first. When duplicate flags are specified,
@@ -379,12 +389,12 @@ async function spawnDlvDapServerProcess(
 
 	const logDest = launchArgs.logDest;
 	if (typeof logDest === 'number') {
-		logErr('Using a file descriptor for `logDest` is not allowed.');
+		logErr(`Using a file descriptor for 'logDest' (${logDest}) is not allowed.\n`);
 		throw new Error('Using a file descriptor for `logDest` is not allowed.');
 	}
 	if (logDest && !path.isAbsolute(logDest)) {
 		logErr(
-			'Using a relative path for `logDest` is not allowed.\nSee [variables](https://code.visualstudio.com/docs/editor/variables-reference)'
+			`Using a relative path for 'logDest' (${logDest}) is not allowed.\nSee https://code.visualstudio.com/docs/editor/variables-reference if you want workspace-relative path.\n`
 		);
 		throw new Error('Using a relative path for `logDest` is not allowed');
 	}
@@ -397,14 +407,13 @@ async function spawnDlvDapServerProcess(
 
 	const logDestStream = logDest ? fs.createWriteStream(logDest) : undefined;
 
-	logConsole(`Running: ${dlvPath} ${dlvArgs.join(' ')}\n`);
+	logConsole(`Starting: ${dlvPath} ${dlvArgs.join(' ')}\n`);
 
-	const dir = parseProgramArgSync(launchArgs).dirname;
 	// TODO(hyangah): determine the directories:
 	//    run `dlv` => where dlv will create the default __debug_bin. (This won't work if the directory is not writable. Fix it)
 	//    build program => 'program' directory. (This won't work for multimodule workspace. Fix it)
 	//    run program => cwd (If test, make sure to run in the package directory.)
-	return await new Promise<ChildProcess>((resolve, reject) => {
+	return new Promise<ChildProcess>((resolve, reject) => {
 		const p = spawn(dlvPath, dlvArgs, {
 			cwd: dir,
 			env,
@@ -521,8 +530,3 @@ export function parseProgramArgSync(
 	const dirname = programIsDirectory ? program : path.dirname(program);
 	return { program, dirname, programIsDirectory };
 }
-
-// appendToDebugConsole is declared as an exported const rather than a function, so it can be stubbbed in testing.
-export const appendToDebugConsole = (msg: string) => {
-	console.error(msg);
-};

test/integration/goDebug.test.ts

@@ -1760,7 +1760,7 @@ const testAll = (ctx: Mocha.Context, isDlvDap: boolean) => {
 		});
 	});
 
-	suite.skip('logDest attribute tests', () => {
+	suite('logDest attribute tests', () => {
 		const PROGRAM = path.join(DATA_ROOT, 'baseTest');
 
 		let tmpDir: string;
@@ -1808,13 +1808,13 @@ const testAll = (ctx: Mocha.Context, isDlvDap: boolean) => {
 				logDest
 			};
 
+			await initializeDebugConfig(config);
 			try {
-				await initializeDebugConfig(config);
+				await dc.initializeRequest();
+				assert.fail('dlv dap started normally, wanted the invalid logDest to cause failure');
 			} catch (error) {
 				assert(error?.message.includes(wantedErrorMessage), `unexpected error: ${error}`);
-				return;
 			}
-			assert.fail('dlv dap started normally, wanted the invalid logDest to cause failure');
 		}
 		test('relative path as logDest triggers an error', async function () {
 			if (!isDlvDap || process.platform === 'win32') this.skip();