debug: fix 'pause' command #2126

xiphon · 2018-11-14T15:45:58Z

Fixes #978

Addressed issues

Should maintain alive threads list and update it on 'ThreadsRequest'
Should update debugState and threads on 'PauseRequest'
Returning an empty array on threads request. VS Code expects that a process consists of at least one thread
Unhandled exception in handleReenterDebug in case of this.debugState.currentGoroutine is unitialized
Unhandled exception in threadsRequest when this.debugState is unitialized

msftclas · 2018-11-14T15:46:10Z

All CLA requirements met.

ramya-rao-a · 2018-11-21T04:35:07Z

Thanks for the PR @xiphon! This is a very much needed feature along with your work in #2128.
Unfortunately, I am little tight on time, so it will take a while for me to look and test the changes. But I'll surely get to this in the next 2 weeks

Nerzal · 2019-01-02T12:47:34Z

@ramya-rao-a did u have time to look at this ? :)

OneOfOne · 2019-01-07T22:00:29Z

Any news about this?

ramya-rao-a · 2019-01-08T04:03:27Z

@OneOfOne, @Nerzal,

Not yet, just back from vacation and catching up on notifications.
Have either of you gotten a chance to try out the changes in this PR?

@segevfiner, @brycekahle, @lggomez Would you mind taking a look at this PR, since all 3 of you have worked in various debugging features of this extension?

OneOfOne · 2019-01-08T17:04:32Z

I rebased it on current master and I can confirm this fixes the pause command, however I hit #2133.

jhendrixMSFT · 2019-01-10T19:20:20Z

@xiphon can you please rebase on top of master? Once that's done we can merge this change.

xiphon · 2019-01-12T00:03:47Z

Rebased

ramya-rao-a

@xiphon Thanks again for the PR!

@jhendrixMSFT has reviewed and tried out the changes and they work as expected.

Can you update the description of this PR with details about why the pause feature was not working before and how this PR fixes it?

Since the changes in this PR are not trivial, it would help us in future maintenance to have a record of what changes went in and why.

I would like to mainly cover the below points:

We are now updating this.threads in the threadRequest and ensuring that it only has threads corresponding to current goroutines. What problem did this fix?
In handleReenterDebug, if the current state doesnt have a currentGoroutine, we add the first goroutine to it. What problem did this fix? Why dont we need to update the currentThread here as well?

ramya-rao-a · 2019-01-14T18:04:03Z

src/debugAdapter/goDebug.ts

@@ -751,15 +751,35 @@ class GoDebugSession extends LoggingDebugSession {
 		});
 	}

+	private updateThreads(goroutines: DebugGoroutine[]): void {


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

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.

ramya-rao-a · 2019-01-14T18:04:30Z

src/debugAdapter/goDebug.ts

 	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')] };


Can you give us some insight on why we have to create a dummy thread here?

VS Code expects that a process consists of at least one thread.
I guess it is something like "How would we stop/continue if there is no single thread?" or "How does the process exist if it doesn't have any alive threads?"

Per the spec.
"Even if a debug adapter does not support multiple threads, it must implement the threads request and return a single (dummy) thread."
So since we have no threads yet we must send a dummy thread.

ramya-rao-a · 2019-01-19T00:41:54Z

@xiphon Can you answer the last 2 questions?

We are now updating this.threads in the threadRequest and ensuring that it only has threads corresponding to current goroutines. What problem did this fix?
In handleReenterDebug, if the current state doesnt have a currentGoroutine, we add the first goroutine to it. What problem did this fix? Why dont we need to update the currentThread here as well?

xiphon · 2019-01-19T01:20:46Z

@ramya-rao-a

We are now updating this.threads in the threadRequest and ensuring that it only has threads corresponding to current goroutines. What problem did this fix?

Otherwise we will send to debugger a list containing non-existing/exited goroutines.

In handleReenterDebug, if the current state doesnt have a currentGoroutine, we add the first goroutine to it. What problem did this fix?

Unhandled exception in handleReenterDebug in case of unitialized currentGoroutine, currentGoroutine might be unitialized.

Why dont we need to update the currentThread here as well?

We always update currentThread before any handleReenterDebug call.

ramya-rao-a · 2019-01-19T05:10:10Z

We always update currentThread before any handleReenterDebug call.

currentThread isnt being updated independently anywhere. It gets updated when they entire debug state is updated.

So now that we are specifically updating the goroutineId if the state doesnt have it, I was curious as to why we do that but not update the thread

xiphon force-pushed the fix-debug-pause branch 2 times, most recently from 5461e6b to 41e7373 Compare November 16, 2018 00:45

xiphon changed the title ~~debug: fix 'pause' command and stack frames data fetching~~ debug: fix 'pause' command Nov 16, 2018

jhendrixMSFT approved these changes Jan 10, 2019

View reviewed changes

xiphon force-pushed the fix-debug-pause branch from 41e7373 to c15ed9d Compare January 12, 2019 00:02

xiphon added 5 commits January 12, 2019 03:03

debug: fixed 'ThreadsRequest' handling when debugState is null

643e4d6

debug: update threads state on 'ThreadsRequest'

f7085dd

debug: return dummy thread on 'ThreadsRequest' while running

3648a2e

debug: use any active goroutine when debugState.currentGoroutine is null

e56e19c

debug: update debugState and threads on 'PauseRequest'

c15ed9d

ramya-rao-a reviewed Jan 14, 2019

View reviewed changes

xiphon mentioned this pull request Jan 18, 2019

debug: fix updating breakpoints while debuggee is running #2128

Merged

ramya-rao-a added 2 commits January 27, 2019 22:50

Merge remote-tracking branch 'Microsoft/master' into pr/xiphon/2126

bb6661b

Rename for clarity

fc07840

ramya-rao-a approved these changes Jan 28, 2019

View reviewed changes

ramya-rao-a merged commit 50a47a8 into microsoft:master Jan 28, 2019

ramya-rao-a mentioned this pull request Jan 29, 2019

Test Plan for early Feb (2019) release #2288

Closed

This was referenced Mar 6, 2020

service/dap: Add support for threads request go-delve/delve#1914

Merged

DAP specification should clarify which requests are mandatory microsoft/debug-adapter-protocol#99

Closed

ramya-rao-a mentioned this pull request Apr 5, 2020

debugAdapter: Remove redundant support for thread events #3145

Merged

polinasok mentioned this pull request Apr 7, 2020

debug: "Pause on breakpoint" is shown when pausing with "Pause (F6)" #3158

Closed

polinasok mentioned this pull request Jun 10, 2020

debug: unable to produce stack trace: "unknown goroutine 1" golang/vscode-go#179

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

debug: fix 'pause' command #2126

debug: fix 'pause' command #2126

xiphon commented Nov 14, 2018 •

edited

Loading

msftclas commented Nov 14, 2018 •

edited

Loading

ramya-rao-a commented Nov 21, 2018

Nerzal commented Jan 2, 2019

OneOfOne commented Jan 7, 2019

ramya-rao-a commented Jan 8, 2019

OneOfOne commented Jan 8, 2019

jhendrixMSFT commented Jan 10, 2019

xiphon commented Jan 12, 2019

ramya-rao-a left a comment •

edited

Loading

ramya-rao-a Jan 14, 2019

xiphon Jan 19, 2019

ramya-rao-a Jan 14, 2019

xiphon Jan 19, 2019

jhendrixMSFT Jan 23, 2019

ramya-rao-a commented Jan 19, 2019

xiphon commented Jan 19, 2019

ramya-rao-a commented Jan 19, 2019

debug: fix 'pause' command #2126

debug: fix 'pause' command #2126

Conversation

xiphon commented Nov 14, 2018 • edited Loading

msftclas commented Nov 14, 2018 • edited Loading

ramya-rao-a commented Nov 21, 2018

Nerzal commented Jan 2, 2019

OneOfOne commented Jan 7, 2019

ramya-rao-a commented Jan 8, 2019

OneOfOne commented Jan 8, 2019

jhendrixMSFT commented Jan 10, 2019

xiphon commented Jan 12, 2019

ramya-rao-a left a comment • edited Loading

Choose a reason for hiding this comment

ramya-rao-a Jan 14, 2019

Choose a reason for hiding this comment

xiphon Jan 19, 2019

Choose a reason for hiding this comment

ramya-rao-a Jan 14, 2019

Choose a reason for hiding this comment

xiphon Jan 19, 2019

Choose a reason for hiding this comment

jhendrixMSFT Jan 23, 2019

Choose a reason for hiding this comment

ramya-rao-a commented Jan 19, 2019

xiphon commented Jan 19, 2019

ramya-rao-a commented Jan 19, 2019

xiphon commented Nov 14, 2018 •

edited

Loading

msftclas commented Nov 14, 2018 •

edited

Loading

ramya-rao-a left a comment •

edited

Loading