Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adopt ensureNoDisposablesLeaked in code action model tests #209279

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
+24 −20
Open

adopt ensureNoDisposablesLeaked in code action model tests #209279

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d8f0dee
adopt ensureNoDisposables
justschen Apr 1, 2024
a2f7e04
fix the leaky leak
justschen Apr 1, 2024
ee71421
passing tests, but TS error
justschen Apr 1, 2024
2394fcc
better fix, put disposal behind try/catch
justschen Apr 2, 2024
4d25e85
better fix, put disposal behind try/catch
justschen Apr 2, 2024
2907961
switch to using disposable store
justschen Apr 2, 2024
f005f2e
add timeout for disposable
justschen Apr 2, 2024
a18a742
remove extra disposables
justschen Apr 19, 2024
0dcb64a
another extra disposable store removal
justschen Apr 19, 2024
a5dc84c
Adds back using disposable store, no more leaking
justschen Apr 19, 2024
88f4711
clear and dispose?
justschen Apr 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .eslintrc.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,6 @@
"src/vs/base/test/browser/browser.test.ts",
"src/vs/base/test/browser/ui/scrollbar/scrollableElement.test.ts",
"src/vs/base/test/browser/ui/scrollbar/scrollbarState.test.ts",
"src/vs/editor/contrib/codeAction/test/browser/codeActionModel.test.ts",
"src/vs/editor/test/common/services/languageService.test.ts",
"src/vs/editor/test/node/classification/typescript.test.ts",
"src/vs/platform/configuration/test/common/configuration.test.ts",
Expand Down
2 changes: 1 addition & 1 deletion src/vs/editor/contrib/codeAction/browser/codeAction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ export async function getCodeActions(

try {
const actions = await Promise.all(promises);
const allActions = actions.map(x => x.actions).flat();
const allActions = actions.flatMap(x => x.actions);
const allDocumentation = [
...coalesce(actions.map(x => x.documentation)),
...getAdditionalDocumentationForShowingActions(registry, model, trigger, allActions)
Expand Down
23 changes: 16 additions & 7 deletions src/vs/editor/contrib/codeAction/browser/codeActionModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import { CancelablePromise, createCancelablePromise, TimeoutTimer } from 'vs/base/common/async';
import { isCancellationError } from 'vs/base/common/errors';
import { Emitter } from 'vs/base/common/event';
import { Disposable, MutableDisposable } from 'vs/base/common/lifecycle';
import { Disposable, DisposableStore, MutableDisposable } from 'vs/base/common/lifecycle';
import { isEqual } from 'vs/base/common/resources';
import { URI } from 'vs/base/common/uri';
import { ICodeEditor } from 'vs/editor/browser/editorBrowser';
Expand Down Expand Up @@ -48,8 +48,13 @@ class CodeActionOracle extends Disposable {
}

public trigger(trigger: CodeActionTrigger): void {
const selection = this._getRangeOfSelectionUnlessWhitespaceEnclosed(trigger);
this._signalChange(selection ? { trigger, selection } : undefined);
try {
const selection = this._getRangeOfSelectionUnlessWhitespaceEnclosed(trigger);
this._signalChange(selection ? { trigger, selection } : undefined);
} finally {
// dispose after finishing (and after our _delay has passed)
setTimeout(() => this.dispose(), 300);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

private _onMarkerChanges(resources: readonly URI[]): void {
Expand Down Expand Up @@ -163,6 +168,8 @@ export class CodeActionModel extends Disposable {
private readonly _onDidChangeState = this._register(new Emitter<CodeActionsState.State>());
public readonly onDidChangeState = this._onDidChangeState.event;

private readonly disposables = this._register(new DisposableStore());

private _disposed = false;

constructor(
Expand Down Expand Up @@ -192,6 +199,7 @@ export class CodeActionModel extends Disposable {
return;
}
this._disposed = true;
this.disposables.dispose();

super.dispose();
this.setState(CodeActionsState.Empty, true);
Expand Down Expand Up @@ -229,9 +237,10 @@ 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));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like something for a DisposableStore and/or a MutableDisposable

const allCodeActions = [...codeActionSet.allActions];
if (token.isCancellationRequested) {
this.disposables.delete(codeActionSet);
return emptyCodeActionSet;
}

Expand Down Expand Up @@ -270,7 +279,7 @@ export class CodeActionModel extends Disposable {
};

const selectionAsPosition = new Selection(trackedPosition.lineNumber, trackedPosition.column, trackedPosition.lineNumber, trackedPosition.column);
const actionsAtMarker = await getCodeActions(this._registry, model, selectionAsPosition, newCodeActionTrigger, Progress.None, token);
const actionsAtMarker = this.disposables.add(await getCodeActions(this._registry, model, selectionAsPosition, newCodeActionTrigger, Progress.None, token));

if (actionsAtMarker.validActions.length !== 0) {
for (const action of actionsAtMarker.validActions) {
Expand Down Expand Up @@ -315,8 +324,8 @@ export class CodeActionModel extends Disposable {
}
}
}
// temporarilly hiding here as this is enabled/disabled behind a setting.
return 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));
return codeActionSet;
});
if (trigger.trigger.type === CodeActionTriggerType.Invoke) {
this._progressService?.showWhile(actions, 250);
Expand Down
25 changes: 12 additions & 13 deletions src/vs/editor/contrib/codeAction/test/browser/codeActionModel.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@

import * as assert from 'assert';
import { promiseWithResolvers } from 'vs/base/common/async';
import { DisposableStore } from 'vs/base/common/lifecycle';
import { assertType } from 'vs/base/common/types';
import { URI } from 'vs/base/common/uri';
import { runWithFakedTimers } from 'vs/base/test/common/timeTravelScheduler';
import { ensureNoDisposablesAreLeakedInTestSuite } from 'vs/base/test/common/utils';
import { ICodeEditor } from 'vs/editor/browser/editorBrowser';
import { LanguageFeatureRegistry } from 'vs/editor/common/languageFeatureRegistry';
import * as languages from 'vs/editor/common/languages';
Expand Down Expand Up @@ -38,19 +38,18 @@ suite('CodeActionModel', () => {
let markerService: MarkerService;
let editor: ICodeEditor;
let registry: LanguageFeatureRegistry<languages.CodeActionProvider>;
const disposables = new DisposableStore();

setup(() => {
disposables.clear();
markerService = new MarkerService();
model = createTextModel('foobar foo bar\nfarboo far boo', languageId, undefined, uri);
editor = createTestCodeEditor(model);
editor.setPosition({ lineNumber: 1, column: 1 });
registry = new LanguageFeatureRegistry();
});

const store = ensureNoDisposablesAreLeakedInTestSuite();

teardown(() => {
disposables.clear();
editor.dispose();
model.dispose();
markerService.dispose();
Expand All @@ -61,11 +60,11 @@ suite('CodeActionModel', () => {

await runWithFakedTimers({ useFakeTimers: true }, () => {
const reg = registry.register(languageId, testProvider);
disposables.add(reg);
store.add(reg);

const contextKeys = new MockContextKeyService();
const model = disposables.add(new CodeActionModel(editor, registry, markerService, contextKeys, undefined));
disposables.add(model.onDidChangeState((e: CodeActionsState.State) => {
const model = store.add(new CodeActionModel(editor, registry, markerService, contextKeys, undefined));
store.add(model.onDidChangeState((e: CodeActionsState.State) => {
assertType(e.type === CodeActionsState.Type.Triggered);

assert.strictEqual(e.trigger.type, languages.CodeActionTriggerType.Auto);
Expand Down Expand Up @@ -93,7 +92,7 @@ suite('CodeActionModel', () => {
test('Oracle -> position changed', async () => {
await runWithFakedTimers({ useFakeTimers: true }, () => {
const reg = registry.register(languageId, testProvider);
disposables.add(reg);
store.add(reg);

markerService.changeOne('fake', uri, [{
startLineNumber: 1, startColumn: 1, endLineNumber: 1, endColumn: 6,
Expand All @@ -107,8 +106,8 @@ suite('CodeActionModel', () => {

return new Promise((resolve, reject) => {
const contextKeys = new MockContextKeyService();
const model = disposables.add(new CodeActionModel(editor, registry, markerService, contextKeys, undefined));
disposables.add(model.onDidChangeState((e: CodeActionsState.State) => {
const model = store.add(new CodeActionModel(editor, registry, markerService, contextKeys, undefined));
store.add(model.onDidChangeState((e: CodeActionsState.State) => {
assertType(e.type === CodeActionsState.Type.Triggered);

assert.strictEqual(e.trigger.type, languages.CodeActionTriggerType.Auto);
Expand All @@ -129,12 +128,12 @@ suite('CodeActionModel', () => {
const { promise: donePromise, resolve: done } = promiseWithResolvers<void>();
await runWithFakedTimers({ useFakeTimers: true }, () => {
const reg = registry.register(languageId, testProvider);
disposables.add(reg);
store.add(reg);

let triggerCount = 0;
const contextKeys = new MockContextKeyService();
const model = disposables.add(new CodeActionModel(editor, registry, markerService, contextKeys, undefined));
disposables.add(model.onDidChangeState((e: CodeActionsState.State) => {
const model = store.add(new CodeActionModel(editor, registry, markerService, contextKeys, undefined));
store.add(model.onDidChangeState((e: CodeActionsState.State) => {
assertType(e.type === CodeActionsState.Type.Triggered);

assert.strictEqual(e.trigger.type, languages.CodeActionTriggerType.Auto);
Expand Down
Loading