search

microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
163.06k stars 28.8k forks source link

Make sure event subscriptions are stored and disposed #214234

Open jrieken opened 4 months ago

jrieken commented 4 months ago

The following is a list of code locations at which an event listener is added but the subscription (e.g the disposable that's being returned) is getting lost (garbage collected before disposal). This means the listener is question cannot be removed anymore and therefore it is a leak.

You can enable these warnings via src/vs/base/common/event.ts#_enableListenerGCedWarning

connor4312 commented 4 months ago

I am somewhat confused about vs/workbench/contrib/debug/browser/debugProgress.ts, since those listeners are added to a disposable array kept in an IWorkbenchContribution, which I would not expect to get disposed until the workbench is shutting down, and never forgotten about. (Nothing fancy going on, the two listeners are pushed to the disposable array in the contributing constructor and the array is never again modified before it's disposed)

edit: oh, I see, workbench contributions aren't actually disposables/disposed of. All of debug's "leaking listeners" are from workbench contributions or contributions that share the same lifecycle. I guess I can manually listen to the ILifecycleService.onWillShutDown and dispose listeners right before the workbench shuts down, but this seems like extra code size and computation for not much purpose...

jrieken commented 4 months ago

I guess I can manually listen to the ILifecycleService.onWillShutDown and dispose listeners right before the workbench shuts down, but this seems like extra code size and computation for not much purpose...

Maybe or we make it a thing of workbench contributions, e.g probe if they "implement" disposable do that for all of them by default. @bpasero Opinions?

bpasero commented 4 months ago

If I had to pick, I would rather not let contributions listen to the shutdown but do this from 1 listener from the contributions registry.

But then: whats the point of disposing on shutdown? Its not consistently applied, mainly because we know the JS context goes away anyway, so there is no way something can leak beyond that?

Tyriar commented 4 months ago

+ @meganrogge for task.contribution.ts

jrieken commented 3 months ago

But then: whats the point of disposing on shutdown? Its not consistently applied, mainly because we know the JS context goes away anyway, so there is no way something can leak beyond that?

To me it's less about unloading JS but to have the theoretical possibility to shutdown the workbench and have it leave no traces, e.g no more DOM nodes, no more listener etc. So, that everything can be GC'ed and that we could restart the workbench in a different configuration, e.g with different contribution etc

jrieken commented 3 months ago

🔈 fyi everyone: this list is now updated without auto-tracked workbench contributions but also with leaks from the main process and the EH

bpasero commented 3 months ago

I think some are wrongly reported, for example:

https://github.com/microsoft/vscode/blob/d2020d8e006d529a08353b6e367474681e32bee1/src/vs/workbench/contrib/contextmenu/browser/contextmenu.contribution.ts#L22-L23

Where we pass in the disposables array to store into.

jrieken commented 3 months ago

I think some are wrongly reported, for example:

They leak because this is a workbench contribution but it doesn't implement a dispose method. Therefore it get's GC'ed and the subscriptions are lost.

bpasero commented 3 months ago

Ah yes, true!

connor4312 commented 3 months ago

The leaks found in debug's statusBarColorProvider and debugLifecycle are workbench contributions but are still tracked here

jrieken commented 3 months ago

The leaks found in debug's statusBarColorProvider and debugLifecycle are workbench contributions but are still tracked here

@connor4312 Both look like legit leaks to me:

  1. statusBarColorProvider passes the disposable store as this argument (so no store to track)
  2. debugLifecycle calling onBeforeShutdown return a disposable which isn't tracked
connor4312 commented 3 months ago

Oh, I see. That's a nasty mixup. I'll add a check for that (passing a DisposableStore as a thisArg) since that should never be intended/

Edit: looks like that was the only place that mixup happened in a basic sanity test of the workbench

jrieken commented 3 months ago

ping @aeschli @joyceerhl @joaomoreno

aeschli commented 3 months ago

The 'leaks' in colorUtils. iconRegistry and tokenClassificationRegistry come from wiring registries: registering & updating the JSON schema registry with schemas describing known colors, icons and semantic tokens.

The registries are all globals and registries don't get disposed.

Is there a better way to structure this?

jrieken commented 3 months ago

@aeschli Good question and not really sure of a good suggestion. Maybe it is already wrong that registries fire events all together. Should they be "done and dusted" after code loading and then read into some service which does more complex things?