debug: fix 'pause' command - #2126
Conversation
|
|
5461e6b to
41e7373
Compare
xiphon
changed the title
|
Ramya Rao (@ramya-rao-a) did u have time to look at this ? :) |
|
Any news about this? |
|
Ahmed W. (@OneOfOne), Nerzal (@Nerzal), Not yet, just back from vacation and catching up on notifications. Segev Finer (@segevfiner), Bryce Kahle (@brycekahle), Luis Gabriel Gomez (@lggomez) Would you mind taking a look at this PR, since all 3 of you have worked in various debugging features of this extension? |
|
I rebased it on current master and I can confirm this fixes the pause command, however I hit #2133. |
|
xiphon can you please rebase on top of master? Once that's done we can merge this change. |
41e7373 to
c15ed9d
Compare
|
Rebased |
There was a problem hiding this comment.
xiphon Thanks again for the PR!
Joel Hendrix (@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.threadsin thethreadRequestand ensuring that it only has threads corresponding to current goroutines. What problem did this fix? - In
handleReenterDebug, if the current state doesnt have acurrentGoroutine, we add the first goroutine to it. What problem did this fix? Why dont we need to update thecurrentThreadhere as well?
| }); | ||
| } | ||
|
|
||
| private updateThreads(goroutines: DebugGoroutine[]): void { |
There was a problem hiding this comment.
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.
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.
| 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.
Can you give us some insight on why we have to create a dummy thread here?
There was a problem hiding this comment.
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?"
There was a problem hiding this comment.
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.
|
xiphon Can you answer the last 2 questions?
|
Otherwise we will send to debugger a list containing non-existing/exited goroutines.
Unhandled exception in
We always update |
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 |
6 participants
Fixes #978
Addressed issues
debugStateandthreadson 'PauseRequest'threadsrequest. VS Code expects that a process consists of at least one threadhandleReenterDebugin case ofthis.debugState.currentGoroutineis unitializedthreadsRequestwhenthis.debugStateis unitialized