Theia: [output][vscode][plugin] Output channels are not cleaned up after closing the channel

[kittaakos]

Bug Description:

The output channel registry for the extensions (VS Code and Theia plug-in) does its own caching:
https://github.com/eclipse-theia/theia/blob/250841db7f5de49a1c57ffb437379c8638d35be0/packages/plugin-ext/src/main/browser/output-channel-registry-main.ts#L93

Disposing the channel on the FE does not update the channel cache for the extensions, and tries to work on a disposed editor model.

textModel.ts:360 Uncaught (in promise) Error: Model is disposed!
    at TextModel._assertNotDisposed (textModel.ts:360)
    at TextModel.getLineCount (textModel.ts:739)
    at OutputChannel.<anonymous> (output-channel.ts:422)
    at step (output-channel.ts:15)
    at Object.next (output-channel.ts:15)
    at fulfilled (output-channel.ts:15)

Steps to Reproduce:

1. Start a channel from an extension: const c = (theia|vscode).window.createOutputChannel('test-channel'); c.appendLine('running...');
2. Close the channel with the _Output: Close Output Channel..._ command.
3. Repeat step 1.

Additional Information

• Operating System:
• Theia Version:

[mikael-desharnais]

Adding the following code to the registry would solve the issue :

@postConstruct()
    init(): void {
        this.outputChannelManager.onChannelDeleted((channel) => {
            if (this.channels.has(channel.name)) {
                this.channels.delete(channel.name);
            }
        })
    }

But That would not remove the cache done by the registry just managing it properly ?
Could anyone provide an opinion on which solution would be better ?

[kittaakos]

Could anyone provide an opinion on which solution would be better ?

Why do we need to cache it?

[mikael-desharnais]

Reading code, it does not seem that the caching is usefull (this.channels is private) so removing the cache and using this.outputChannelManager.getChannel all the time should not be an issue

[kittaakos]

Reading code, it does not seem that the caching is usefull (this.channels is private) so removing the cache and using this.outputChannelManager.getChannel all the time should not be an issue

Exactly, I do not see any reason for caching here. The OutputChannelManager manages the channels.

[mikael-desharnais]

Can you propose a pull request ? At the moment, I'm cannot provide one.
Otherwise, I will provide one in a few days

[kittaakos]

Otherwise, I will provide one in a few days

The fix is not urgent, it can wait for you ;) Let me know if you're interested in helping us. I can provide you a few pointers for the PR. Otherwise, we will fix this later.

[mikael-desharnais]

Just to comment on this, there is one drawback to deleting the cache.
The getChannel method of the outputChannelManager returns an existing channel if there is one or creates one if there is none. This method always provides a channel.
Using the cache can prevent the creation of a new channel in the case where someone tries to hide a channel that does not exist.
In this case : this.outputChannelManager.getChannel("Not existing").setVisibility(false), a new channel would be created.
To me, the real problem is the implementation of the OutputChannelManager, the getChannel method should only return existing channels and throw an exception if no channel exists by this name.
There should be a few more methods for this class : hasChannel & createChannel. That would make the code clearer and prevent useless creations of channels.

[kittaakos]

The getChannel method of the outputChannelManager returns an existing channel if there is one or creates one if there is none. This method always provides a channel.

Correct and this is the expected behavior.

Using the cache can prevent the creation of a new channel in the case where someone tries to hide a channel that does not exist.

I do not understand this. Hiding a channel is not removal, we just do not show it in the UI.

In this case : this.outputChannelManager.getChannel("Not existing").setVisibility(false), a new channel would be created.

This is also expected.

the getChannel method should only return existing channels and throw an exception if no channel exists by this name.

Well, we won't break this behavior.

There should be a few more methods for this class : hasChannel & createChannel. That would make the code clearer and prevent useless creations of channels.

Feel free to propose additional APIs, and we will look into your PR, but we cannot change the getChannel behavior.

[mikael-desharnais]

Ok, if we can't break this behavior, I think we can remove all the if (channel) that clutter the registry and can never be false.