Do not self-dispose

[jrieken]

stumbled over https://github.com/microsoft/vscode/blob/dac0c97d082bd3bf38fdb3251e955e25c7c992a0/src/vs/workbench/contrib/chat/common/promptSyntax/contributions/languageFeatures/providers/providerInstanceBase.ts#L43

It seems ProviderInstanceBase-instances dispose themselves when their parser-property gets disposed. This is wrong, only the creator of an object should dispose of it.

[legomushroom]

Hey @jrieken, would love to learn more about why do you consider this pattern to be bad, can you guide docs recommendations regarding this?

Overall my use case is trivial - the ProviderInstanceBase class relies on it's underlying parser object to be functional, therefore it is being disposed when the parser object is no longer available. What would you recommend doing instead?

As for the pattern overall, it is easy to spot it all over the codebase:

https://github.com/microsoft/vscode/blob/main/src/vs/base/browser/ui/tree/abstractTree.ts#L891 https://github.com/microsoft/vscode/blob/main/src/vs/base/common/cancellation.ts#L70 https://github.com/microsoft/vscode/blob/main/src/vs/editor/browser/services/hoverService/hoverWidget.ts#L251 https://github.com/microsoft/vscode/blob/main/src/vs/base/parts/ipc/node/ipc.cp.ts#L35 https://github.com/microsoft/vscode/blob/main/src/vs/base/parts/ipc/node/ipc.net.ts#L307 https://github.com/microsoft/vscode/blob/main/src/vs/code/electron-main/app.ts#L381 https://github.com/microsoft/vscode/blob/main/src/vs/code/electron-utility/sharedProcess/sharedProcessMain.ts#L151 https://github.com/microsoft/vscode/blob/main/src/vs/editor/browser/services/hoverService/hoverWidget.ts#L150 https://github.com/microsoft/vscode/blob/main/extensions/markdown-language-features/src/preview/preview.ts#L500 https://github.com/microsoft/vscode/blob/main/extensions/git-base/src/remoteSource.ts#L47

My search for this.dispose yields around 106 results. Should we start project-wide adoption effort to eliminate this?

[jrieken]

My search for this.dispose yields around 106 results. Should we start project-wide adoption effort to eliminate this?

Without knowing them all I would strongly say "YES"

Generally, self-disposal is bad because someone created the object and tracks it for disposal, e.g. it assumes the object is workable until disposed (which the owner/creator is in charge of). Otherwise things will break at random. Think of a UI that works with a model object (it created/acquired/owns). Once the model self-disposes the UI is busted. In such a world disposing would bubble "upwards" which is cumbersome, e.g the model tells the UI (its owner) it has been disposed, then UI tells the Application it is defunct etc. That's why we model disposal the other way around - by ownership.

the ProviderInstanceBase class relies on it's underlying parser object to be functional, therefore it is being disposed when the parser object is no longer available.

Why doesn't the ProviderInstanceBase class take ownership? Even tho the parser object comes from a service they could be owned by ref-counting and ProviderInstanceBase releases its reference when going out of business. That would make the dependencies and ownership go "one direction" from "class owns properties" and not the other way around.

[jrieken]

Another case seems to be TextModelContentsProvider - it will also self-dispose when the underlying text model disposes. Still, the owner TextModelPromptParser won't notice and in consequence the ObjectCache continues to hold on to the ITextModel and TextModelPromptParser instances - forever (leak) or until you are lucky and call to getSyntaxParserFor (which is unlikely because the text model is gone).

Both cases are bad and this should work the other way around, e.g acquire a reference to the model (for the time you need). That way your parser service owns the lifecycle and you can make assumptions that the model is there. Use ITextModelService#createModelReference for this

[legomushroom]

@jrieken it can makes sense in general if we can somehow enforce this rule somehow. I see that a lot of folks do self-dispose and that might be due to the fact that it feels a natural path to take for the devs.

One case I still trying to understand is what to do in the case when an object that you rely on gets disposed so you cannot function anymore. For instance, we don't generally have control over a text model lifecycle, so if a component relies on a model that suddenly gets disposed by something else, what the component has to do next? It cannot continue functioning anymore, its "parent" cannot function anymore, and so on. Therefore it feels like there is still the "dipose" state bubbling up the stack, the only difference is that now we try to ignore it instead? Can you explain the > That's why we model disposal the other way around - by ownership, more to answer this question?

[jrieken]

For instance, we don't generally have control over a text model lifecycle, so if a component relies on a model that suddenly gets disposed by something else,

This is not true. You have ITextModelService#createModelReference to create a text model reference and the underlying model will only be disposed when the last reference is cleaned up.

I see that a lot of folks do self-dispose and that might be due to the fact that it feels a natural path to take for the devs.

I doubt this is true. We have invested massively in top-down disposing with DisposableStore, the Disposable-super type, disposable tracking etc pp. It's also the natural and only sane way because otherwise you constantly need to check if your objects have been disposed. Image the garbage collector would work like this and would delete objects that are still referenced.

[legomushroom]

This is not true. You have ITextModelService#createModelReference to create a text model reference and the underlying model will only be disposed when the last reference is cleaned up.

But in my case there is the "decoration" object that uses a text model and the object does not want to keep the text model reference around indefinitely? I'd I want to dispose the decoration object as soon as user closes an editor tab and the underlying text model get's disposed because of that?

I doubt this is true. We have invested massively in top-down disposing with DisposableStore, the Disposable-super type, disposable tracking etc pp. It's also the natural and only sane way because otherwise you constantly need to check if your objects have been disposed. Image the garbage collector would work like this and would delete objects that are still referenced.

I was referring to list of places where this pattern is used by other folks in https://github.com/microsoft/vscode/issues/248366#issuecomment-2864967847 Seems there are quite a bit of such places, ppl seem to fall into the same path as it might feel natural to them?

[jrieken]

Seems there are quite a bit of such places, ppl seem to fall into the same path as it might feel natural to them?

In comparison to those (which we should all revisit) there are 16k+ references to Disposable#_register or DisposableStore#add. So, I think it is out of question what dispose-model we are using. It is top down.

But in my case there is the "decoration" object that uses a text model and the object does not want to keep the text model reference around indefinitely?

First, your ObjectCache does hold on to text models indefinitely and esp. after dispose. Second, why don't you adjust your design? Nobody says you need to hold the reference indefinitely but you can acquire and release text model references at any time and at your own pace/cycle.

I'd I want to dispose the decoration object as soon as user closes an editor tab and the underlying text model get's disposed because of that?

It is not true that closing an editor automatically disposes the text model, it is true that the text model is disposed when the last reference is disposed. Also, if you want to clean-up when the editor tab gets closed, how about listening to that event?

[joaomoreno]

Does ProviderInstanceBase need to know about promptsService? If instances of ProviderInstanceBase are meant to just glue a model to a parser... then why isn't its ctor simply taking (model, parser) as arguments? That would solve the lifecycle dilemma: whomever creates instances of ProviderInstanceBase would be responsible for listening to when the parser is disposed, to then dispose those same instances.

---

Sort of related...

I would seriously re-evaluate the need for ProviderInstanceBase at all tho. It provides very - very - little functionality to warrant being its own thing. Each one of these 3 contributions (PromptDecorator, PromptHeaderDiagnosticsProvider, PromptLinkDiagnosticsProvider) have to extend two abstract classes: ProviderInstanceBase and a generic ProviderInstanceManagerBase. This is way too complex for what this entire feature is doing, IMO. All this is doing is updating a few decorations and markers on model change...

I understand the desire for proper engineering, but I've often found more long-term success in finding the right balance between that and code maintainability, readability and ease of understanding. We are working as a team, after all, and the more aligned we are, the better.

@legomushroom thoughts?

[legomushroom]

@joaomoreno the promptsService provides the way to reuse existing parser object for each text model active, otherwise we would need to parse contents of each text model reference from the scratch over and over again (also subscribe to models events every time).

I'm still not clear how (model, parser) would help in this case? If a provider depends on a non-disposed text model to be functional, and the model gets disposed, what would need to do next? Keeping the provider around not only wasted compute resources but also does not make much sense logic-wise?

Sort of related...

It was actually refactored into these reusable recently because of how much functionality was duplicated so we tried to maintain the balance for a while until it was not convenient to maintain the code anymore 🤔 Some of the decorators are not super complex atm, but we keeping adding more and more logic to them so they will get pretty large very soon I believe 🤷

@jrieken: Also, if you want to clean-up when the editor tab gets closed, how about listening to that event?

To reiterate on the use case I have:

• I apply decorations/markers to a model, not an editor, therefore
• I cannot rely on the editor close events
• the text models are required for the providers to be functional, so disposing a provider as soon as it gets dysfunctional makes sense to me; it also feels like it aligns with the comments you've 'someone created the object and tracks it for disposal, e.g. it assumes the object is workable until disposed (which the owner/creator is in charge of).' made before 🤔 When you use tracks it for disposal, do you mean subscribe to its onDispose event or something else?
• furthermore the parser objects are shared objects that can be used by multiple different components, hence the owner of the objects is a global singleton service not some "local" component (the same case as for the IModelService for instance)

Do you mind providing couple of clear examples for the cases when non-top-to-bottom can lead to real issues or at least results in a hard-to-follow code? That can really help me to understand the issue, not just follow some recommendation blindly.

Given there is the number of similar self-dispose cases in the codebase, do you feel like starting a thread in dev-discuss regarding this? Or you think those cases are fundamentally different from mine? I assume we would have to start this conversation from the beginning if we would want to adopt this rule across the board 🤔

[jrieken]

the text models are required for the providers to be functional, so disposing a provider as soon as it gets dysfunctional makes sense to me;

Your provider can ensure that the text models it depends on don't get disposed. Why do you not use that mechanism?

[legomushroom]

@jrieken because in this case I provide decorations/markers for an existing model to implement, for instance "diagnostics" inside a text editor. If a user closes the editor, there is no need for the diagnostics anymore, so I don't see a reason for keeping the model alive for such diagnostics that are not relevant any longer. Isn't it just a leak of a model if we'd do that?

[jrieken]

For those, from the side cases, you should use IModelService#onModelAdded|onModelRemoved. Like the pseudo sample below is control and don't rely on self-disposal

const store...

modelService.onModelAdded(doc => {
  store.set(doc, new Validation(doc));  
})

modelServce.onModelRemoved(doc => {
  store.dispose(doc)
})

[legomushroom]

Yeah I totally understand that we can do this, but for me it looks like unnecessary indirection and thus complexity:

• all I care about is the direct object I depend on (in this case a model), and I don't need to depend on some external service or other component manages its lifecycle
• the model already provides the onWillDispose event that I can subscribe to
• I could do some hybrid approach similar to what you've suggested:

  // inside `store` component
  model.onWillDispose(() => { this.findBy(model).dispose() })
  

  But this adds extra complexity because we now need to associate the IDisposable returned from the onWillDispose call with a specific model and dispose them at the same time.

So for me, without understanding the downsides of "self dispose", the non-self-dispose approach only adds coupling between components and requires implementing extra logic that is not needed otherwise, without clear benefits that can justify these pitfalls 🤔

[jrieken]

the IDisposable returned from the onWillDispose call with a specific model and dispose them at the same time.

That's why I recommend IModelService#onModelRemoved - it's the onDidDispose event for but on the service level.

So for me, without understanding the downsides of "self dispose",

The downsides of self dispose is that you need many checks for "isDisposed" - I count around 30 usages of ObservableDisposable#isDisposed and ObservableDisposable#assertNotDisposed. There is also type-gymnastics like TextModelPromptParser & { isDisposed: false } You need none of those in the top down approach. With that you also don't need the this.someProperty.onDispose(this.dispose.bind(this))-bubbling, like from ChatPromptAttachmentModel to InstructionsAttachmentWidget to PromptInstructionsAttachmentsCollectionWidget.

approach only adds coupling between components

Please explain how?

[aeschli]

Fixed now