From 659280d5e532a9aa90954fbd81a136974a083b5d Mon Sep 17 00:00:00 2001 From: Vlad Barosan Date: Thu, 14 Feb 2019 16:26:39 -0800 Subject: [PATCH 01/15] Execute detach even when halt fails --- src/debugAdapter/goDebug.ts | 84 ++++++++++++++++++------------------- 1 file changed, 41 insertions(+), 43 deletions(-) diff --git a/src/debugAdapter/goDebug.ts b/src/debugAdapter/goDebug.ts index 30cb5b078..06751d010 100644 --- a/src/debugAdapter/goDebug.ts +++ b/src/debugAdapter/goDebug.ts @@ -479,12 +479,11 @@ class Delve { }); } - callPromise(command: string, args: any[]): Thenable { + public callPromise(command: string, args: any[]): Thenable { return new Promise((resolve, reject) => { this.connection.then(conn => { - conn.call('RPCServer.' + command, args, (err, res) => { - if (err) return reject(err); - resolve(res); + conn.call(`RPCServer.${command}`, args, (err, res) => { + return err ? reject(err) : resolve(res); }); }, err => { reject(err); @@ -492,56 +491,55 @@ class Delve { }); } - close(): Thenable { + public close(): Thenable { if (this.noDebug) { // delve isn't running so no need to halt return Promise.resolve(); } log('HaltRequest'); - return new Promise(resolve => { - let timeoutToken: NodeJS.Timer; - if (this.debugProcess) { - timeoutToken = setTimeout(() => { - log('Killing debug process manually as we could not halt and detach delve in time'); - killTree(this.debugProcess.pid); - resolve(); - }, 1000); + return new Promise(async resolve => { + const timeoutToken: NodeJS.Timer = this.debugProcess && setTimeout(() => { + log('Killing debug process manually as we could not halt and detach delve in time'); + killTree(this.debugProcess.pid); + resolve(); + }, 1000); + + try { + await this.callPromise('Command', [{ name: 'halt' }]); + } + catch (err) { + const errMsg = err ? err.toString() : ''; + log(`Failed to halt - ${errMsg}`); + } + + if (timeoutToken) { + clearTimeout(timeoutToken); } - this.callPromise('Command', [{ name: 'halt' }]).then(() => { - if (timeoutToken) { - clearTimeout(timeoutToken); + log('HaltResponse'); + if (!this.debugProcess) { + log('RestartRequest'); + try { + await this.callPromise('Restart', this.isApiV1 ? [] : [{ position: '', resetArgs: false, newArgs: [] }]); } - log('HaltResponse'); - if (!this.debugProcess) { - log('RestartRequest'); - return this.callPromise('Restart', this.isApiV1 ? [] : [{ position: '', resetArgs: false, newArgs: [] }]) - .then(null, err => { - log('RestartResponse'); - logError(`Failed to restart - ${(err || '').toString()}`); - }) - .then(() => resolve()); - } else { - log('DetachRequest'); - return this.callPromise('Detach', [this.isApiV1 ? true : { Kill: true }]) - .then(null, err => { - log('DetachResponse'); - logError(`Killing debug process manually as we failed to detach - ${(err || '').toString()}`); - killTree(this.debugProcess.pid); - }) - .then(() => resolve()); + catch (err) { + log('RestartResponse'); + logError(`Failed to restart - ${(err || '').toString()}`); } - }, err => { - const errMsg = err ? err.toString() : ''; - log('Failed to halt - ' + errMsg.toString()); - if (errMsg.endsWith('has exited with status 0')) { - if (timeoutToken) { - clearTimeout(timeoutToken); - } - return resolve(); + return resolve(); + } else { + log('DetachRequest'); + try { + await this.callPromise('Detach', [this.isApiV1 ? true : { Kill: true }]); } - }); + catch (err) { + log('DetachResponse'); + logError(`Killing debug process manually as we failed to detach - ${(err.toString() || '')}`); + killTree(this.debugProcess.pid); + } + return resolve(); + } }); } } From c022731b111c1ff68f75acbe07dbd61fc0cd69f4 Mon Sep 17 00:00:00 2001 From: Vlad Barosan Date: Tue, 19 Feb 2019 13:51:04 -0800 Subject: [PATCH 02/15] Include detach in the timeout for closing the debugger --- src/debugAdapter/goDebug.ts | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/src/debugAdapter/goDebug.ts b/src/debugAdapter/goDebug.ts index 06751d010..3f4f8db92 100644 --- a/src/debugAdapter/goDebug.ts +++ b/src/debugAdapter/goDebug.ts @@ -491,6 +491,11 @@ class Delve { }); } + /** + * To close the debugging session we do the following; If its a local process, + * we try to halt and detach within a timeout otherwise we kill the debugger. If its a remote + * process we try to halt and restart the remote target process running under the debugger. + */ public close(): Thenable { if (this.noDebug) { // delve isn't running so no need to halt @@ -513,30 +518,26 @@ class Delve { log(`Failed to halt - ${errMsg}`); } - if (timeoutToken) { - clearTimeout(timeoutToken); - } - log('HaltResponse'); - if (!this.debugProcess) { - log('RestartRequest'); + if (this.debugProcess) { + log('DetachRequest'); try { - await this.callPromise('Restart', this.isApiV1 ? [] : [{ position: '', resetArgs: false, newArgs: [] }]); + await this.callPromise('Detach', [this.isApiV1 ? true : { Kill: true }]); + clearTimeout(timeoutToken); } catch (err) { - log('RestartResponse'); - logError(`Failed to restart - ${(err || '').toString()}`); + log('DetachResponse'); + logError(`The timeout will kill the debug process manually as we failed to detach - ${(err.toString() || '')}`); } return resolve(); } else { - log('DetachRequest'); + log('RestartRequest'); try { - await this.callPromise('Detach', [this.isApiV1 ? true : { Kill: true }]); + await this.callPromise('Restart', this.isApiV1 ? [] : [{ position: '', resetArgs: false, newArgs: [] }]); } catch (err) { - log('DetachResponse'); - logError(`Killing debug process manually as we failed to detach - ${(err.toString() || '')}`); - killTree(this.debugProcess.pid); + log('RestartResponse'); + logError(`Failed to restart - ${(err || '').toString()}`); } return resolve(); } From 298c49978501ea9cd4d34d3f218a44136d9d49f4 Mon Sep 17 00:00:00 2001 From: Vlad Barosan Date: Tue, 5 Mar 2019 13:33:51 -0800 Subject: [PATCH 03/15] update based on feedback --- src/debugAdapter/goDebug.ts | 45 ++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/src/debugAdapter/goDebug.ts b/src/debugAdapter/goDebug.ts index 3f4f8db92..7cb9e13d2 100644 --- a/src/debugAdapter/goDebug.ts +++ b/src/debugAdapter/goDebug.ts @@ -357,7 +357,7 @@ class Delve { logError('Process exiting with code: ' + code); if (this.onclose) { this.onclose(code); } }); - this.debugProcess.on('error', function(err) { + this.debugProcess.on('error', (err) => { reject(err); }); resolve(); @@ -465,7 +465,7 @@ class Delve { logError('Process exiting with code: ' + code); if (this.onclose) { this.onclose(code); } }); - this.debugProcess.on('error', function(err) { + this.debugProcess.on('error', (err) => { reject(err); }); }); @@ -504,45 +504,44 @@ class Delve { log('HaltRequest'); return new Promise(async resolve => { - const timeoutToken: NodeJS.Timer = this.debugProcess && setTimeout(() => { - log('Killing debug process manually as we could not halt and detach delve in time'); + const timeoutToken: NodeJS.Timer = !this.isRemoteDebugging() && setTimeout(() => { + log('Killing debug process manually as we could not halt delve in time'); killTree(this.debugProcess.pid); resolve(); }, 1000); + let errMsg; try { await this.callPromise('Command', [{ name: 'halt' }]); - } - catch (err) { - const errMsg = err ? err.toString() : ''; + } catch (err) { + log('HaltResponse'); + errMsg = err ? err.toString() : ''; log(`Failed to halt - ${errMsg}`); } + clearTimeout(timeoutToken); - log('HaltResponse'); - if (this.debugProcess) { + if (this.shouldDetach(errMsg)) { log('DetachRequest'); try { await this.callPromise('Detach', [this.isApiV1 ? true : { Kill: true }]); - clearTimeout(timeoutToken); - } - catch (err) { + } catch (err) { log('DetachResponse'); logError(`The timeout will kill the debug process manually as we failed to detach - ${(err.toString() || '')}`); } - return resolve(); - } else { - log('RestartRequest'); - try { - await this.callPromise('Restart', this.isApiV1 ? [] : [{ position: '', resetArgs: false, newArgs: [] }]); - } - catch (err) { - log('RestartResponse'); - logError(`Failed to restart - ${(err || '').toString()}`); - } - return resolve(); } + return resolve(); }); } + + private isRemoteDebugging() { + return !this.debugProcess; + } + private targetHasExited(errMsg: string) { + return errMsg.endsWith('has exited with status 0'); + } + private shouldDetach(errMsg: string) { + return !errMsg || this.targetHasExited(errMsg); + } } class GoDebugSession extends LoggingDebugSession { From 6222ea4fb5c4cddc86bbad203d7b8123148ca6e4 Mon Sep 17 00:00:00 2001 From: Vlad Barosan Date: Thu, 7 Mar 2019 09:29:11 -0800 Subject: [PATCH 04/15] Remove misleading log --- src/debugAdapter/goDebug.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/debugAdapter/goDebug.ts b/src/debugAdapter/goDebug.ts index 7cb9e13d2..5c5b80c99 100644 --- a/src/debugAdapter/goDebug.ts +++ b/src/debugAdapter/goDebug.ts @@ -526,7 +526,7 @@ class Delve { await this.callPromise('Detach', [this.isApiV1 ? true : { Kill: true }]); } catch (err) { log('DetachResponse'); - logError(`The timeout will kill the debug process manually as we failed to detach - ${(err.toString() || '')}`); + logError(`Failed to detach - ${(err.toString() || '')}`); } } return resolve(); From b2fceb4ea627111525849afeebf2094f61127d19 Mon Sep 17 00:00:00 2001 From: Vlad Barosan Date: Thu, 7 Mar 2019 11:08:24 -0800 Subject: [PATCH 05/15] Ensure local debugee gets removed --- package-lock.json | 8 ++------ package.json | 2 +- src/debugAdapter/goDebug.ts | 27 +++++++++++++++++++++++++-- 3 files changed, 28 insertions(+), 9 deletions(-) diff --git a/package-lock.json b/package-lock.json index 231a3ff74..60ba74ef8 100644 --- a/package-lock.json +++ b/package-lock.json @@ -630,7 +630,6 @@ "version": "7.0.1", "resolved": "https://registry.npmjs.org/fs-extra/-/fs-extra-7.0.1.tgz", "integrity": "sha512-YJDaCJZEnBmcbw13fvdAM9AwNOJwOzrE4pqMqBq5nFiEqXUqHwlK4B+3pUw6JNvfSPtX05xFHtYy/1ni01eGCw==", - "dev": true, "requires": { "graceful-fs": "^4.1.2", "jsonfile": "^4.0.0", @@ -725,8 +724,7 @@ "graceful-fs": { "version": "4.1.15", "resolved": "https://registry.npmjs.org/graceful-fs/-/graceful-fs-4.1.15.tgz", - "integrity": "sha512-6uHUhOPEBgQ24HM+r6b/QwWfZq+yiFcipKFrOFiBEnWdy5sdzYoi+pJeQaPI5qOLRFqWmAXUPQNsielzdLoecA==", - "dev": true + "integrity": "sha512-6uHUhOPEBgQ24HM+r6b/QwWfZq+yiFcipKFrOFiBEnWdy5sdzYoi+pJeQaPI5qOLRFqWmAXUPQNsielzdLoecA==" }, "growl": { "version": "1.10.3", @@ -1175,7 +1173,6 @@ "version": "4.0.0", "resolved": "https://registry.npmjs.org/jsonfile/-/jsonfile-4.0.0.tgz", "integrity": "sha1-h3Gq4HmbZAdrdmQPygWPnBDjPss=", - "dev": true, "requires": { "graceful-fs": "^4.1.6" } @@ -1926,8 +1923,7 @@ "universalify": { "version": "0.1.2", "resolved": "https://registry.npmjs.org/universalify/-/universalify-0.1.2.tgz", - "integrity": "sha512-rBJeI5CXAlmy1pV+617WB9J63U6XcazHHF2f2dbJix4XzpUF0RS3Zbj0FGIOCAva5P/d/GBOYaACQ1w+0azUkg==", - "dev": true + "integrity": "sha512-rBJeI5CXAlmy1pV+617WB9J63U6XcazHHF2f2dbJix4XzpUF0RS3Zbj0FGIOCAva5P/d/GBOYaACQ1w+0azUkg==" }, "uri-js": { "version": "4.2.2", diff --git a/package.json b/package.json index 9a48585ab..629833df1 100644 --- a/package.json +++ b/package.json @@ -37,6 +37,7 @@ "extensionDependencies": [], "dependencies": { "diff": "^3.5.0", + "fs-extra": "^7.0.1", "json-rpc2": "^1.0.2", "vscode-debugadapter": "^1.32.1", "vscode-debugprotocol": "^1.32.0", @@ -47,7 +48,6 @@ "@types/fs-extra": "^5.0.4", "@types/mocha": "^5.2.5", "@types/node": "^6.14.0", - "fs-extra": "^7.0.0", "tslint": "^5.11.0", "typescript": "^3.1.3", "vscode": "^1.1.26" diff --git a/src/debugAdapter/goDebug.ts b/src/debugAdapter/goDebug.ts index 5c5b80c99..7493eea9d 100644 --- a/src/debugAdapter/goDebug.ts +++ b/src/debugAdapter/goDebug.ts @@ -5,13 +5,14 @@ import * as path from 'path'; import * as os from 'os'; +import * as fs from 'fs-extra'; import { DebugProtocol } from 'vscode-debugprotocol'; import { DebugSession, InitializedEvent, TerminatedEvent, ThreadEvent, StoppedEvent, OutputEvent, Thread, StackFrame, Scope, Source, Handles, LoggingDebugSession, Logger, logger } from 'vscode-debugadapter'; import { existsSync, lstatSync } from 'fs'; import { basename, dirname, extname } from 'path'; import { spawn, ChildProcess, execSync, spawnSync, execFile } from 'child_process'; import { Client, RPCConnection } from 'json-rpc2'; -import { parseEnvFile, getBinPathWithPreferredGopath, resolveHomeDir, getInferredGopath, getCurrentGoWorkspaceFromGOPATH, envPath, fixDriveCasingInWindows } from '../goPath'; +import { parseEnvFile, getBinPathWithPreferredGopath, getInferredGopath, getCurrentGoWorkspaceFromGOPATH, envPath, fixDriveCasingInWindows } from '../goPath'; // This enum should stay in sync with https://golang.org/pkg/reflect/#Kind @@ -260,6 +261,7 @@ function normalizePath(filePath: string) { class Delve { program: string; remotePath: string; + localDebugeePath: string | undefined; debugProcess: ChildProcess; loadConfig: LoadConfig; connection: Promise; @@ -391,7 +393,7 @@ class Delve { return reject(`Cannot find Delve debugger. Install from https://github.com/derekparker/delve & ensure it is in your Go tools path, "GOPATH/bin" or "PATH".`); } - let currentGOWorkspace = getCurrentGoWorkspaceFromGOPATH(env['GOPATH'], dirname); + const currentGOWorkspace = getCurrentGoWorkspaceFromGOPATH(env['GOPATH'], dirname); let dlvArgs = [mode || 'debug']; if (mode === 'exec') { dlvArgs = dlvArgs.concat([program]); @@ -436,6 +438,7 @@ class Delve { env, }); + this.localDebugeePath = this.privateGetLocalDebugeePath(launchArgs.output); function connectClient(port: number, host: string) { // Add a slight delay to avoid issues on Linux with // Delve failing calls made shortly after connection. @@ -529,10 +532,30 @@ class Delve { logError(`Failed to detach - ${(err.toString() || '')}`); } } + if (!this.isRemoteDebugging()) { + await this.ensureDebugeeExecutableIsRemoved(); + } return resolve(); }); } + private privateGetLocalDebugeePath(output: string | undefined): string { + const configOutput = output || "debug" + return path.isAbsolute(configOutput) + ? configOutput + : path.resolve(this.program, configOutput) + } + + private async ensureDebugeeExecutableIsRemoved(): Promise { + try { + if (this.localDebugeePath && await fs.pathExists(this.localDebugeePath)) { + await fs.remove(this.localDebugeePath); + } + } catch (e) { + logError(`Failed to potentially remove leftover debug file ${this.localDebugeePath} - ${e.toString() || ""}`) + } + } + private isRemoteDebugging() { return !this.debugProcess; } From 823cd2d0ac2094bde854fa4058022bfc6be89940 Mon Sep 17 00:00:00 2001 From: Vlad Barosan Date: Thu, 7 Mar 2019 11:33:38 -0800 Subject: [PATCH 06/15] Fix test --- src/debugAdapter/goDebug.ts | 6 +++--- test/go.test.ts | 26 +++++++++++--------------- 2 files changed, 14 insertions(+), 18 deletions(-) diff --git a/src/debugAdapter/goDebug.ts b/src/debugAdapter/goDebug.ts index 7493eea9d..a9dbf116f 100644 --- a/src/debugAdapter/goDebug.ts +++ b/src/debugAdapter/goDebug.ts @@ -540,10 +540,10 @@ class Delve { } private privateGetLocalDebugeePath(output: string | undefined): string { - const configOutput = output || "debug" + const configOutput = output || 'debug'; return path.isAbsolute(configOutput) ? configOutput - : path.resolve(this.program, configOutput) + : path.resolve(this.program, configOutput); } private async ensureDebugeeExecutableIsRemoved(): Promise { @@ -552,7 +552,7 @@ class Delve { await fs.remove(this.localDebugeePath); } } catch (e) { - logError(`Failed to potentially remove leftover debug file ${this.localDebugeePath} - ${e.toString() || ""}`) + logError(`Failed to potentially remove leftover debug file ${this.localDebugeePath} - ${e.toString() || ''}`); } } diff --git a/test/go.test.ts b/test/go.test.ts index dfe2fd1e5..4a9243ea3 100644 --- a/test/go.test.ts +++ b/test/go.test.ts @@ -544,22 +544,18 @@ It returns the number of bytes written and any write error encountered. test('Test Outline document symbols', (done) => { const uri = vscode.Uri.file(path.join(fixturePath, 'outlineTest', 'test.go')); vscode.workspace.openTextDocument(uri).then(document => { - new GoDocumentSymbolProvider().provideDocumentSymbols(document, null).then(symbols => { - const groupedSymbolNames = symbols.reduce((map: any, symbol) => { - map[symbol.kind] = (map[symbol.kind] || []).concat([symbol.name]); - return map; - }, {}); - - const packageNames = groupedSymbolNames[vscode.SymbolKind.Package]; - const variableNames = groupedSymbolNames[vscode.SymbolKind.Variable]; - const functionNames = groupedSymbolNames[vscode.SymbolKind.Function]; - const structs = groupedSymbolNames[vscode.SymbolKind.Struct]; - assert.equal(packageNames[0], 'main'); - assert.equal(variableNames, undefined); - assert.equal(functionNames[0], 'print'); - assert.equal(functionNames[1], 'main'); + new GoDocumentSymbolProvider().provideDocumentSymbols(document, null).then(outlines => { + const packages = outlines.filter(x => x.kind === vscode.SymbolKind.Package); + const variables = outlines[0].children.filter((x: any) => x.kind === vscode.SymbolKind.Variable); + const functions = outlines[0].children.filter((x: any) => x.kind === vscode.SymbolKind.Function); + const structs = outlines[0].children.filter((x: any) => x.kind === vscode.SymbolKind.Struct); + + assert.equal(packages[0].name, 'main'); + assert.equal(variables.length, 0); + assert.equal(functions[0].name, 'print'); + assert.equal(functions[1].name, 'main'); assert.equal(structs.length, 1); - assert.equal(structs[0], 'foo'); + assert.equal(structs[0].name, 'foo'); }); }).then(() => done(), done); }); From 46bc0c7f1f429dda755e44a781e47171060e5faf Mon Sep 17 00:00:00 2001 From: Vlad Barosan Date: Thu, 7 Mar 2019 19:56:46 -0800 Subject: [PATCH 07/15] Use fs module directly --- package-lock.json | 16 ++++++++++------ package.json | 4 ++-- src/debugAdapter/goDebug.ts | 14 +++++++++++--- 3 files changed, 23 insertions(+), 11 deletions(-) diff --git a/package-lock.json b/package-lock.json index 60ba74ef8..69b7b5e4f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -20,9 +20,9 @@ "dev": true }, "@types/node": { - "version": "6.14.2", - "resolved": "https://registry.npmjs.org/@types/node/-/node-6.14.2.tgz", - "integrity": "sha512-JWB3xaVfsfnFY8Ofc9rTB/op0fqqTSqy4vBcVk1LuRJvta7KTX+D//fCkiTMeLGhdr2EbFZzQjC97gvmPilk9Q==", + "version": "8.10.43", + "resolved": "https://registry.npmjs.org/@types/node/-/node-8.10.43.tgz", + "integrity": "sha512-5m5W13HR2k3cu88mpzlnPBBv5+GyMHtj4F0P83RG4mqoC0AYVYHVMHfF3SgwKNtqEZiZQASMxU92QsLEekKcnw==", "dev": true }, "ajv": { @@ -525,7 +525,7 @@ }, "event-stream": { "version": "3.3.4", - "resolved": "https://registry.npmjs.org/event-stream/-/event-stream-3.3.4.tgz", + "resolved": "http://registry.npmjs.org/event-stream/-/event-stream-3.3.4.tgz", "integrity": "sha1-SrTJoPWlTbkzi0w02Gv86PSzVXE=", "dev": true, "requires": { @@ -630,6 +630,7 @@ "version": "7.0.1", "resolved": "https://registry.npmjs.org/fs-extra/-/fs-extra-7.0.1.tgz", "integrity": "sha512-YJDaCJZEnBmcbw13fvdAM9AwNOJwOzrE4pqMqBq5nFiEqXUqHwlK4B+3pUw6JNvfSPtX05xFHtYy/1ni01eGCw==", + "dev": true, "requires": { "graceful-fs": "^4.1.2", "jsonfile": "^4.0.0", @@ -724,7 +725,8 @@ "graceful-fs": { "version": "4.1.15", "resolved": "https://registry.npmjs.org/graceful-fs/-/graceful-fs-4.1.15.tgz", - "integrity": "sha512-6uHUhOPEBgQ24HM+r6b/QwWfZq+yiFcipKFrOFiBEnWdy5sdzYoi+pJeQaPI5qOLRFqWmAXUPQNsielzdLoecA==" + "integrity": "sha512-6uHUhOPEBgQ24HM+r6b/QwWfZq+yiFcipKFrOFiBEnWdy5sdzYoi+pJeQaPI5qOLRFqWmAXUPQNsielzdLoecA==", + "dev": true }, "growl": { "version": "1.10.3", @@ -1173,6 +1175,7 @@ "version": "4.0.0", "resolved": "https://registry.npmjs.org/jsonfile/-/jsonfile-4.0.0.tgz", "integrity": "sha1-h3Gq4HmbZAdrdmQPygWPnBDjPss=", + "dev": true, "requires": { "graceful-fs": "^4.1.6" } @@ -1923,7 +1926,8 @@ "universalify": { "version": "0.1.2", "resolved": "https://registry.npmjs.org/universalify/-/universalify-0.1.2.tgz", - "integrity": "sha512-rBJeI5CXAlmy1pV+617WB9J63U6XcazHHF2f2dbJix4XzpUF0RS3Zbj0FGIOCAva5P/d/GBOYaACQ1w+0azUkg==" + "integrity": "sha512-rBJeI5CXAlmy1pV+617WB9J63U6XcazHHF2f2dbJix4XzpUF0RS3Zbj0FGIOCAva5P/d/GBOYaACQ1w+0azUkg==", + "dev": true }, "uri-js": { "version": "4.2.2", diff --git a/package.json b/package.json index 629833df1..d3eb855ef 100644 --- a/package.json +++ b/package.json @@ -37,7 +37,6 @@ "extensionDependencies": [], "dependencies": { "diff": "^3.5.0", - "fs-extra": "^7.0.1", "json-rpc2": "^1.0.2", "vscode-debugadapter": "^1.32.1", "vscode-debugprotocol": "^1.32.0", @@ -47,7 +46,8 @@ "devDependencies": { "@types/fs-extra": "^5.0.4", "@types/mocha": "^5.2.5", - "@types/node": "^6.14.0", + "@types/node": "^8.10.43", + "fs-extra": "^7.0.1", "tslint": "^5.11.0", "typescript": "^3.1.3", "vscode": "^1.1.26" diff --git a/src/debugAdapter/goDebug.ts b/src/debugAdapter/goDebug.ts index a9dbf116f..e7f592993 100644 --- a/src/debugAdapter/goDebug.ts +++ b/src/debugAdapter/goDebug.ts @@ -5,7 +5,8 @@ import * as path from 'path'; import * as os from 'os'; -import * as fs from 'fs-extra'; +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 { existsSync, lstatSync } from 'fs'; @@ -14,6 +15,10 @@ import { spawn, ChildProcess, execSync, spawnSync, execFile } from 'child_proces import { Client, RPCConnection } from 'json-rpc2'; import { parseEnvFile, getBinPathWithPreferredGopath, getInferredGopath, getCurrentGoWorkspaceFromGOPATH, envPath, fixDriveCasingInWindows } from '../goPath'; +const access = util.promisify(fs.access); +const unlink = util.promisify(fs.unlink); + + // This enum should stay in sync with https://golang.org/pkg/reflect/#Kind enum GoReflectKind { @@ -548,8 +553,11 @@ class Delve { private async ensureDebugeeExecutableIsRemoved(): Promise { try { - if (this.localDebugeePath && await fs.pathExists(this.localDebugeePath)) { - await fs.remove(this.localDebugeePath); + const fileExists = await access(this.localDebugeePath) + .then(() => true) + .catch(() => false); + if (this.localDebugeePath && fileExists) { + await unlink(this.localDebugeePath); } } catch (e) { logError(`Failed to potentially remove leftover debug file ${this.localDebugeePath} - ${e.toString() || ''}`); From a3f18b1b133315f3322c4e7f79516469931cd9c2 Mon Sep 17 00:00:00 2001 From: Vlad Barosan Date: Fri, 8 Mar 2019 10:57:56 -0800 Subject: [PATCH 08/15] Ensure file removal in timeout --- src/debugAdapter/goDebug.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/debugAdapter/goDebug.ts b/src/debugAdapter/goDebug.ts index e7f592993..c3613eb7a 100644 --- a/src/debugAdapter/goDebug.ts +++ b/src/debugAdapter/goDebug.ts @@ -515,6 +515,7 @@ class Delve { const timeoutToken: NodeJS.Timer = !this.isRemoteDebugging() && setTimeout(() => { log('Killing debug process manually as we could not halt delve in time'); killTree(this.debugProcess.pid); + this.ensureDebugeeExecutableIsRemoved(); resolve(); }, 1000); From 1eb5e9636e1d140a6d2c06a15b9300cf046c7f16 Mon Sep 17 00:00:00 2001 From: Vlad Barosan Date: Fri, 8 Mar 2019 13:19:03 -0800 Subject: [PATCH 09/15] remove one time use functions --- src/debugAdapter/goDebug.ts | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/src/debugAdapter/goDebug.ts b/src/debugAdapter/goDebug.ts index c3613eb7a..68f8ebb85 100644 --- a/src/debugAdapter/goDebug.ts +++ b/src/debugAdapter/goDebug.ts @@ -511,8 +511,10 @@ class Delve { } log('HaltRequest'); + const isLocalDebugging = this.debugProcess; + return new Promise(async resolve => { - const timeoutToken: NodeJS.Timer = !this.isRemoteDebugging() && setTimeout(() => { + const timeoutToken: NodeJS.Timer = isLocalDebugging && setTimeout(() => { log('Killing debug process manually as we could not halt delve in time'); killTree(this.debugProcess.pid); this.ensureDebugeeExecutableIsRemoved(); @@ -529,7 +531,10 @@ class Delve { } clearTimeout(timeoutToken); - if (this.shouldDetach(errMsg)) { + const targetHasExited = errMsg.endsWith('has exited with status 0'); + const shouldDetach = !errMsg || targetHasExited; + + if (shouldDetach(errMsg)) { log('DetachRequest'); try { await this.callPromise('Detach', [this.isApiV1 ? true : { Kill: true }]); @@ -538,7 +543,7 @@ class Delve { logError(`Failed to detach - ${(err.toString() || '')}`); } } - if (!this.isRemoteDebugging()) { + if (isLocalDebugging) { await this.ensureDebugeeExecutableIsRemoved(); } return resolve(); @@ -564,16 +569,6 @@ class Delve { logError(`Failed to potentially remove leftover debug file ${this.localDebugeePath} - ${e.toString() || ''}`); } } - - private isRemoteDebugging() { - return !this.debugProcess; - } - private targetHasExited(errMsg: string) { - return errMsg.endsWith('has exited with status 0'); - } - private shouldDetach(errMsg: string) { - return !errMsg || this.targetHasExited(errMsg); - } } class GoDebugSession extends LoggingDebugSession { From 6836de6bfb89a968d71ccbd25a7279acec381e6d Mon Sep 17 00:00:00 2001 From: Vlad Barosan Date: Fri, 8 Mar 2019 17:40:29 -0800 Subject: [PATCH 10/15] Update logic per latest feedback --- src/debugAdapter/goDebug.ts | 49 ++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/src/debugAdapter/goDebug.ts b/src/debugAdapter/goDebug.ts index 68f8ebb85..09264bdda 100644 --- a/src/debugAdapter/goDebug.ts +++ b/src/debugAdapter/goDebug.ts @@ -512,12 +512,14 @@ class Delve { log('HaltRequest'); const isLocalDebugging = this.debugProcess; - + const forceCleanup = async () => { + killTree(this.debugProcess.pid); + await removeFile(this.localDebugeePath); + }; return new Promise(async resolve => { - const timeoutToken: NodeJS.Timer = isLocalDebugging && setTimeout(() => { + const timeoutToken: NodeJS.Timer = isLocalDebugging && setTimeout(async () => { log('Killing debug process manually as we could not halt delve in time'); - killTree(this.debugProcess.pid); - this.ensureDebugeeExecutableIsRemoved(); + await forceCleanup(); resolve(); }, 1000); @@ -531,20 +533,21 @@ class Delve { } clearTimeout(timeoutToken); - const targetHasExited = errMsg.endsWith('has exited with status 0'); - const shouldDetach = !errMsg || targetHasExited; - - if (shouldDetach(errMsg)) { + const targetHasExited: boolean = errMsg.endsWith('has exited with status 0'); + const shouldDetach: boolean = !errMsg || targetHasExited; + let shouldForceClean: boolean = !shouldDetach; + if (shouldDetach) { log('DetachRequest'); try { await this.callPromise('Detach', [this.isApiV1 ? true : { Kill: true }]); } catch (err) { log('DetachResponse'); logError(`Failed to detach - ${(err.toString() || '')}`); + shouldForceClean = true; } } - if (isLocalDebugging) { - await this.ensureDebugeeExecutableIsRemoved(); + if (isLocalDebugging && shouldForceClean) { + await forceCleanup(); } return resolve(); }); @@ -556,19 +559,6 @@ class Delve { ? configOutput : path.resolve(this.program, configOutput); } - - private async ensureDebugeeExecutableIsRemoved(): Promise { - try { - const fileExists = await access(this.localDebugeePath) - .then(() => true) - .catch(() => false); - if (this.localDebugeePath && fileExists) { - await unlink(this.localDebugeePath); - } - } catch (e) { - logError(`Failed to potentially remove leftover debug file ${this.localDebugeePath} - ${e.toString() || ''}`); - } - } } class GoDebugSession extends LoggingDebugSession { @@ -1431,4 +1421,17 @@ function killTree(processId: number): void { } } +async function removeFile(filePath: string): Promise { + try { + const fileExists = await access(filePath) + .then(() => true) + .catch(() => false); + if (filePath && fileExists) { + await unlink(filePath); + } + } catch (e) { + logError(`Potentially failed remove file: ${filePath} - ${e.toString() || ''}`); + } +} + DebugSession.run(GoDebugSession); From 5ce4a9217dd736f94675763a81c712ae41f38525 Mon Sep 17 00:00:00 2001 From: Ramya Achutha Rao Date: Sun, 10 Mar 2019 18:08:52 -0700 Subject: [PATCH 11/15] Small refactorings --- src/debugAdapter/goDebug.ts | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/src/debugAdapter/goDebug.ts b/src/debugAdapter/goDebug.ts index 09264bdda..5220eb79e 100644 --- a/src/debugAdapter/goDebug.ts +++ b/src/debugAdapter/goDebug.ts @@ -15,9 +15,8 @@ import { spawn, ChildProcess, execSync, spawnSync, execFile } from 'child_proces import { Client, RPCConnection } from 'json-rpc2'; import { parseEnvFile, getBinPathWithPreferredGopath, getInferredGopath, getCurrentGoWorkspaceFromGOPATH, envPath, fixDriveCasingInWindows } from '../goPath'; -const access = util.promisify(fs.access); -const unlink = util.promisify(fs.unlink); - +const fsAccess = util.promisify(fs.access); +const fsUnlink = util.promisify(fs.unlink); // This enum should stay in sync with https://golang.org/pkg/reflect/#Kind @@ -443,7 +442,7 @@ class Delve { env, }); - this.localDebugeePath = this.privateGetLocalDebugeePath(launchArgs.output); + this.localDebugeePath = this.getLocalDebugeePath(launchArgs.output); function connectClient(port: number, host: string) { // Add a slight delay to avoid issues on Linux with // Delve failing calls made shortly after connection. @@ -511,7 +510,7 @@ class Delve { } log('HaltRequest'); - const isLocalDebugging = this.debugProcess; + const isLocalDebugging: boolean = !!this.debugProcess; const forceCleanup = async () => { killTree(this.debugProcess.pid); await removeFile(this.localDebugeePath); @@ -523,18 +522,18 @@ class Delve { resolve(); }, 1000); - let errMsg; + let haltErrMsg; try { await this.callPromise('Command', [{ name: 'halt' }]); } catch (err) { log('HaltResponse'); - errMsg = err ? err.toString() : ''; - log(`Failed to halt - ${errMsg}`); + haltErrMsg = err ? err.toString() : ''; + log(`Failed to halt - ${haltErrMsg}`); } clearTimeout(timeoutToken); - const targetHasExited: boolean = errMsg.endsWith('has exited with status 0'); - const shouldDetach: boolean = !errMsg || targetHasExited; + const targetHasExited: boolean = haltErrMsg.endsWith('has exited with status 0'); + const shouldDetach: boolean = !haltErrMsg || targetHasExited; let shouldForceClean: boolean = !shouldDetach; if (shouldDetach) { log('DetachRequest'); @@ -553,7 +552,7 @@ class Delve { }); } - private privateGetLocalDebugeePath(output: string | undefined): string { + private getLocalDebugeePath(output: string | undefined): string { const configOutput = output || 'debug'; return path.isAbsolute(configOutput) ? configOutput @@ -1423,11 +1422,11 @@ function killTree(processId: number): void { async function removeFile(filePath: string): Promise { try { - const fileExists = await access(filePath) + const fileExists = await fsAccess(filePath) .then(() => true) .catch(() => false); if (filePath && fileExists) { - await unlink(filePath); + await fsUnlink(filePath); } } catch (e) { logError(`Potentially failed remove file: ${filePath} - ${e.toString() || ''}`); From 94f236bf51b43d8f7897e500ee9268f40876664f Mon Sep 17 00:00:00 2001 From: Ramya Achutha Rao Date: Sun, 10 Mar 2019 18:09:51 -0700 Subject: [PATCH 12/15] http -> https for event-stream --- package-lock.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package-lock.json b/package-lock.json index 69b7b5e4f..94332005c 100644 --- a/package-lock.json +++ b/package-lock.json @@ -525,7 +525,7 @@ }, "event-stream": { "version": "3.3.4", - "resolved": "http://registry.npmjs.org/event-stream/-/event-stream-3.3.4.tgz", + "resolved": "https://registry.npmjs.org/event-stream/-/event-stream-3.3.4.tgz", "integrity": "sha1-SrTJoPWlTbkzi0w02Gv86PSzVXE=", "dev": true, "requires": { From 720db1223f3d6a1e3c984d824720fbfb7224bdf7 Mon Sep 17 00:00:00 2001 From: Vlad Barosan Date: Wed, 13 Mar 2019 17:17:04 -0700 Subject: [PATCH 13/15] Don't kill on remote debugging --- src/debugAdapter/goDebug.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/debugAdapter/goDebug.ts b/src/debugAdapter/goDebug.ts index 5220eb79e..56daf1703 100644 --- a/src/debugAdapter/goDebug.ts +++ b/src/debugAdapter/goDebug.ts @@ -532,13 +532,13 @@ class Delve { } clearTimeout(timeoutToken); - const targetHasExited: boolean = haltErrMsg.endsWith('has exited with status 0'); + const targetHasExited: boolean = haltErrMsg && haltErrMsg.endsWith('has exited with status 0'); const shouldDetach: boolean = !haltErrMsg || targetHasExited; let shouldForceClean: boolean = !shouldDetach; if (shouldDetach) { log('DetachRequest'); try { - await this.callPromise('Detach', [this.isApiV1 ? true : { Kill: true }]); + await this.callPromise('Detach', [this.isApiV1 ? true : { Kill: isLocalDebugging }]); } catch (err) { log('DetachResponse'); logError(`Failed to detach - ${(err.toString() || '')}`); From 13a34afd85ae9a96be4c9b580c5069df765e4aec Mon Sep 17 00:00:00 2001 From: Ramya Achutha Rao Date: Thu, 14 Mar 2019 10:15:31 -0700 Subject: [PATCH 14/15] Update comment --- src/debugAdapter/goDebug.ts | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/debugAdapter/goDebug.ts b/src/debugAdapter/goDebug.ts index 56daf1703..20a44ef74 100644 --- a/src/debugAdapter/goDebug.ts +++ b/src/debugAdapter/goDebug.ts @@ -499,9 +499,16 @@ class Delve { } /** - * To close the debugging session we do the following; If its a local process, - * we try to halt and detach within a timeout otherwise we kill the debugger. If its a remote - * process we try to halt and restart the remote target process running under the debugger. + * Closing a debugging session follows different approaches for local vs remote delve process. + * + * For local process, since the extension starts the delve process, the extension should close it as well. + * To gracefully clean up the assets created by delve, we send the Detach request with kill option set to true. + * + * For remote process, since the extension doesnt start the delve process, we only detach from it without killing it. + * + * 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 */ public close(): Thenable { if (this.noDebug) { From 24645cc4d9083c86b08372c8d5b2155637ff09be Mon Sep 17 00:00:00 2001 From: Ramya Achutha Rao Date: Thu, 14 Mar 2019 10:19:03 -0700 Subject: [PATCH 15/15] shouldForceClean should never be true for remote debugging --- src/debugAdapter/goDebug.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/debugAdapter/goDebug.ts b/src/debugAdapter/goDebug.ts index 20a44ef74..2892827eb 100644 --- a/src/debugAdapter/goDebug.ts +++ b/src/debugAdapter/goDebug.ts @@ -541,7 +541,7 @@ class Delve { const targetHasExited: boolean = haltErrMsg && haltErrMsg.endsWith('has exited with status 0'); const shouldDetach: boolean = !haltErrMsg || targetHasExited; - let shouldForceClean: boolean = !shouldDetach; + let shouldForceClean: boolean = !shouldDetach && isLocalDebugging; if (shouldDetach) { log('DetachRequest'); try { @@ -549,10 +549,10 @@ class Delve { } catch (err) { log('DetachResponse'); logError(`Failed to detach - ${(err.toString() || '')}`); - shouldForceClean = true; + shouldForceClean = isLocalDebugging; } } - if (isLocalDebugging && shouldForceClean) { + if (shouldForceClean) { await forceCleanup(); } return resolve();