Fixes #3650: Support stop-all-threads mode debugging for multi-thread… #3990

edumunoz · 2016-03-11T04:14:03Z

As discussed in #3650, this adds support for stop-all-threads mode by adding a flag to the StoppedEvent that the debug adapter sends. Here's the fix in action:

I tested this change with a basic multi-threaded program, and verified:

Expanding threads that are not the one that sent the stopped event triggers a query for their callstack
Already expanded threads get updated when a new stopped event is sent
Node debugging works (including debugging VSCode extensions)
Mono debugging works (handling multiple threads work as before)
Go debugging works (handling multiple threads work as before)
C# debugging works (with the new omnisharp-based extension)

msftclas · 2016-03-11T04:14:07Z

Hi @edumunoz, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

edumunoz · 2016-03-11T04:27:34Z

I changed my username on Github after linking my account with my corporate account. Hence it's asking for CLA.

Anyway of re-triggering msftclas? I have already unlinked and re-linked my account again, so it shows the correct username on my corporate profile.

msftclas · 2016-03-11T08:28:39Z

@edumunoz, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

weinand · 2016-03-11T09:20:50Z

From a debug protocol perspective I suggest to rename the stopAllThreads attribute on the StoppedEvent to allThreadsStopped. stopAllThreads sounds like an action, whereas allThreadsStopped makes clear that this is additional information on the StoppedEvent.

@isidorn could you please review the PR from the front-end perspective.

isidorn · 2016-03-11T15:50:16Z

@edumunoz thanks for the PR.
This code change impacts all debugging scenarios, so most of the debug adapters need to be tested as this can easily break lots of user scenarios - I have added checkboxes to your original comment.
Unfortunetely, we do not have tests for the debug service.

Concerns which I have regarding how model data is handled I will comment directly in the commits.

felixfbecker · 2016-03-11T17:17:29Z

I don't understand the change of this PR. Could someone please explain?

isidorn · 2016-03-11T17:20:48Z

@felixfbecker #3650 has more details

edumunoz · 2016-03-11T19:00:45Z

Thanks for the feedback, @isidorn!

I'll work through it and move the logic to Thread. I left a couple of replies/comments.

@felixfbecker, we wanted to support multi-threading in the debug adapters in a more efficient way.

edumunoz · 2016-03-15T03:46:08Z

@isidorn, I updated the PR addressing your feedback, as well as adding some testing.

For testing the existing debug adapters, on top of what is in brackets (which is the stuff that needs to be tested thoroughly), is there a standard list of stuff to test (e.g., list of stuff for breakpoints, list of stuff for step, etc.)?

isidorn · 2016-03-15T11:17:08Z

@edumunoz great! It looks much better now. I will add some comments directly in your commits.

For testing, I would focus on the things in brackets, a smoke test of 'node' debugging with some focus on clicking in the call stack should do the trick. For 'go' and 'mono' focussing on threads makes most sense since changes here refactor how we deal with threads.
Other debuggers like powershell and chrome should not use additional threads, and just checking 'node' should cover their case. @daviwil can you please confirm if powershell uses multiple threads?

Since the changes also touch the debugEditorModelManager we need to test that all the decorations in the editor did not get broken, these include:

breakpoints, conditional breakpoints, unverified breakpoints, breakpoints jump around
top of call stack (yellow), focused call stack (green), exception in call stack (red)
setting breakpoints in editor work and they get nicely sent to adapter

I guess that you can already build your custom version of code. For testing 'mono' and 'go' you would need to install those extension in your custom built vscode, or just copy over those extensions to your userFolder.vscode-oss\extensions

Here's a part of our smoke test that covers debugging, so doing something like this while focussing on the call stack would be best for 'node'. If you do not have access to out smoketest-express, taking any node app out there should do the trick. E.g https://github.com/bpasero/standup

Debugging JavaScript

• git clone https://monacotools.visualstudio.com/DefaultCollection/Samples/_git/vscode-smoketest-express
    •  in VSCode switch to debugging viewlet
• Click on the gear to create a default launch.json file:
    make sure that VSCode automatically detects ./bin/www as the 'program' attribute
• Select Launch in the dropdown at the top of the debug view
    verify that the drop-down menu shows entries from launch.json
• set a breakpoint in index.js:6
• press F5 to start debugging. Verify:
    workbench transforms into "debug mode" - glyph margin and status bar turns orange
• open browser at http://localhost:3000/ 
    verify the breakpoint in index.js gets hit
• Verify step over, step in, continue work
• while stopped, verify:
    ○ you can hover over values
    ○ clicking on a stack frame shows variables associated with that stack frame
    ○ clicking on a breakpoint takes you to that breakpoint
    ○ you can inspect values of complex variables
    ○ you can add a watch expression
    ○ You can evaluate a simple expression in the debug console
• Verify you can press Restart and the server restarts and you are back in debug mode
   • Verify you can stop the debug session and that workbench exits "debug mode"

daviwil · 2016-03-15T17:25:31Z

@isidorn For now we don't deal with multithreading, not sure if it's something we'll have to support. Thanks for the notification!

cc @rkeithhill

edumunoz · 2016-03-16T02:48:04Z

src/vs/workbench/parts/debug/common/debug.ts

+	 * Indicates whether this thread is stopped. The callstack for stopped
+	 * threads can be retrieved from the debug adapter.
+	 */
+	stopped: boolean;


stopped doesn't necessarily need to be exposed. We could have two methods setRunning and setStopped that would set stopped to false and true, respectively.

I left it exposed so that the tests can check it. I'm fine removing stopped from the interface and only have setters. Let me know what you think.

Having stopped exposed like this is fine I think. It is a general property that other users of Thread might be interested in the future. So I would leave it as it is.

edumunoz · 2016-03-16T02:59:02Z

@isidorn, thanks for your thorough review. In the end I went for exposing a non-promise based callstack in the Thread model. That way the changes to other parts of the codebase are limited to renaming the callstack property.

I left a couple of comments in 2 places to get your opinion.

It would be great if you could give us the heads up today that the code looks good (bar very small changes), so we can spend more time testing the partner debug adapters thoroughly.

Also, let me know if you'd like me to squash the commits after you review them.

isidorn · 2016-03-16T08:43:02Z

Ok, I'll review your newest changes today - and I will also comment on your older questions even though we decided to take the non-promise based callStack in the Thread model. Which I think is a good decision for now to make the spread of changes minimal.

Squashing the commits would be great!

Edit: I went through the commits - and it looks really good to me! I left some comments in the code directly.

Before testing make sure to merge with latest from master so we test the closest possible version to the one when we merge this PR in.

On how to setup Go, Node, Mono in your built VSCode I quickly explained in my comment above, let me know if you have questions regarding this.

isidorn · 2016-03-16T10:20:32Z

src/vs/workbench/parts/debug/browser/debugEditorModelManager.ts

@@ -134,18 +134,18 @@ export class DebugEditorModelManager implements IWorkbenchContribution {
 		const result: editorcommon.IModelDeltaDecoration[] = [];


I like that there are not a lot of changes in this file now. Due to this we will not have to intensively test the UI changes in the editor (e.g. breakpoints).

isidorn · 2016-03-16T10:25:45Z

src/vs/workbench/parts/debug/test/common/debugModel.test.ts

@@ -7,6 +7,11 @@ import assert = require('assert');
 import uri from 'vs/base/common/uri';


Really happy for the tests.

…ti-threaded programs Add a new flag to the StoppedEvent that will trigger the debug adapter to be queried for callstacks of the expanded threads in the callstack viewlet.

edumunoz · 2016-03-17T00:57:12Z

src/vs/workbench/parts/debug/common/debugModel.ts

@@ -624,11 +676,13 @@ export class Model extends ee.EventEmitter implements debug.IModel {

 	public sourceIsUnavailable(source: Source): void {
 		Object.keys(this.threads).forEach(key => {
-			this.threads[key].callStack.forEach(stackFrame => {


For some reason I had missed updating this reference to callStack.

Just pointing out what we have changed today that wasn't in yesterday's commit and you hadn't commented on.

edumunoz · 2016-03-17T01:12:05Z

@isidorn, I addressed your feedback from yesterday.
We performed the testing tasks for different platforms based on the items in Debugging JavaScript *(checked off the items in the description of this PR).

For the existing debuggers, we tried that the behavior remains identical.
For new debuggers that support allThreadsStopped, the multi-threading support kicks in and the experience is as we expected.

Let us know if you need anything else from us.

isidorn · 2016-03-17T08:41:05Z

Looks good to me! Great work - merging!

Fixes #3650: Support stop-all-threads mode debugging for multi-thread…

msftclas added the cla-required label Mar 11, 2016

isidorn self-assigned this Mar 11, 2016

isidorn added this to the March 2016 milestone Mar 11, 2016

msftclas added cla-signed and removed cla-required labels Mar 11, 2016

isidorn mentioned this pull request Mar 11, 2016

Thread exited event ignored #1703

Closed

edumunoz force-pushed the edumunoz/stop-all-debug branch from 5c1233e to 3c10151 Compare March 15, 2016 03:42

edumunoz force-pushed the edumunoz/stop-all-debug branch from 3c10151 to 8f26690 Compare March 15, 2016 07:44

edumunoz force-pushed the edumunoz/stop-all-debug branch 2 times, most recently from a44acce to 29d3a19 Compare March 16, 2016 02:46

edumunoz reviewed Mar 16, 2016
View reviewed changes

isidorn reviewed Mar 16, 2016
View reviewed changes

edumunoz force-pushed the edumunoz/stop-all-debug branch from f18af94 to 9d5f576 Compare March 16, 2016 17:02

Fixes microsoft#3650: Support stop-all-threads mode debugging for mul…

9af920e

…ti-threaded programs Add a new flag to the StoppedEvent that will trigger the debug adapter to be queried for callstacks of the expanded threads in the callstack viewlet.

edumunoz force-pushed the edumunoz/stop-all-debug branch from 9d5f576 to 9af920e Compare March 16, 2016 20:53

edumunoz reviewed Mar 17, 2016
View reviewed changes

isidorn added a commit that referenced this pull request Mar 17, 2016

Merge pull request #3990 from edumunoz/edumunoz/stop-all-debug

5d620ea

Fixes #3650: Support stop-all-threads mode debugging for multi-thread…

isidorn merged commit 5d620ea into microsoft:master Mar 17, 2016

microsoft deleted a comment Jul 20, 2019

microsoft locked as resolved and limited conversation to collaborators Jul 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #3650: Support stop-all-threads mode debugging for multi-thread… #3990

Fixes #3650: Support stop-all-threads mode debugging for multi-thread… #3990

edumunoz commented Mar 11, 2016

msftclas commented Mar 11, 2016

edumunoz commented Mar 11, 2016

msftclas commented Mar 11, 2016

weinand commented Mar 11, 2016

isidorn commented Mar 11, 2016

felixfbecker commented Mar 11, 2016

isidorn commented Mar 11, 2016

edumunoz commented Mar 11, 2016

edumunoz commented Mar 15, 2016

isidorn commented Mar 15, 2016

daviwil commented Mar 15, 2016

edumunoz Mar 16, 2016

isidorn Mar 16, 2016

edumunoz commented Mar 16, 2016

isidorn commented Mar 16, 2016

isidorn Mar 16, 2016

isidorn Mar 16, 2016

edumunoz Mar 17, 2016

edumunoz commented Mar 17, 2016

isidorn commented Mar 17, 2016

		@@ -134,18 +134,18 @@ export class DebugEditorModelManager implements IWorkbenchContribution {
		const result: editorcommon.IModelDeltaDecoration[] = [];

		@@ -7,6 +7,11 @@ import assert = require('assert');
		import uri from 'vs/base/common/uri';

Fixes #3650: Support stop-all-threads mode debugging for multi-thread… #3990

Fixes #3650: Support stop-all-threads mode debugging for multi-thread… #3990

Conversation

edumunoz commented Mar 11, 2016

msftclas commented Mar 11, 2016

edumunoz commented Mar 11, 2016

msftclas commented Mar 11, 2016

weinand commented Mar 11, 2016

isidorn commented Mar 11, 2016

felixfbecker commented Mar 11, 2016

isidorn commented Mar 11, 2016

edumunoz commented Mar 11, 2016

edumunoz commented Mar 15, 2016

isidorn commented Mar 15, 2016

Debugging JavaScript

daviwil commented Mar 15, 2016

edumunoz Mar 16, 2016

Choose a reason for hiding this comment

isidorn Mar 16, 2016

Choose a reason for hiding this comment

edumunoz commented Mar 16, 2016

isidorn commented Mar 16, 2016

isidorn Mar 16, 2016

Choose a reason for hiding this comment

isidorn Mar 16, 2016

Choose a reason for hiding this comment

edumunoz Mar 17, 2016

Choose a reason for hiding this comment

edumunoz commented Mar 17, 2016

isidorn commented Mar 17, 2016