-
Notifications
You must be signed in to change notification settings - Fork 646
debug: fix 'pause' command #2126
Changes from 5 commits
643e4d6
f7085dd
3648a2e
e56e19c
c15ed9d
bb6661b
fc07840
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -751,15 +751,35 @@ class GoDebugSession extends LoggingDebugSession { | |
}); | ||
} | ||
|
||
private updateThreads(goroutines: DebugGoroutine[]): void { | ||
// Assume we need to stop all the threads we saw before... | ||
let needsToBeStopped = new Set<number>(); | ||
this.threads.forEach(id => needsToBeStopped.add(id)); | ||
for (let goroutine of goroutines) { | ||
// ...but delete from list of threads to stop if we still see it | ||
needsToBeStopped.delete(goroutine.id); | ||
if (!this.threads.has(goroutine.id)) { | ||
// Send started event if it's new | ||
this.sendEvent(new ThreadEvent('started', goroutine.id)); | ||
} | ||
this.threads.add(goroutine.id); | ||
} | ||
// Send existed event if it's no longer there | ||
needsToBeStopped.forEach(id => { | ||
this.sendEvent(new ThreadEvent('exited', id)); | ||
this.threads.delete(id); | ||
}); | ||
} | ||
|
||
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 | ||
response.body = { threads: [] }; | ||
response.body = { threads: [new Thread(1, 'Dummy')] }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you give us some insight on why we have to create a dummy thread here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. VS Code expects that a process consists of at least one thread. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per the spec. |
||
return this.sendResponse(response); | ||
} | ||
log('ThreadsRequest'); | ||
this.delve.call<DebugGoroutine[] | ListGoroutinesOut>('ListGoroutines', [], (err, out) => { | ||
if (this.debugState.exited) { | ||
if (this.debugState && this.debugState.exited) { | ||
// If the program exits very quickly, the initial threadsRequest will complete after it has exited. | ||
// A TerminatedEvent has already been sent. Ignore the err returned in this case. | ||
response.body = { threads: [] }; | ||
|
@@ -772,6 +792,7 @@ class GoDebugSession extends LoggingDebugSession { | |
} | ||
const goroutines = this.delve.isApiV1 ? <DebugGoroutine[]>out : (<ListGoroutinesOut>out).Goroutines; | ||
log('goroutines', goroutines); | ||
this.updateThreads(goroutines); | ||
let threads = goroutines.map(goroutine => | ||
new Thread( | ||
goroutine.id, | ||
|
@@ -1055,23 +1076,10 @@ class GoDebugSession extends LoggingDebugSession { | |
logError('Failed to get threads - ' + err.toString()); | ||
} | ||
const goroutines = this.delve.isApiV1 ? <DebugGoroutine[]>out : (<ListGoroutinesOut>out).Goroutines; | ||
// Assume we need to stop all the threads we saw before... | ||
let needsToBeStopped = new Set<number>(); | ||
this.threads.forEach(id => needsToBeStopped.add(id)); | ||
for (let goroutine of goroutines) { | ||
// ...but delete from list of threads to stop if we still see it | ||
needsToBeStopped.delete(goroutine.id); | ||
if (!this.threads.has(goroutine.id)) { | ||
// Send started event if it's new | ||
this.sendEvent(new ThreadEvent('started', goroutine.id)); | ||
} | ||
this.threads.add(goroutine.id); | ||
this.updateThreads(goroutines); | ||
if (!this.debugState.currentGoroutine && goroutines.length > 0) { | ||
this.debugState.currentGoroutine = goroutines[0]; | ||
} | ||
// Send existed event if it's no longer there | ||
needsToBeStopped.forEach(id => { | ||
this.sendEvent(new ThreadEvent('exited', id)); | ||
this.threads.delete(id); | ||
}); | ||
|
||
let stoppedEvent = new StoppedEvent(reason, this.debugState.currentGoroutine.id); | ||
(<any>stoppedEvent.body).allThreadsStopped = true; | ||
|
@@ -1157,9 +1165,11 @@ class GoDebugSession extends LoggingDebugSession { | |
} | ||
const state = this.delve.isApiV1 ? <DebuggerState>out : (<CommandOut>out).State; | ||
log('pause state', state); | ||
this.sendResponse(response); | ||
log('PauseResponse'); | ||
this.debugState = state; | ||
this.handleReenterDebug('pause'); | ||
}); | ||
this.sendResponse(response); | ||
log('PauseResponse'); | ||
} | ||
|
||
protected evaluateRequest(response: DebugProtocol.EvaluateResponse, args: DebugProtocol.EvaluateArguments): void { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand this correctly, what we are doing here is ensuring
this.threads
only has entries corresponding to existing goroutines.We should either rename this to something that communicates the above or add a comment above the function for the same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest
updateThreads
sounds good to me and is exactly what this routine is doing: it updates the threads list and sends the appropriate notifications to VS Code through Events interface.Can't be more accurate than
updateAndPropagateThreadsListToVsCode
, though sounds redundant to me.