Skip to content

Commit

Permalink
fix leaking disposables (#237586)
Browse files Browse the repository at this point in the history
* chore - mark disposables from registry#register functions as disposables so that they don't appear as leaking

* fix various leaking disposables
  • Loading branch information
jrieken authored Jan 10, 2025
1 parent 8e81ea6 commit f80816a
Show file tree
Hide file tree
Showing 12 changed files with 57 additions and 40 deletions.
2 changes: 1 addition & 1 deletion src/vs/editor/common/languages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2107,7 +2107,7 @@ export interface CodeLens {

export interface CodeLensList {
lenses: CodeLens[];
dispose(): void;
dispose?(): void;
}

export interface CodeLensProvider {
Expand Down
10 changes: 5 additions & 5 deletions src/vs/editor/common/languages/languageConfigurationRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*--------------------------------------------------------------------------------------------*/

import { Emitter, Event } from '../../../base/common/event.js';
import { Disposable, IDisposable, toDisposable } from '../../../base/common/lifecycle.js';
import { Disposable, IDisposable, markAsSingleton, toDisposable } from '../../../base/common/lifecycle.js';
import * as strings from '../../../base/common/strings.js';
import { ITextModel } from '../model.js';
import { DEFAULT_WORD_REGEXP, ensureValidWordDefinition } from '../core/wordHelper.js';
Expand Down Expand Up @@ -202,15 +202,15 @@ class ComposedLanguageConfiguration {
);
this._entries.push(entry);
this._resolved = null;
return toDisposable(() => {
return markAsSingleton(toDisposable(() => {
for (let i = 0; i < this._entries.length; i++) {
if (this._entries[i] === entry) {
this._entries.splice(i, 1);
this._resolved = null;
break;
}
}
});
}));
}

public getResolvedConfiguration(): ResolvedLanguageConfiguration | null {
Expand Down Expand Up @@ -332,10 +332,10 @@ export class LanguageConfigurationRegistry extends Disposable {
const disposable = entries.register(configuration, priority);
this._onDidChange.fire(new LanguageConfigurationChangeEvent(languageId));

return toDisposable(() => {
return markAsSingleton(toDisposable(() => {
disposable.dispose();
this._onDidChange.fire(new LanguageConfigurationChangeEvent(languageId));
});
}));
}

public getLanguageConfiguration(languageId: string): ResolvedLanguageConfiguration | null {
Expand Down
2 changes: 1 addition & 1 deletion src/vs/editor/contrib/codelens/browser/codeLensCache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ export class CodeLensCache implements ICodeLensCache {
};
});
const copyModel = new CodeLensModel();
copyModel.add({ lenses: copyItems, dispose: () => { } }, this._fakeProvider);
copyModel.add({ lenses: copyItems }, this._fakeProvider);

const item = new CacheItem(model.getLineCount(), copyModel);
this._cache.set(model.uri.toString(), item);
Expand Down
13 changes: 8 additions & 5 deletions src/vs/editor/contrib/codelens/browser/codelens.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

import { CancellationToken } from '../../../../base/common/cancellation.js';
import { illegalArgument, onUnexpectedExternalError } from '../../../../base/common/errors.js';
import { DisposableStore } from '../../../../base/common/lifecycle.js';
import { DisposableStore, isDisposable } from '../../../../base/common/lifecycle.js';
import { assertType } from '../../../../base/common/types.js';
import { URI } from '../../../../base/common/uri.js';
import { ITextModel } from '../../../common/model.js';
Expand All @@ -24,18 +24,21 @@ export class CodeLensModel {

lenses: CodeLensItem[] = [];

private readonly _disposables = new DisposableStore();
private _store: DisposableStore | undefined;

dispose(): void {
this._disposables.dispose();
this._store?.dispose();
}

get isDisposed(): boolean {
return this._disposables.isDisposed;
return this._store?.isDisposed ?? false;
}

add(list: CodeLensList, provider: CodeLensProvider): void {
this._disposables.add(list);
if (isDisposable(list)) {
this._store ??= new DisposableStore();
this._store.add(list);
}
for (const symbol of list.lenses) {
this.lenses.push({ symbol, provider });
}
Expand Down
3 changes: 2 additions & 1 deletion src/vs/editor/contrib/hover/browser/contentHoverRendered.ts
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,7 @@ class RenderedContentHoverParts extends Disposable {
...hoverContext
};
const disposables = new DisposableStore();
disposables.add(statusBar);
for (const participant of participants) {
const renderedHoverParts = this._renderHoverPartsForParticipant(hoverParts, participant, hoverRenderingContext);
disposables.add(renderedHoverParts);
Expand All @@ -294,7 +295,7 @@ class RenderedContentHoverParts extends Disposable {
actions: renderedStatusBar.actions,
});
}
return toDisposable(() => { disposables.dispose(); });
return disposables;
}

private _renderHoverPartsForParticipant(hoverParts: IHoverPart[], participant: IEditorHoverParticipant<IHoverPart>, hoverRenderingContext: IEditorHoverRenderContext): IRenderedHoverParts<IHoverPart> {
Expand Down
32 changes: 19 additions & 13 deletions src/vs/editor/contrib/links/browser/getLinks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ export class Link implements ILink {

export class LinksList {

static readonly Empty = new LinksList([]);

readonly links: Link[];

private readonly _disposables = new DisposableStore();
Expand Down Expand Up @@ -137,27 +139,31 @@ export class LinksList {

}

export function getLinks(providers: LanguageFeatureRegistry<LinkProvider>, model: ITextModel, token: CancellationToken): Promise<LinksList> {

export async function getLinks(providers: LanguageFeatureRegistry<LinkProvider>, model: ITextModel, token: CancellationToken): Promise<LinksList> {
const lists: [ILinksList, LinkProvider][] = [];

// ask all providers for links in parallel
const promises = providers.ordered(model).reverse().map((provider, i) => {
return Promise.resolve(provider.provideLinks(model, token)).then(result => {
const promises = providers.ordered(model).reverse().map(async (provider, i) => {
try {
const result = await provider.provideLinks(model, token);
if (result) {
lists[i] = [result, provider];
}
}, onUnexpectedExternalError);
});

return Promise.all(promises).then(() => {
const result = new LinksList(coalesce(lists));
if (!token.isCancellationRequested) {
return result;
} catch (err) {
onUnexpectedExternalError(err);
}
result.dispose();
return new LinksList([]);
});

await Promise.all(promises);

let res = new LinksList(coalesce(lists));

if (token.isCancellationRequested) {
res.dispose();
res = LinksList.Empty;
}

return res;
}


Expand Down
2 changes: 1 addition & 1 deletion src/vs/monaco.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8060,7 +8060,7 @@ declare namespace monaco.languages {

export interface CodeLensList {
lenses: CodeLens[];
dispose(): void;
dispose?(): void;
}

export interface CodeLensProvider {
Expand Down
10 changes: 5 additions & 5 deletions src/vs/platform/actions/common/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

import { IAction, SubmenuAction } from '../../../base/common/actions.js';
import { Event, MicrotaskEmitter } from '../../../base/common/event.js';
import { DisposableStore, dispose, IDisposable, toDisposable } from '../../../base/common/lifecycle.js';
import { DisposableStore, dispose, IDisposable, markAsSingleton, toDisposable } from '../../../base/common/lifecycle.js';
import { LinkedList } from '../../../base/common/linkedList.js';
import { ThemeIcon } from '../../../base/common/themables.js';
import { ICommandAction, ICommandActionTitle, Icon, ILocalizedString } from '../../action/common/action.js';
Expand Down Expand Up @@ -397,11 +397,11 @@ export const MenuRegistry: IMenuRegistry = new class implements IMenuRegistry {
this._commands.set(command.id, command);
this._onDidChangeMenu.fire(MenuRegistryChangeEvent.for(MenuId.CommandPalette));

return toDisposable(() => {
return markAsSingleton(toDisposable(() => {
if (this._commands.delete(command.id)) {
this._onDidChangeMenu.fire(MenuRegistryChangeEvent.for(MenuId.CommandPalette));
}
});
}));
}

getCommand(id: string): ICommandAction | undefined {
Expand All @@ -422,10 +422,10 @@ export const MenuRegistry: IMenuRegistry = new class implements IMenuRegistry {
}
const rm = list.push(item);
this._onDidChangeMenu.fire(MenuRegistryChangeEvent.for(id));
return toDisposable(() => {
return markAsSingleton(toDisposable(() => {
rm();
this._onDidChangeMenu.fire(MenuRegistryChangeEvent.for(id));
});
}));
}

appendMenuItems(items: Iterable<{ id: MenuId; item: IMenuItem | ISubmenuItem }>): IDisposable {
Expand Down
4 changes: 2 additions & 2 deletions src/vs/platform/commands/common/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import { Emitter, Event } from '../../../base/common/event.js';
import { Iterable } from '../../../base/common/iterator.js';
import { IJSONSchema } from '../../../base/common/jsonSchema.js';
import { IDisposable, toDisposable } from '../../../base/common/lifecycle.js';
import { IDisposable, markAsSingleton, toDisposable } from '../../../base/common/lifecycle.js';
import { LinkedList } from '../../../base/common/linkedList.js';
import { TypeConstraint, validateConstraints } from '../../../base/common/types.js';
import { ILocalizedString } from '../../action/common/action.js';
Expand Down Expand Up @@ -121,7 +121,7 @@ export const CommandsRegistry: ICommandRegistry = new class implements ICommandR
// tell the world about this command
this._onDidRegisterCommand.fire(id);

return ret;
return markAsSingleton(ret);
}

registerCommandAlias(oldId: string, newId: string): IDisposable {
Expand Down
4 changes: 2 additions & 2 deletions src/vs/workbench/contrib/extensions/browser/extensionsList.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ export class Renderer implements IPagedRenderer<IExtension, ITemplateData> {
focusOnlyEnabledItems: true
});
actionbar.setFocusable(false);
actionbar.onDidRun(({ error }) => error && this.notificationService.error(error));
const actionBarListener = actionbar.onDidRun(({ error }) => error && this.notificationService.error(error));

const extensionStatusIconAction = this.instantiationService.createInstance(ExtensionStatusAction);
const actions = [
Expand Down Expand Up @@ -150,7 +150,7 @@ export class Renderer implements IPagedRenderer<IExtension, ITemplateData> {
const extensionContainers: ExtensionContainers = this.instantiationService.createInstance(ExtensionContainers, [...actions, ...widgets]);

actionbar.push(actions, { icon: true, label: true });
const disposable = combinedDisposable(...actions, ...widgets, actionbar, extensionContainers);
const disposable = combinedDisposable(...actions, ...widgets, actionbar, actionBarListener, extensionContainers);

return {
root, element, icon, name, installCount, ratings, description, publisherDisplayName, disposables: [disposable], actionbar,
Expand Down
11 changes: 9 additions & 2 deletions src/vs/workbench/contrib/scm/browser/scmViewPane.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import { IContextKeyService, IContextKey, ContextKeyExpr, RawContextKey } from '
import { ICommandService } from '../../../../platform/commands/common/commands.js';
import { IKeybindingService } from '../../../../platform/keybinding/common/keybinding.js';
import { MenuItemAction, IMenuService, registerAction2, MenuId, IAction2Options, MenuRegistry, Action2, IMenu } from '../../../../platform/actions/common/actions.js';
import { IAction, ActionRunner, Action, Separator, IActionRunner } from '../../../../base/common/actions.js';
import { IAction, ActionRunner, Action, Separator, IActionRunner, toAction } from '../../../../base/common/actions.js';
import { ActionBar, IActionViewItemProvider } from '../../../../base/browser/ui/actionbar/actionbar.js';
import { IThemeService, IFileIconTheme } from '../../../../platform/theme/common/themeService.js';
import { isSCMResource, isSCMResourceGroup, isSCMRepository, isSCMInput, collectContextMenuActions, getActionViewItemProvider, isSCMActionButton, isSCMViewService, isSCMResourceNode, connectPrimaryMenu } from './util.js';
Expand Down Expand Up @@ -2988,7 +2988,14 @@ export class SCMActionButton implements IDisposable {
for (let index = 0; index < button.secondaryCommands.length; index++) {
const commands = button.secondaryCommands[index];
for (const command of commands) {
actions.push(new Action(command.id, command.title, undefined, true, async () => await this.executeCommand(command.id, ...(command.arguments || []))));
actions.push(toAction({
id: command.id,
label: command.title,
enabled: true,
run: async () => {
await this.executeCommand(command.id, ...(command.arguments || []));
}
}));
}
if (commands.length) {
actions.push(new Separator());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,11 @@ export class AuthenticationUsageService extends Disposable implements IAuthentic
}
}

this._authenticationService.onDidRegisterAuthenticationProvider(
this._register(this._authenticationService.onDidRegisterAuthenticationProvider(
provider => this._queue.queue(
() => this._addExtensionsToCache(provider.id)
)
);
));
}

async initializeExtensionUsageCache(): Promise<void> {
Expand Down

0 comments on commit f80816a

Please sign in to comment.