-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
fixes #12112 Removing exited thread from threads #12113
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution 👍
In order to accept your changes please be sure to sure to sign the eclipse contributor agreement (eca) with the same email as your authorship.
@vince-fugnitto should I close this PR and recommit changes with a proper signature? I don't understand what I should do, sorry |
@EvilBeaver you don't need to modify or close the pull-request, just be sure to sign the agreement with the same email as authorship and we can re-run the |
@vince-fugnitto I've created eclipse account and signed agreement. Hope I've done everything correct :) |
@vince-fugnitto can you give any clues to why ubuntu tests could fail? It seems there's some external problem on build agents... |
No problem, there is an issue tracking the ubuntu tests which execute our api test-suite as it is currently known to be flaky (#12081). |
@vince-fugnitto thanks. Can I say that PR is fine and I shouldn't do anything else with it to have it merged? |
Right, we just need to review the changes and confirm it is correct. I haven't had the time to look into it yet but I will try shortly. |
Thanks for the heads-up, @vince-fugnitto 👍 I could not verify it with the HEAD of Theia, but I applied the change from this PR on 1.31.1, and the |
To reproduce this, you'll need to have no threads debugged (all exited), but debug session still active. I'm not familiar with cortex-debug, but it's hard to achieve in node.js for example, cause it's usually single threaded. |
@kittaakos @vince-fugnitto ping. Can we either merge or close this one? |
Thanks for the ping. Feel free to merge it. I can confirm, It does not cause any regression with the |
@kittaakos @vince-fugnitto not without an approval ;-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried the changeset (#12113 (comment)) by manually applying it in the Arduino IDE source code, and I can confirm it does not cause any problems with the cortex-debug
VS Code extension.
Thanks for the work @EvilBeaver 💪
@EvilBeaver do you mind rebasing the pull-request so we can run it against the latest CI state? |
updated to latest state of master branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 I did not notice any regressions with the approach and agree with Akos.
What it does
Fixes #12112
How to test
Check that when multiple threads existed, and one of them exited - it removed from threads panel
Review checklist
Reminder for reviewers