Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

debug: fix updating breakpoints while debuggee is running #2128

Merged
merged 7 commits into from
Jan 24, 2019
Merged
60 changes: 50 additions & 10 deletions src/debugAdapter/goDebug.ts
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,7 @@ class GoDebugSession extends LoggingDebugSession {

private _variableHandles: Handles<DebugVariable>;
private breakpoints: Map<string, DebugBreakpoint[]>;
private skipStopEventOnce: boolean; // Editing breakpoints requires halting delve, skip sending Stop Event to VS Code in such cases
private threads: Set<number>;
private debugState: DebuggerState;
private delve: Delve;
Expand All @@ -548,6 +549,7 @@ class GoDebugSession extends LoggingDebugSession {
public constructor(debuggerLinesStartAt1: boolean, isServer: boolean = false) {
super('', debuggerLinesStartAt1, isServer);
this._variableHandles = new Handles<DebugVariable>();
this.skipStopEventOnce = false;
this.threads = new Set<number>();
this.debugState = null;
this.delve = null;
Expand Down Expand Up @@ -694,15 +696,14 @@ class GoDebugSession extends LoggingDebugSession {
return pathToConvert.replace(this.delve.remotePath, this.delve.program).split(this.remotePathSeparator).join(this.localPathSeparator);
}

protected setBreakPointsRequest(response: DebugProtocol.SetBreakpointsResponse, args: DebugProtocol.SetBreakpointsArguments): void {
log('SetBreakPointsRequest');
private setBreakPoints(response: DebugProtocol.SetBreakpointsResponse, args: DebugProtocol.SetBreakpointsArguments): Thenable<void> {
let file = normalizePath(args.source.path);
if (!this.breakpoints.get(file)) {
this.breakpoints.set(file, []);
}
let remoteFile = this.toDebuggerPath(file);

Promise.all(this.breakpoints.get(file).map(existingBP => {
return Promise.all(this.breakpoints.get(file).map(existingBP => {
log('Clearing: ' + existingBP.id);
return this.delve.callPromise('ClearBreakpoint', [this.delve.isApiV1 ? existingBP.id : { Id: existingBP.id }]);
})).then(() => {
Expand Down Expand Up @@ -751,6 +752,26 @@ class GoDebugSession extends LoggingDebugSession {
});
}

protected setBreakPointsRequest(response: DebugProtocol.SetBreakpointsResponse, args: DebugProtocol.SetBreakpointsArguments): void {
log('SetBreakPointsRequest');
if (!this.continueRequestRunning) {
this.setBreakPoints(response, args);
} else {
this.skipStopEventOnce = true;
this.delve.callPromise('Command', [{ name: 'halt' }]).then(() => {
return this.setBreakPoints(response, args).then(() => {
return this.continue(true).then(null, err => {
logError(`Failed to continue delve after halting it to set breakpoints: "${err.toString()}"`);
});
});
}, err => {
this.skipStopEventOnce = false;
logError(err);
return this.sendErrorResponse(response, 2008, 'Failed to halt delve before attempting to set breakpoint: "{e}"', { e: err.toString() });
});
}
}

protected threadsRequest(response: DebugProtocol.ThreadsResponse): void {
if (this.continueRequestRunning) {
// Thread request to delve is syncronous and will block if a previous async continue request didn't return
Expand Down Expand Up @@ -1073,32 +1094,51 @@ class GoDebugSession extends LoggingDebugSession {
this.threads.delete(id);
});

if (this.skipStopEventOnce) {
this.skipStopEventOnce = false;
return;
Copy link
Contributor

@ramya-rao-a ramya-rao-a Jan 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its not clear to me as to why we need to do this...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To silently skip the pause that we introduce just to set the breakpoint and continue right after.

}

let stoppedEvent = new StoppedEvent(reason, this.debugState.currentGoroutine.id);
(<any>stoppedEvent.body).allThreadsStopped = true;
this.sendEvent(stoppedEvent);
log('StoppedEvent("' + reason + '")');
});
}
}

private continueEpoch = 0;
private continueRequestRunning = false;
protected continueRequest(response: DebugProtocol.ContinueResponse): void {
log('ContinueRequest');
private continue(calledWhenSettingBreakpoint?: boolean): Thenable<void> {
this.continueEpoch++;
let closureEpoch = this.continueEpoch;
this.continueRequestRunning = true;
this.delve.call<DebuggerState | CommandOut>('Command', [{ name: 'continue' }], (err, out) => {

const callback = (out) => {
if (closureEpoch === this.continueEpoch) {
this.continueRequestRunning = false;
}
if (err) {
logError('Failed to continue - ' + err.toString());
}
const state = this.delve.isApiV1 ? <DebuggerState>out : (<CommandOut>out).State;
log('continue state', state);
this.debugState = state;
this.handleReenterDebug('breakpoint');
});
};

// If called when setting breakpoint internally, we want the error to bubble up.
const errorCallback = calledWhenSettingBreakpoint ? null : (err) => {
if (err) {
logError('Failed to continue - ' + err.toString());
}
this.handleReenterDebug('breakpoint');
throw err;
};

return this.delve.callPromise('Command', [{ name: 'continue' }]).then(callback, errorCallback);
}

protected continueRequest(response: DebugProtocol.ContinueResponse): void {
log('ContinueRequest');
this.continue();
this.sendResponse(response);
log('ContinueResponse');
}
Expand Down