Skip to content

Commit 943ff11

Browse files
hyangahhyangah
committed
src/goDebugFactory: support integrated/external console
Implementation is done by taking advantage of VS Code's RunInTerminal implementation. Upon receiving a `initializeRequest` with console=integrated/external request, the thin debug adapter (DelveDAPOutputAdapter) starts a temporary TCP listener and sends a RunInTerminal request to start the `dlv dap --client-addr` command from the specified terminal. The `dlv dap` command will connect to the TCP listener, and then DelveDAPOutputAdapter starts to forward messages between VS Code and the dlv dap process running in the terminal. Strictly speaking, sending a RunInTerminal request before `initializeResponse` is incorrect. According to DAP spec: ``` Until the debug adapter has responded to with an ‘initialize’ response, the client must not send any additional requests or events to the debug adapter. In addition the debug adapter is not allowed to send any requests or events to the client until it has responded with an ‘initialize’ response. ``` More correct ways are either 1) do not depend on RunInTerminal, but directly manage terminals using vscode terminal APIs, or - we cannot utilize the RunInTerminal implementation vscode team actively manages and keeps improving. - there is no public vscode API for external console mode support so we need to implement it from scratch. 2) first respond with an `initialize` response specifying a minimal capability before sending RunInTerminal request. Once `dlv dap` starts, forward the stashed `initialize` request to `dlv dap`. When `dlv dap` returns an `initialize` response, compute the capability difference and report them as an Capabilities event. - add complexity to the DelveDAPOutputDebugAdapter, which will no longer be a thin adapter. - need significant changes in testing we built using DebugClient. - what capabilities changed through Capabilities events are honored is also unspeficied in the spec. 3) Delve DAP implements a proper Debug Adapter including reverse requests. This is the most ideal solution that benefit other DAP-aware editors. - need to agree with delve devs, so may need more time and discussion. 4) ask the vscode team if debug's RunInTerminal capability can be exposed as a vscode API. If that's possible, we even can do it before the initialize request and set up this from the factory or tracker. - new API. no guarantee if this can be accepted. Will investigate the possibility of 4 or 1. If not working, we will work harder to convince delve devs with option 3. Updates #124 Change-Id: I3d28356a3da99832785815f8af43f7443acf8863 Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/358618 Trust: Hyang-Ah Hana Kim <hyangah@gmail.com> Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com> Reviewed-by: Suzy Mueller <suzmue@golang.org>
1 parent f4e5154 commit 943ff11

File tree

2 files changed

+130
-89
lines changed

2 files changed

+130
-89
lines changed

‎src/goDebugFactory.ts

Lines changed: 77 additions & 87 deletions
Original file line numberDiff line numberDiff line change
@@ -228,9 +228,32 @@ export class DelveDAPOutputAdapter extends ProxyDebugAdapter {
228228
private socket: net.Socket;
229229
private terminatedOnError = false;
230230

231+
protected sendMessageToClient(message: vscode.DebugProtocolMessage) {
232+
const m = message as any;
233+
if (m.type === 'request') {
234+
logVerbose(`do not forward reverse request: dropping ${JSON.stringify(m)}`);
235+
return;
236+
}
237+
238+
super.sendMessageToClient(message);
239+
}
240+
231241
protected async sendMessageToServer(message: vscode.DebugProtocolMessage): Promise<void> {
242+
const m = message as any;
243+
if (m.type === 'response') {
244+
logVerbose(`do not forward reverse request response: dropping ${JSON.stringify(m)}`);
245+
return;
246+
}
247+
232248
if (!this.connected) {
233-
this.connected = this.startAndConnectToServer();
249+
if (m.type === 'request' && m.command === 'initialize') {
250+
this.connected = this.launchDelveDAP();
251+
} else {
252+
this.connected = Promise.resolve({
253+
connected: false,
254+
reason: `the first message must be an initialize request, got ${JSON.stringify(m)}`
255+
});
256+
}
234257
}
235258
const { connected, reason } = await this.connected;
236259
if (connected) {
@@ -243,7 +266,7 @@ export class DelveDAPOutputAdapter extends ProxyDebugAdapter {
243266
this.outputEvent('stderr', errMsg);
244267
this.sendMessageToClient(new TerminatedEvent());
245268
}
246-
if ((message as any).type === 'request') {
269+
if (m.type === 'request') {
247270
const req = message as DebugProtocol.Request;
248271
this.sendMessageToClient({
249272
seq: 0,
@@ -299,7 +322,7 @@ export class DelveDAPOutputAdapter extends ProxyDebugAdapter {
299322
});
300323
}
301324

302-
private async startAndConnectToServer() {
325+
private async launchDelveDAP() {
303326
try {
304327
const { dlvDapServer, socket } = await this.startDapServer(this.configuration);
305328

@@ -313,7 +336,7 @@ export class DelveDAPOutputAdapter extends ProxyDebugAdapter {
313336
return { connected: true };
314337
}
315338

316-
private outputEvent(dest: string, output: string, data?: any) {
339+
protected outputEvent(dest: string, output: string, data?: any) {
317340
this.sendMessageToClient(new OutputEvent(output, dest, data));
318341
}
319342

@@ -339,7 +362,7 @@ export class DelveDAPOutputAdapter extends ProxyDebugAdapter {
339362
!dlvExternallyLaunched &&
340363
(configuration.console === 'integrated' || configuration.console === 'external')
341364
) {
342-
return startDAPServerWithClientAddrFlag(configuration, log, logErr, logConsole);
365+
return this.startDAPServerWithClientAddrFlag(configuration, logErr);
343366
}
344367
const host = configuration.host || '127.0.0.1';
345368
const port = configuration.port || (await getPort());
@@ -363,104 +386,71 @@ export class DelveDAPOutputAdapter extends ProxyDebugAdapter {
363386

364387
return { dlvDapServer, socket };
365388
}
366-
}
367389

368-
async function startDAPServerWithClientAddrFlag(
369-
launchAttachArgs: vscode.DebugConfiguration,
370-
log: (msg: string) => void,
371-
logErr: (msg: string) => void,
372-
logConsole: (msg: string) => void
373-
): Promise<{ dlvDapServer?: ChildProcessWithoutNullStreams; socket: net.Socket }> {
374-
const { dlvArgs, dlvPath, dir, env } = getSpawnConfig(launchAttachArgs, logErr);
390+
async startDAPServerWithClientAddrFlag(
391+
launchAttachArgs: vscode.DebugConfiguration,
392+
logErr: (msg: string) => void
393+
): Promise<{ dlvDapServer?: ChildProcessWithoutNullStreams; socket: net.Socket }> {
394+
// This is called only when launchAttachArgs.console === 'integrated' | 'external' currently.
395+
const console = (launchAttachArgs as any).console || 'integrated';
375396

376-
// logDest - unlike startDAPServer that relies on piping log messages to a file descriptor
377-
// using --log-dest, we can pass the user-specified logDest directly to the flag.
378-
const logDest = launchAttachArgs.logDest;
379-
if (logDest) {
380-
dlvArgs.push(`--log-dest=${logDest}`);
381-
}
397+
const { dlvArgs, dlvPath, dir, env } = getSpawnConfig(launchAttachArgs, logErr);
382398

383-
const port = await getPort();
384-
// TODO(hyangah): In module-module workspace mode, the program should be build in the super module directory
385-
// where go.work (gopls.mod) file is present. Where dlv runs determines the build directory currently. Two options:
386-
// 1) launch dlv in the super-module module directory and adjust launchArgs.cwd (--wd).
387-
// 2) introduce a new buildDir launch attribute.
388-
return new Promise<{ dlvDapServer: ChildProcessWithoutNullStreams; socket: net.Socket }>((resolve, reject) => {
389-
let s: net.Server = undefined; // temporary server that waits for p to connect to.
390-
let p: ChildProcessWithoutNullStreams = undefined; // dlv dap
399+
// logDest - unlike startDAPServer that relies on piping log messages to a file descriptor
400+
// using --log-dest, we can pass the user-specified logDest directly to the flag.
401+
const logDest = launchAttachArgs.logDest;
402+
if (logDest) {
403+
dlvArgs.push(`--log-dest=${logDest}`);
404+
}
391405

392-
const timeoutToken: NodeJS.Timer = setTimeout(() => {
406+
try {
407+
const port = await getPort();
408+
const rendezvousServerPromise = waitForDAPServer(port, 30_000);
409+
410+
dlvArgs.push(`--client-addr=:${port}`);
411+
412+
dlvArgs.unshift(dlvPath);
413+
super.sendMessageToClient({
414+
seq: 0,
415+
type: 'request',
416+
command: 'runInTerminal',
417+
arguments: {
418+
kind: console,
419+
title: `Go Debug Terminal (${launchAttachArgs.name})`,
420+
cwd: dir,
421+
args: dlvArgs,
422+
env: env
423+
}
424+
});
425+
const socket = await rendezvousServerPromise;
426+
return { socket };
427+
} catch (err) {
428+
logErr(`Failed to launch dlv: ${err}`);
429+
throw new Error('cannot launch dlv dap. See DEBUG CONSOLE');
430+
}
431+
}
432+
}
433+
434+
function waitForDAPServer(port: number, timeoutMs: number): Promise<net.Socket> {
435+
return new Promise((resolve, reject) => {
436+
let s: net.Server = undefined;
437+
const timeoutToken = setTimeout(() => {
393438
if (s?.listening) {
394439
s.close();
395440
}
396-
if (p) {
397-
p.kill('SIGINT');
398-
}
399-
reject(new Error('timed out while waiting for DAP server to start'));
400-
}, 30_000);
441+
reject(new Error('timed out while waiting for DAP in reverse mode to connect'));
442+
}, timeoutMs);
401443

402444
s = net.createServer({ pauseOnConnect: true }, (socket) => {
403-
if (!p) {
404-
logVerbose('unexpected connection, ignoring...');
405-
socket.resume();
406-
socket.destroy(new Error('unexpected connection'));
407-
return;
408-
}
409445
logVerbose(`connected: ${port}`);
410446
clearTimeout(timeoutToken);
411447
s.close(); // accept no more connection
412448
socket.resume();
413-
resolve({ dlvDapServer: p, socket });
414-
});
415-
s.on('close', () => {
416-
logVerbose('server is closed');
449+
resolve(socket);
417450
});
418-
s.on('error', (err) => {
419-
logVerbose(`server got error ${err}`);
420-
reject(err);
421-
});
422-
451+
s.on('error', (err) => reject(err));
423452
s.maxConnections = 1;
424-
425453
s.listen(port);
426-
427-
dlvArgs.push(`--client-addr=:${port}`);
428-
429-
logConsole(`Starting: ${dlvPath} ${dlvArgs.join(' ')} from ${dir}\n`);
430-
431-
p = spawn(dlvPath, dlvArgs, {
432-
cwd: dir,
433-
env
434-
});
435-
p.stdout.on('data', (chunk) => {
436-
const msg = chunk.toString();
437-
log(msg);
438-
});
439-
p.stderr.on('data', (chunk) => {
440-
logErr(chunk.toString());
441-
});
442-
p.on('close', (code, signal) => {
443-
// TODO: should we watch 'exit' instead?
444-
445-
// NOTE: log messages here may not appear in DEBUG CONSOLE if the termination of
446-
// the process was triggered by debug adapter's dispose when dlv dap doesn't
447-
// respond to disconnect on time. In that case, it's possible that the session
448-
// is in the middle of teardown and DEBUG CONSOLE isn't accessible. Check
449-
// Go Debug output channel.
450-
if (typeof code === 'number') {
451-
// The process exited on its own.
452-
logConsole(`dlv dap (${p.pid}) exited with code: ${code}\n`);
453-
} else if (code === null && signal) {
454-
logConsole(`dlv dap (${p.pid}) was killed by signal: ${signal}\n`);
455-
} else {
456-
logConsole(`dlv dap (${p.pid}) terminated with code: ${code} signal: ${signal}\n`);
457-
}
458-
});
459-
p.on('error', (err) => {
460-
if (err) {
461-
logConsole(`Error: ${err}\n`);
462-
}
463-
});
464454
});
465455
}
466456

‎test/integration/goDebug.test.ts

Lines changed: 53 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2274,12 +2274,63 @@ class DelveDAPDebugAdapterOnSocket extends proxy.DelveDAPOutputAdapter {
22742274

22752275
// forward to DelveDAPDebugAdapter, which will forward to dlv dap.
22762276
inStream.on('data', (data: Buffer) => this._handleData(data));
2277-
// handle data from DelveDAPDebugAdapter, that's from dlv dap.
2278-
this.onDidSendMessage((m) => this._send(m));
2277+
// DebugClient silently drops reverse requests. Handle runInTerminal request here.
2278+
this.onDidSendMessage((m) => {
2279+
if (this.handleRunInTerminal(m)) {
2280+
return;
2281+
}
2282+
this._send(m);
2283+
});
22792284

22802285
inStream.resume();
22812286
}
22822287

2288+
// handleRunInTerminal spawns the requested command and simulates RunInTerminal
2289+
// handler implementation inside an editor.
2290+
private _dlvInTerminal: cp.ChildProcess;
2291+
private handleRunInTerminal(m: vscode.DebugProtocolMessage) {
2292+
const m0 = m as any;
2293+
if (m0['type'] !== 'request' || m0['command'] !== 'runInTerminal') {
2294+
return false;
2295+
}
2296+
const json = JSON.stringify(m0);
2297+
this.log(`<- server: ${json}`);
2298+
2299+
const resp = {
2300+
seq: 0,
2301+
type: 'response',
2302+
success: false,
2303+
request_seq: m0['seq'],
2304+
command: m0['command'],
2305+
body: {}
2306+
};
2307+
2308+
if (!this._dlvInTerminal && m0['arguments']?.args?.length > 0) {
2309+
const args = m0['arguments'].args as string[];
2310+
const p = cp.spawn(args[0], args.slice(1), {
2311+
cwd: m0['arguments'].cwd,
2312+
env: m0['arguments'].env
2313+
});
2314+
// stdout/stderr are supposed to appear in the terminal, but
2315+
// some of noDebug tests depend on access to stdout/stderr.
2316+
// For those tests, let's pump the output as OutputEvent.
2317+
p.stdout.on('data', (chunk) => {
2318+
this.outputEvent('stdout', chunk.toString());
2319+
});
2320+
p.stderr.on('data', (chunk) => {
2321+
this.outputEvent('stderr', chunk.toString());
2322+
});
2323+
resp.success = true;
2324+
resp.body = { processId: p.pid };
2325+
this._dlvInTerminal = p;
2326+
}
2327+
2328+
this.log(`-> server: ${JSON.stringify(resp)}`);
2329+
this.handleMessage(resp);
2330+
2331+
return true;
2332+
}
2333+
22832334
private _disposed = false;
22842335
public async dispose(timeoutMS?: number) {
22852336
if (this._disposed) {

0 commit comments

Comments
 (0)