adopt ensureNoDisposablesLeaked in code action model tests #209279
Conversation
|
looks like there were some leaks 👎 |
| @@ -229,7 +231,7 @@ export class CodeActionModel extends Disposable { | |||
|
|
|||
| const actions = createCancelablePromise(async token => { | |||
| if (this._settingEnabledNearbyQuickfixes() && trigger.trigger.type === CodeActionTriggerType.Invoke && (trigger.trigger.triggerAction === CodeActionTriggerSource.QuickFix || trigger.trigger.filter?.include?.contains(CodeActionKind.QuickFix))) { | |||
| const codeActionSet = await getCodeActions(this._registry, model, trigger.selection, trigger.trigger, Progress.None, token); | |||
| const codeActionSet = this.disposables.add(await getCodeActions(this._registry, model, trigger.selection, trigger.trigger, Progress.None, token)); | |||
There was a problem hiding this comment.
I think this introduces a leak as items are added to disposables but never removed
Instead try to make sure the old disposable values are released once a new set of values come in
There was a problem hiding this comment.
That sounds like something for a DisposableStore and/or a MutableDisposable
| this._signalChange(selection ? { trigger, selection } : undefined); | ||
| } finally { | ||
| // dispose after finishing (and after our _delay has passed) | ||
| setTimeout(() => this.dispose(), 300); |
There was a problem hiding this comment.
This seems fishy. You rarely want to call dispose directly like this as using the object should no longer be used after it has been disposed of . It looks like this object is meant to hang around for a while
What object was getting reported as leaked?
There was a problem hiding this comment.
yeah, this is the stack trace:
(shared with 1/2 leaks) at trackDisposable@http://localhost:58253/960ce1ff9eb3ef82c943175eaac845ed/out/vs/base/common/lifecycle.js:46:21
(shared with 1/2 leaks) at trackDisposable@http://localhost:58253/960ce1ff9eb3ef82c943175eaac845ed/out/vs/base/common/lifecycle.js:183:43
(shared with 1/2 leaks) at Disposable@http://localhost:58253/960ce1ff9eb3ef82c943175eaac845ed/out/vs/base/common/lifecycle.js:379:28
(shared with 1/2 leaks) at ManagedCodeActionSet@http://localhost:58253/960ce1ff9eb3ef82c943175eaac845ed/out/vs/editor/contrib/codeAction/browser/codeAction.js:49:13
(shared with 1/2 leaks) at getCodeActions@http://localhost:58253/960ce1ff9eb3ef82c943175eaac845ed/out/vs/editor/contrib/codeAction/browser/codeAction.js:119:20
(shared with 1/2 leaks) at async*_update/this._codeActionOracle.value</actions<@http://localhost:58253/960ce1ff9eb3ef82c943175eaac845ed/out/vs/editor/contrib/codeAction/browser/codeActionModel.js:269:79
codeActionModel.js:269 is the call to getCodeActions that we are returning.
agree, when disposing immediately, you would get the error i mentioned before, so the code action set and actions derived from it are seemingly used in the command later on as well, so disposing it early (even after no longer being used in the function) leads to issues. will continue to explore this as well.
regarding: #200091
cc. @mjbvz