From 72119db103c9365cace4a2a0ef6faca83dec0fd6 Mon Sep 17 00:00:00 2001 From: Johannes Faltermeier Date: Wed, 10 Jan 2024 13:11:53 +0100 Subject: [PATCH 1/2] Memory Leak when using MessageService#showProgress on Backend #13253 * dispose cancellation event listeners in RpcProtocol --- .../src/common/message-rpc/rpc-protocol.ts | 47 ++++++++++++++++++- 1 file changed, 46 insertions(+), 1 deletion(-) diff --git a/packages/core/src/common/message-rpc/rpc-protocol.ts b/packages/core/src/common/message-rpc/rpc-protocol.ts index 9ebd529c4a3ed..9a9461c634cec 100644 --- a/packages/core/src/common/message-rpc/rpc-protocol.ts +++ b/packages/core/src/common/message-rpc/rpc-protocol.ts @@ -45,6 +45,31 @@ export interface RpcProtocolOptions { mode?: 'default' | 'clientOnly' | 'serverOnly' } +/** + * Wrapper for a {@link Disposable} that is not available immediately. + */ +export class MaybeDisposable { + + private disposed = false; + private disposable: Disposable | undefined = undefined; + + setDisposable(disposable: Disposable): void { + if (this.disposed) { + disposable.dispose(); + } else { + this.disposable = disposable; + } + } + + dispose(): void { + this.disposed = true; + if (this.disposable) { + this.disposable.dispose(); + this.disposable = undefined; + } + } +} + /** * Establish a RPC protocol on top of a given channel. By default the rpc protocol is bi-directional, meaning it is possible to send * requests and notifications to the remote side (i.e. acts as client) as well as receiving requests and notifications from the remote side (i.e. acts as a server). @@ -57,6 +82,7 @@ export class RpcProtocol { static readonly CANCELLATION_TOKEN_KEY = 'add.cancellation.token'; protected readonly pendingRequests: Map> = new Map(); + protected readonly pendingRequestCancellationEventListeners: Map = new Map(); protected nextMessageId: number = 0; @@ -80,6 +106,8 @@ export class RpcProtocol { channel.onClose(event => { this.pendingRequests.forEach(pending => pending.reject(new Error(event.reason))); this.pendingRequests.clear(); + this.pendingRequestCancellationEventListeners.forEach(disposable => disposable.dispose()); + this.pendingRequestCancellationEventListeners.clear(); this.toDispose.dispose(); }); this.toDispose.push(channel.onMessage(readBuffer => this.handleMessage(this.decoder.parse(readBuffer())))); @@ -131,6 +159,7 @@ export class RpcProtocol { } else { throw new Error(`No reply handler for reply with id: ${id}`); } + this.disposeCancellationEventListener(id); } protected handleReplyErr(id: number, error: any): void { @@ -141,6 +170,15 @@ export class RpcProtocol { } else { throw new Error(`No reply handler for error reply with id: ${id}`); } + this.disposeCancellationEventListener(id); + } + + protected disposeCancellationEventListener(id: number): void { + const toDispose = this.pendingRequestCancellationEventListeners.get(id); + if (toDispose) { + this.pendingRequestCancellationEventListeners.delete(id); + toDispose.dispose(); + } } sendRequest(method: string, args: any[]): Promise { @@ -157,6 +195,10 @@ export class RpcProtocol { this.pendingRequests.set(id, reply); + // register disposable before output.commit() + const maybeDisposable = new MaybeDisposable(); + this.pendingRequestCancellationEventListeners.set(id, maybeDisposable); + const output = this.channel.getWriteBuffer(); this.encoder.request(output, id, method, args); output.commit(); @@ -164,7 +206,10 @@ export class RpcProtocol { if (cancellationToken?.isCancellationRequested) { this.sendCancel(id); } else { - cancellationToken?.onCancellationRequested(() => this.sendCancel(id)); + const disposable = cancellationToken?.onCancellationRequested(() => this.sendCancel(id)); + if (disposable) { + maybeDisposable.setDisposable(disposable); + } } return reply.promise; From 0b047a0382ea333254ce6a53eb4eed45c3a49512 Mon Sep 17 00:00:00 2001 From: Johannes Faltermeier Date: Mon, 15 Jan 2024 13:01:57 +0100 Subject: [PATCH 2/2] Memory Leak when using MessageService#showProgress on Backend #13253 * renamed MaybeDisposable to DisposableWrapper --- packages/core/src/common/disposable.ts | 25 +++++++++++++ .../src/common/message-rpc/rpc-protocol.ts | 37 +++---------------- 2 files changed, 31 insertions(+), 31 deletions(-) diff --git a/packages/core/src/common/disposable.ts b/packages/core/src/common/disposable.ts index 1920343c9073e..cfb07b03166b0 100644 --- a/packages/core/src/common/disposable.ts +++ b/packages/core/src/common/disposable.ts @@ -133,3 +133,28 @@ export function disposableTimeout(...args: Parameters): Dispo const handle = setTimeout(...args); return { dispose: () => clearTimeout(handle) }; } + +/** + * Wrapper for a {@link Disposable} that is not available immediately. + */ +export class DisposableWrapper implements Disposable { + + private disposed = false; + private disposable: Disposable | undefined = undefined; + + set(disposable: Disposable): void { + if (this.disposed) { + disposable.dispose(); + } else { + this.disposable = disposable; + } + } + + dispose(): void { + this.disposed = true; + if (this.disposable) { + this.disposable.dispose(); + this.disposable = undefined; + } + } +} diff --git a/packages/core/src/common/message-rpc/rpc-protocol.ts b/packages/core/src/common/message-rpc/rpc-protocol.ts index 9a9461c634cec..f3f51decb3270 100644 --- a/packages/core/src/common/message-rpc/rpc-protocol.ts +++ b/packages/core/src/common/message-rpc/rpc-protocol.ts @@ -16,7 +16,7 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ import { CancellationToken, CancellationTokenSource } from '../cancellation'; -import { Disposable, DisposableCollection } from '../disposable'; +import { DisposableWrapper, Disposable, DisposableCollection } from '../disposable'; import { Emitter, Event } from '../event'; import { Deferred } from '../promise-util'; import { Channel } from './channel'; @@ -45,31 +45,6 @@ export interface RpcProtocolOptions { mode?: 'default' | 'clientOnly' | 'serverOnly' } -/** - * Wrapper for a {@link Disposable} that is not available immediately. - */ -export class MaybeDisposable { - - private disposed = false; - private disposable: Disposable | undefined = undefined; - - setDisposable(disposable: Disposable): void { - if (this.disposed) { - disposable.dispose(); - } else { - this.disposable = disposable; - } - } - - dispose(): void { - this.disposed = true; - if (this.disposable) { - this.disposable.dispose(); - this.disposable = undefined; - } - } -} - /** * Establish a RPC protocol on top of a given channel. By default the rpc protocol is bi-directional, meaning it is possible to send * requests and notifications to the remote side (i.e. acts as client) as well as receiving requests and notifications from the remote side (i.e. acts as a server). @@ -82,7 +57,7 @@ export class RpcProtocol { static readonly CANCELLATION_TOKEN_KEY = 'add.cancellation.token'; protected readonly pendingRequests: Map> = new Map(); - protected readonly pendingRequestCancellationEventListeners: Map = new Map(); + protected readonly pendingRequestCancellationEventListeners: Map = new Map(); protected nextMessageId: number = 0; @@ -195,9 +170,9 @@ export class RpcProtocol { this.pendingRequests.set(id, reply); - // register disposable before output.commit() - const maybeDisposable = new MaybeDisposable(); - this.pendingRequestCancellationEventListeners.set(id, maybeDisposable); + // register disposable before output.commit() even when not available yet + const disposableWrapper = new DisposableWrapper(); + this.pendingRequestCancellationEventListeners.set(id, disposableWrapper); const output = this.channel.getWriteBuffer(); this.encoder.request(output, id, method, args); @@ -208,7 +183,7 @@ export class RpcProtocol { } else { const disposable = cancellationToken?.onCancellationRequested(() => this.sendCancel(id)); if (disposable) { - maybeDisposable.setDisposable(disposable); + disposableWrapper.set(disposable); } }