src/goDebugFactory: send SIGINT to delve and avoid treekill
dlv handles SIGINT better than SIGTERM.
And, stop using treekill. Treekill sends signal to dlv and its children
(debugee and their children, and debugserver). It doesn't look like
the debugserver on the mac doesn't seem to finish cleaning up the
target before exiting as a response of SIGINT. The target becomes a
child of the launchd and remains a zombie. Even when debugserver
could handle this gracefully, sending signals to all the children
processes may interfere with debug target's attempt to gracefully
terminate and doesn't seem like a good idea. Trust dlv's termination
handling logic instead.
Testing this beyond manual testing is hard. Instead, just check
whether the temporary binary is removed that is an indirect sign
that delve responded to the signal.
Changed DelveDAPOutputAdapter's dispose to take an optional timeout
parameter - to use it in testing.
And this CL fixes some places in goDebugFactory where null logger object
can cause to crash.
For https://github.com/go-delve/delve/issues/2445
Updates golang/vscode-go#120
Change-Id: I72de903b27b12ed51079d1c21be89c41d8189d8b
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/315151
Trust: Hyang-Ah Hana Kim <hyangah@gmail.com>
Trust: Suzy Mueller <suzmue@golang.org>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
diff --git a/src/goDebugFactory.ts b/src/goDebugFactory.ts
index a970121..720307e 100644
--- a/src/goDebugFactory.ts
+++ b/src/goDebugFactory.ts
@@ -106,13 +106,13 @@
'utf8',
(err) => {
if (err) {
- this.logger.error(`error sending message: ${err}`);
+ this.logger?.error(`error sending message: ${err}`);
this.sendMessageToClient(new TerminatedEvent());
}
}
);
} else {
- this.logger.error(`stream is closed; dropping ${json}`);
+ this.logger?.error(`stream is closed; dropping ${json}`);
}
}
@@ -135,7 +135,7 @@
this.terminated = true;
if (err) {
- this.logger.error(`connection error: ${err}`);
+ this.logger?.error(`connection error: ${err}`);
this.sendMessageToClient(new OutputEvent(`connection error: ${err}\n`, 'console'));
}
this.sendMessageToClient(new TerminatedEvent());
@@ -204,7 +204,7 @@
super.sendMessageToServer(message);
}
- async dispose() {
+ async dispose(timeoutMS?: number) {
// NOTE: OutputEvents from here may not show up in DEBUG CONSOLE
// because the debug session is terminating.
await super.dispose();
@@ -212,7 +212,11 @@
return;
}
+ if (timeoutMS === undefined) {
+ timeoutMS = 1_000;
+ }
const dlvDapServer = this.dlvDapServer;
+ this.dlvDapServer = undefined;
if (!dlvDapServer) {
return;
}
@@ -225,9 +229,9 @@
await new Promise<void>((resolve) => {
const exitTimeoutToken = setTimeout(() => {
this.logger?.error(`dlv dap process (${dlvDapServer.pid}) isn't responding. Killing...`);
- killProcessTree(dlvDapServer);
+ dlvDapServer.kill('SIGINT'); // Don't use treekill but let dlv handle cleaning up the child processes.
resolve();
- }, 1_000);
+ }, timeoutMS);
dlvDapServer.on('exit', (code, signal) => {
clearTimeout(exitTimeoutToken);
this.logger?.error(
diff --git a/test/integration/goDebug.test.ts b/test/integration/goDebug.test.ts
index 665eea2..8628162 100644
--- a/test/integration/goDebug.test.ts
+++ b/test/integration/goDebug.test.ts
@@ -340,7 +340,7 @@
}
d.dispose();
} else {
- dc.stop();
+ dc?.stop();
}
sinon.restore();
});
@@ -1563,6 +1563,52 @@
await Promise.all([dc.disconnectRequest({ restart: false }), dc.waitForEvent('terminated')]);
});
+
+ test('should cleanup when stopped', async function () {
+ if (!isDlvDap || !dlvDapSkipsEnabled) {
+ this.skip();
+ }
+ const PROGRAM = path.join(DATA_ROOT, 'loop');
+ const OUTPUT = path.join(PROGRAM, '_loop_output');
+
+ const config = {
+ name: 'Launch',
+ type: 'go',
+ request: 'launch',
+ mode: 'debug',
+ program: PROGRAM,
+ stopOnEntry: false,
+ output: OUTPUT
+ };
+ const debugConfig = await initializeDebugConfig(config);
+
+ await Promise.all([dc.configurationSequence(), dc.launch(debugConfig)]);
+
+ try {
+ const fsstat = util.promisify(fs.stat);
+ await fsstat(OUTPUT);
+ } catch (e) {
+ assert.fail(`debug output ${OUTPUT} wasn't built: ${e}`);
+ }
+
+ // It's possible dlv-dap doesn't respond. So, don't wait.
+ dc.disconnectRequest({ restart: false });
+ await sleep(10);
+ dlvDapAdapter.dispose(1);
+ dlvDapAdapter = undefined;
+ dc = undefined;
+ await sleep(100); // allow dlv to respond and finish cleanup.
+ let stat: fs.Stats = null;
+ try {
+ const fsstat = util.promisify(fs.stat);
+ stat = await fsstat(OUTPUT);
+ fs.unlinkSync(OUTPUT);
+ } catch (e) {
+ console.log(`output was cleaned ${OUTPUT} ${e}`);
+ }
+ assert.strictEqual(stat, null, `debug output ${OUTPUT} wasn't cleaned up. ${JSON.stringify(stat)}`);
+ console.log('finished');
+ });
});
suite('switch goroutine', () => {
@@ -1945,14 +1991,15 @@
}
private _disposed = false;
- public async dispose() {
+ public async dispose(timeoutMS?: number) {
if (this._disposed) {
return;
}
this._disposed = true;
- this.log('adapter disposed');
+ this.log('adapter disposing');
await this._server.close();
- await super.dispose();
+ await super.dispose(timeoutMS);
+ this.log('adapter disposed');
}
// Code from
@@ -2023,3 +2070,7 @@
this._log.forEach((msg) => console.log(msg));
}
}
+
+function sleep(ms: number) {
+ return new Promise((resolve) => setTimeout(resolve, ms));
+}
diff --git a/test/testdata/loop/loop.go b/test/testdata/loop/loop.go
index 9740987..394e063 100644
--- a/test/testdata/loop/loop.go
+++ b/test/testdata/loop/loop.go
@@ -1,7 +1,7 @@
package main
-
+import "time"
func main() {
for {
- print("Hello")
+ time.Sleep(10*time.Millisecond)
}
}
