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

Add support for lifecycleManagedByParent. #11751

Merged

Conversation

tsmaeder
Copy link
Contributor

What it does

Adds support for the lifecycleSupportedByParent flag on debug session. Fixes #11511

How to test

Contributed on behalf of ST Microelectronics

Review checklist

Reminder for reviewers

Forwards lifecycle events to parent session where applicable and changes
the behavior of compound sessions to behave same way.

Contributed on behalf of ST Microelectronics

Signed-off-by: Thomas Mäder <[email protected]>
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

I noticed some issue related to parent and compound session management: When terminating a child session which has lifecycleManagedByParent, the corresponding parent session is correctly terminated. However, when the parent session is part of a compound session, only the parent session is terminated, not the whole compound. The following screencap demonstrates this effect:

2022-10-13.14-56-48.mp4

VSCode correctly terminates the whole compound session in that case.

packages/debug/src/browser/debug-session-manager.ts Outdated Show resolved Hide resolved
@JonasHelming JonasHelming added the vscode issues related to VSCode compatibility label Oct 13, 2022
@msujew msujew added the debug issues that related to debug functionality label Oct 18, 2022
Signed-off-by: Thomas Mäder <[email protected]>
@tsmaeder
Copy link
Contributor Author

tsmaeder commented Nov 3, 2022

@msujew could you have another look, please?

@msujew
Copy link
Member

msujew commented Nov 3, 2022

@tsmaeder I haven't tried reproducing it, but looking at the code, I believe you didn't address the comment I had in here yet?

@tsmaeder
Copy link
Contributor Author

tsmaeder commented Nov 3, 2022

I verified that the code correctly walks upward to the compound configuration an that it terminates all root sessions from there. I was able to reproduce the case where terminating the back end by hand would leave child sessions behind, but not reliably. Looks to me like it's some kind of timing issue that IMO is unrelated to the change at hand.

@tsmaeder
Copy link
Contributor Author

tsmaeder commented Nov 4, 2022

Hmh....turns out the "currentSession" management in the UI is completely off: the session being terminated is not what's selected in the UI. Debugging.

@tsmaeder
Copy link
Contributor Author

tsmaeder commented Nov 4, 2022

Some more debugging shows that the "Launch Browser Frontend" sessions has a child session which has lifecycleManagedByParent=false. So going stop going up the chain at this point, which explains why the back end is not completely stopped (just the part that is bound to the front end). Not sure why the session structure in VS Code looks so much different though.

@tsmaeder
Copy link
Contributor Author

tsmaeder commented Nov 4, 2022

@msujew from debugging, I believe our behaviour is correct. Not sure why VS Code behaves differently, but one thing to keep in mind is that it is using a newer version of js-debug than what we are using.

@tsmaeder
Copy link
Contributor Author

tsmaeder commented Nov 7, 2022

I debugged through this case in VS Code: debugCommands.ts#stopHandler() has code that walks from the selected debug sessions up the parentSession chain to the root: so what is being terminated is the Launch Browser Frontend session, not the editorWorker sessions. All this leads me to conclude that the code in this PR is correct. Whether we want to change our behaviour to match VS Code as far as which session to terminate when the user presses the "Stop"-button is a question outside the scope of this PR (but I would argue that our behaviour is the better one).

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Alright, thank you @tsmaeder for investigating this, I'll now gladly approve this PR :)

I can confirm that the changes work as expected and the property is marked as supported in the API comparison report.

@tsmaeder tsmaeder merged commit 36bb4b6 into eclipse-theia:master Nov 7, 2022
@vince-fugnitto vince-fugnitto added this to the 1.32.0 milestone Nov 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debug issues that related to debug functionality vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[vscode] Support debug sessions being managed by a parent session
4 participants