Clean up debug session lifecycle

[tsmaeder]

What it does

Reworks the lifecycle of debug session related objects to be more reliable for edge cases. Fixes #10164

The fix consists of multiple changes:

1. Do not rely on fake events to signal debug session lifecycle events ("Exited"), etc. Instead, rely on closing comms channels.
2. The owner of all debug session-related objects is the DebugAdapterSession object. Once that object goes away, all related objects in the back-end and front-end go away.
3. Simplified the "PluginConnection" service:

• shared implementation for main/ext side
• simplify structure of connection objects

How to test

I've tested these changes by using the newest VS Code Java/java debug extensions, the jsdebug typescript debugger and the mock debug extension from #10163

It would be interesting to extend the testing to other debuggers I'm less familiar with, in particular C++ or python, for example. The relevant scenarios are stop and restart actions, as well as killing debuggees or debug adapters.

Review checklist

• As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• As a reviewer, I agree to behave in accordance with the review guidelines

[alvsan09]

I have tested it using the vscode-mock-debug extension using all available run modes and the sessions are now terminated 👍 .
However, using the chrome developer tools (Memory) to take snapshots of heap memory before and after a debug session we can detect debug session objects that are not garbage collected i.e.

• DebugSessionConnection
• DebugSource

These objects increase in proportion to the number of executions. e.g.
the following image show the number of object instances remaining after the first run:

The next image shows the number of object instances after 5 additional runs:

[tsmaeder]

Great catch, @alvsan09. Does the tool show which references retain the leaked objects?

[alvsan09]

Great catch, @alvsan09. Does the tool show which references retain the leaked objects?

The tool itself does not provide you with the facility to move up to the containing references (as far as I can see),
however the references in the leaked objects may help us do that some times e.g
in the picture below we can see that debug source has a reference to the debug session pointing to an instance of PluginDebugSession, so that help us for this case.

[alvsan09]

Great catch, @alvsan09. Does the tool show which references retain the leaked objects?

I was wrong about the facilities of the tool, look at the bottom of the page inside Memory "Retainers",
you can then see the whole tree :-)

[tsmaeder]

It looks like the debug console session never disposes the completion provider it registers with monaco. It looks like this is is not a new bug, but should be trivial to fix.

[tsmaeder]

@msujew while cleaning up sessions, I'm getting stuck at console-session.ts#118 ff.

            // Don't delete the last session by overriding it with 'undefined'
            if (session) {

but we have to let go of the session once it is disposed. If we need to retain something beyond the lifecycle of the debug session, we have to retain it outside of the debug session.

[colin-grant-work]

@msujew while cleaning up sessions, I'm getting stuck at console-session.ts#118 ff.

            // Don't delete the last session by overriding it with 'undefined'
            if (session) {

but we have to let go of the session once it is disposed.

console-widget.ts, I think.

[msujew]

@tsmaeder I see, that makes sense. I'm not totally against removing the last session if it closes. I implemented it this way, since that's how it seemed to work in vscode. When ending the last active debug session, the console would still display the latest session's text. However, when removing the last debug session console, Theia loses the text as well.

I'm alright with you removing the last session as well, I'm sure we'll find another way to persist the last session's text in the debug console somehow later (in a separate PR).

[thegecko]

Still pending testing with our debug adapter, but have added a few minor comments.

[thegecko]

nit: IWebSocket isn't used any more here

[thegecko]

@tsmaeder where possible, please ensure common code (which doesn't use node features) is kept in the common folder. See #10163 (comment)

[tsmaeder]

@tsmaeder I see, that makes sense. I'm not totally against removing the last session if it closes. I implemented it this way, since that's how it seemed to work in vscode. When ending the last active debug session, the console would still display the latest session's text. However, when removing the last debug session console, Theia loses the text as well.

As far as I can tell, VS code keeps debug sessions after they are closed. However, I don't think the debug console session would react properly if we tried to evaluate an expression on a closed debug session, for example.

I'm alright with you removing the last session as well, I'm sure we'll find another way to persist the last session's text in the debug console somehow later (in a separate PR).

In general, what's the lifecycle of console sessions? Do they stick around after the process they are bound to terminates? I think the relationship between the console session and the counterpart that it gets output from is similar.

[thegecko]

Successfully tested with our debug adapter and happy for this to be merged when you are ready.
Thanks for the changes, should make a future PR for upstreaming browser debug support easier.

[msujew]

Looks good to me 👍

• Common debug adapters (js, markdown-debug) work well with the changes
• Malfunctioning debug adapters are closed when a timeout is reached

@thegecko Did you test the changes with a C debug adapter/cortex-debug? I wasn't able to get mine to work correctly.

[thegecko]

@thegecko Did you test the changes with a C debug adapter/cortex-debug? I wasn't able to get mine to work correctly.

No, I tested a proprietary adapter we are developing

[paul-marechal]

I think we would benefit from merging #10265 before this one?

[tsmaeder]

@thegecko Did you test the changes with a C debug adapter/cortex-debug? I wasn't able to get mine to work correctly.

No, I tested a proprietary adapter we are developing

@thegecko did you test after my latest changes?

[thegecko]

@thegecko did you test after my latest changes?

I have now and it still works

[paul-marechal]

@tsmaeder CI is red you may need to remove an import.

Also, do you feel this should land in 1.20.0 being released today or can this wait for 1.21.0?