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

clear focused thread if it does not exist #133637

Merged
merged 3 commits into from
Nov 19, 2021
Merged

Conversation

suzmue
Copy link
Contributor

@suzmue suzmue commented Sep 22, 2021

When working with multithreaded programs, threads that are focused may not exist at the next stop. Make sure that the focus is not on a non-existent thread, since this will cause the toolbar to fail to update appropriately.

This PR fixes #133623

@roblourens
Copy link
Member

Thanks for the PR! Will have to look at it for October

@roblourens
Copy link
Member

So the thread exited via a ThreadEvent? It seems like this should be changed at the moment the thread exits, rather than waiting until another thread stops, does that make sense?

} else if (event.body.reason === 'exited') {

@suzmue
Copy link
Contributor Author

suzmue commented Oct 21, 2021

In this case there is no exited event for the thread that was selected. But the future threads request, does not include the thread. I can definitely add a clearing check when the thread exits, but this check later is still important to handle debug adapters that are not as thorough about sending thread events.

For Go, we use the threads view to present goroutines and would not want to send threads events for every goroutine that is created, since there may be many short lived goroutines. We do not even track the creation / exit of new goroutines, but will look for them when the threads request is sent.

We are also hoping that #116109 will yield some UI improvements.

@roblourens
Copy link
Member

Ok thanks for the explanation. I am guessing that you set the preserveFocusHint flag, otherwise it would have been fixed up above:

if (!event.body.preserveFocusHint && thread.getCallStack().length) {

Would it make sense to do fix in that function instead? So the condition would be if (!event.body.preserveFocusHint && thread.getCallStack().length || threadDoesNotExist) {

@rustyx
Copy link

rustyx commented Nov 18, 2021

Bump. we need this!

@suzmue
Copy link
Contributor Author

suzmue commented Nov 19, 2021

Would it make sense to do fix in that function instead? So the condition would be if (!event.body.preserveFocusHint && thread.getCallStack().length || threadDoesNotExist) {

I moved the logic to also check the condition here. But this also needs to be cleared in the case that there is no thread id returned in the stopped event (the thread id is optional so this case should be handled).

Copy link
Member

@roblourens roblourens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me, thanks!

@roblourens roblourens merged commit d0f6f16 into microsoft:main Nov 19, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Jan 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

debug: toolbar does not update if focused thread no longer exists
3 participants