Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display top-level stop reason when StoppedEvent specifies no threadId #124532

Closed
polinasok opened this issue May 24, 2021 · 5 comments · Fixed by #126852
Closed

Display top-level stop reason when StoppedEvent specifies no threadId #124532

polinasok opened this issue May 24, 2021 · 5 comments · Fixed by #126852
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug debug Debug viewlet, configurations, breakpoints, adapter issues help wanted Issues identified as good community contribution opportunities insiders-released Patch has been released in VS Code Insiders verified Verification succeeded
Milestone

Comments

@polinasok
Copy link

polinasok commented May 24, 2021

In Go sometimes we do not have a user thread id to specify in the stopped event. But if we issue a stopped event with a missing thread id, vscode will not show our stop reason at all.

{"seq":0,"type":"event","event":"stopped","body":{"reason":"pause","allThreadsStopped":true}}

Instead it would be great if the stop reason was either applied to all threads or even better displayed at the top level in line with CALL STACK like when there is only a single thread.

image

@vscodebot
Copy link

vscodebot bot commented May 24, 2021

(Experimental duplicate detection)
Thanks for submitting this issue. Please also check if it is already covered by an existing one, like:

@isidorn
Copy link
Contributor

isidorn commented May 25, 2021

Ok, so the request is to introduce a rule: if a stopped event has not threadId then the reason should be shown top level or at the session level if there are multiple sessions.
This in general makes sense to me. Let's see what @weinand thinks

Also if we agree to change this I would be open to a PR that fixes this.
Here's a code pointer https://github.com/microsoft/vscode/blob/main/src/vs/workbench/contrib/debug/browser/callStackView.ts#L166

@weinand weinand added the bug Issue identified by VS Code Team member as probable bug label Jun 7, 2021
@weinand
Copy link
Contributor

weinand commented Jun 7, 2021

Since the threadId property of the stopped event is optional, we should not ignore showing the reason in that case (I consider this a bug).

I like @polinasok's proposal.

@weinand weinand removed their assignment Jun 7, 2021
@isidorn
Copy link
Contributor

isidorn commented Jun 7, 2021

Let's assign to June to investigate.
Adding help wanted. If nobody jumps on this I can look into it end of June

@isidorn isidorn added the help wanted Issues identified as good community contribution opportunities label Jun 7, 2021
@isidorn isidorn added this to the June 2021 milestone Jun 7, 2021
suzmue added a commit to suzmue/vscode that referenced this issue Jun 21, 2021
In DAP, the threadID for a stopped event is optional. The stopped
reason and text should be displayed at the top level if the thread
id is not included.

Updates microsoft#124532
@isidorn
Copy link
Contributor

isidorn commented Jun 25, 2021

Thanks @suzmue for tackling this via a PR 👏

@connor4312 connor4312 added the verified Verification succeeded label Jul 1, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Aug 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug debug Debug viewlet, configurations, breakpoints, adapter issues help wanted Issues identified as good community contribution opportunities insiders-released Patch has been released in VS Code Insiders verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants
@weinand @isidorn @connor4312 @polinasok and others