Plug-ins leaking CodeLens, etc.

[tsmaeder]

In packages/plugin-ext/src/plugin/languages/lens.ts, we maintain a cache of CodeLens objects we send to the main Theia process. This is required because the VS Code languageserver-node client library relies on the same object being passed to the "resolveCodeLens" call in order to pass back the "data" field to a language server.
However, this cache is never cleaned up, so we're keeping all CodeLens objects that are ever created.

The same problem seems to exist in task-provider.ts and link-provider.ts.

[tsmaeder]

For completions, there is a "dispose" method being called when the completion widget is closed, so we are able to clean up the cache when that happens. A cursory inspection has not yielded similar a similar lifecycle event for code lenses. Detecting that a code lens is no longer needed might be a difficult problem.

[tsmaeder]

There problem here is tracking when the code lenses are not used anymore in the main Theia process. In the absence of lifecycle events, maybe we can detect liveness by putting them in a WeakMap and regularly checking whether they are collected? Since we are talking about only few objects, we could do so relatively seldom (every 10 secs?)

[tolusha]

@tsmaeder @akosyakov
VS Code uses IHeapService to collect such objects. So if there are no objections I will create a CQ and copy necessary code.

[1] https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/services/heap/node/heap.ts#L11

[tolusha]

gc-signals is a native dependency and can be used only at backend. So, I have to find another solution.

[tsmaeder]

@tolusha yes, that's why I suggested doing a weak map and purge thingy. The idea would be to have a map object->id and a set and when stuff gets collected from the map, the ids will be in the set, but not the map anymore and you can send "collected" to the back end.

[tolusha]

@tsmaeder
at first glance it sounds doable, but WeakMap [1] doesn't have any iterable methods over values
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakMap#Methods

[tolusha]

@akosyakov @tsmaeder
I have several solutions:

1. Clean up entities once workpace.onDidCloseTextDocument event is fired.
2. Clean up entities by timer

[tsmaeder]

@tolusha that would work for code lenses, but what about links & tasks?

[tsmaeder]

True about WeakMap. How lame!

[akosyakov]

Clean up entities once workpace.onDidCloseTextDocument event is fired.

It's probably too late, if new code lenses are created on each text change.

Clean up entities by timer

Do you mean to keep WeakMap in languages-main and check which elements were garbage collected by timer?

I wonder can't we just remove them after resolving? Does Monaco try to resolve resolved code lenses again? Could you check?

Also see microsoft/vscode#75048. Seems soon (like in Autumn) there will be dispose method for code lens list on Monaco APIs.

[tsmaeder]

@akosyakov Yeah, the idea was to put them in a weak map and check periodically if they have been collected. Problem is that you can't enumerate a WeakMap, so I don't see how we can implement that algorithm. If we could get the "live" code actions from monaco, we could also compute the "collected" set periodically.

I wonder can't we just remove them after resolving?

What about the code lenses that are never resolved?

Good to see that the MS folks are addressing the issue. The question is: can we live with the leakage until then? We should do an estimation of the garbage produced per hour.

[akosyakov]

Problem is that you can't enumerate a WeakMap

We could keep set of keys identifying code lenses and check periodically whether WeakMap still contains such keys? If not delete a key and release a corresponding code lens in the plugin host.

[tsmaeder]

That was my idea, too, but the problem is that the keys are what is weakly referenced in the WeakMap. So in this case, the CodeLens objects. If something is not in the map, how do we know it was there before? We would have to keep a strong reference to it, which would keep it from being garbage collected.

[akosyakov]

Ah, sorry, i did not read WeakMap spec carefully, so code lenses should be a key. When they look useless to us :(

Maybe we can play with provideCodeLenses method itself, i.e. if it is called again for the same document then we dispose old code lenses and provide new. Plus clean everything when the document is closed.

[tsmaeder]

Yes, I think that would work. At most, we would have one set of code lenses that are garbage, but not collected per open editor.

[tsmaeder]

The same workaround would work for linkProvider

[tsmaeder]

I think our implementation of the "taskProvider" API is wrong: https://code.visualstudio.com/api/references/vscode-api#TaskProvider says:

This method will not be called for tasks returned from the above provideTasks method since those tasks are always fully resolved.

This would indicate that we should never call "resolveTask" on a task that is in the object cache in TaskProviderAdapter. So we can just remove the cache there and we're fine for this case.

[tsmaeder]

I believe with the above cases covered, we can proceed with this task.

[akosyakov]

Monaco PR was merged. It should be possible to invalidate caches properly.
Look for TODO in https://github.com/theia-ide/theia/blob/master/packages/plugin-ext/src/main/browser/languages-main.ts
In order to profile the plugin host process follow https://github.com/theia-ide/theia/blob/master/doc/Developing.md#profiling

[vrubezhny]

@akosyakov, @tsmaeder It looks like dispose() (https://github.com/eclipse-theia/theia/blob/master/packages/plugin-ext/src/main/browser/languages-main.ts#L382) is still never invoked for the Document Link Provider. At least it is true for the links generated for *.md-file editor. Also I couldn't find any traces of links clean up in theia/examples/browser/lib/vs/editor/editor.main.js...

Is it OK to clean up the cache every time provideDocumentLinks() or onDidRemoveDocument() is invoked for the Document Link Provider - as we supposed to implement the clean up before the Monaco Upgrade PR was merged?

[akosyakov]

At least it is true for the links generated for *.md-file editor.

How can I get such links?

Is it OK to clean up the cache every time provideDocumentLinks() or onDidRemoveDocument() is invoked for the Document Link Provider - as we supposed to implement the clean up before the Monaco Upgrade PR was merged?

I want to confirm first that we don't overlook it. VS Code relies on it, dispose method is there, maybe we overlook something or don't understand lifecycle of links.

[akosyakov]

For some reason, they expect dispose method to be on the provider not list of links. Could you check what VS Code does, i.e. how does it register and dispose link providers for the extension host process?

[vrubezhny]

At least it is true for the links generated for *.md-file editor.

How can I get such links?

I'm running my branch (rebased over Theia master) as a Browser Example - and I have defined that dispode() method so it calls according LinkProviderAdapter''s $releaseDocumentLinks()method which should clean the cache. Eclipse Theia is added to my workspace, so, when I open I see that:

• When I open https://github.com/eclipse-theia/theia/blob/master/CODE_OF_CONDUCT.md I see two Document links added to the cache. (CHANGELOG.md in the same directory gives few hundreds of cached records).
• When I add edit the document and add a new link to its text - it is also cached
• When I remove a link from the text - no items removed from the cach (dispose() isn't called)
• When I close the editor - the cache is not cleared (dispose() isn't called)
• When I open the same file again - new Document Links are created and cashed, old linka are still in the cache (dispose() isn't called)

[akosyakov]

Links for markdown is not contributed by the plugin system. You should make sure that links are coming from the plugin host process, only then dispose should be called.

[vrubezhny]

Well then I should find another way yo get links,

But then, should we do anything to clear the cache from the links like those for markdown? It looks like they are really never get removed from the cache.

[akosyakov]

But then, should we do anything to clear the cache from the links like those for markdown? It looks like they are really never get removed from the cache.

I've filed an issue for VS Code to check it. It looks like indeed links are leaking, but it should be fixed in the upstream first. We could try to hack around for the plugin host process, but all other links will leak anyway.

[akosyakov]

It was fixed in the upstream today: microsoft/vscode#91536 (comment) I think it is going to be part of Monaco Feb release. It is important for us not only for the plugin system but fro built-in providers as well.

[vrubezhny]

@akosyakov Thanks!