diff --git a/src/ConfigCatClient.ts b/src/ConfigCatClient.ts index 14aed8c..5c5ca64 100644 --- a/src/ConfigCatClient.ts +++ b/src/ConfigCatClient.ts @@ -235,6 +235,7 @@ export class ConfigCatClient implements IConfigCatClient { protected configService?: IConfigService; protected evaluator: IRolloutEvaluator; private readonly options: OptionsBase; + private readonly hooks: Hooks; private defaultUser?: User; private readonly suppressFinalize: () => void; @@ -283,6 +284,9 @@ export class ConfigCatClient implements IConfigCatClient { throw new Error("Invalid 'configCatKernel.configFetcher' value"); } + // To avoid possible memory leaks, the components of the client should not hold a strong reference to the hooks object (see also SafeHooksWrapper). + this.hooks = options.yieldHooks(); + if (options.defaultUser) { this.setDefaultUser(options.defaultUser); } @@ -297,7 +301,7 @@ export class ConfigCatClient implements IConfigCatClient { (() => { throw new Error("Invalid 'options' value"); })(); } else { - this.options.hooks.emit("clientReady", ClientCacheState.HasLocalOverrideFlagDataOnly); + this.hooks.emit("clientReady", ClientCacheState.HasLocalOverrideFlagDataOnly); } this.suppressFinalize = registerForFinalization(this, { sdkKey: options.apiKey, cacheToken, configService: this.configService, logger: options.logger }); @@ -330,7 +334,7 @@ export class ConfigCatClient implements IConfigCatClient { clientInstanceCache.remove(options.apiKey, this.cacheToken); } - ConfigCatClient.close(this.configService, options.logger, options.hooks); + ConfigCatClient.close(this.configService, options.logger, this.hooks); this.suppressFinalize(); } @@ -340,7 +344,7 @@ export class ConfigCatClient implements IConfigCatClient { let errors: any[] | undefined; for (const instance of removedInstances) { try { - ConfigCatClient.close(instance.configService, instance.options.logger, instance.options.hooks); + ConfigCatClient.close(instance.configService, instance.options.logger, instance.hooks); instance.suppressFinalize(); } catch (err) { @@ -375,7 +379,7 @@ export class ConfigCatClient implements IConfigCatClient { value = defaultValue as SettingTypeOf; } - this.options.hooks.emit("flagEvaluated", evaluationDetails); + this.hooks.emit("flagEvaluated", evaluationDetails); return value; } @@ -398,7 +402,7 @@ export class ConfigCatClient implements IConfigCatClient { evaluationDetails = evaluationDetailsFromDefaultValue(key, defaultValue, getTimestampAsDate(remoteConfig), user, errorToString(err), err); } - this.options.hooks.emit("flagEvaluated", evaluationDetails); + this.hooks.emit("flagEvaluated", evaluationDetails); return evaluationDetails; } @@ -441,7 +445,7 @@ export class ConfigCatClient implements IConfigCatClient { } for (const evaluationDetail of evaluationDetailsArray) { - this.options.hooks.emit("flagEvaluated", evaluationDetail); + this.hooks.emit("flagEvaluated", evaluationDetail); } return result; @@ -468,7 +472,7 @@ export class ConfigCatClient implements IConfigCatClient { } for (const evaluationDetail of evaluationDetailsArray) { - this.options.hooks.emit("flagEvaluated", evaluationDetail); + this.hooks.emit("flagEvaluated", evaluationDetail); } return evaluationDetailsArray; @@ -628,19 +632,19 @@ export class ConfigCatClient implements IConfigCatClient { /** @inheritdoc */ on(eventName: TEventName, listener: (...args: HookEvents[TEventName]) => void): this { - this.options.hooks.on(eventName, listener as (...args: any[]) => void); + this.hooks.on(eventName, listener as (...args: any[]) => void); return this; } /** @inheritdoc */ once(eventName: TEventName, listener: (...args: HookEvents[TEventName]) => void): this { - this.options.hooks.once(eventName, listener as (...args: any[]) => void); + this.hooks.once(eventName, listener as (...args: any[]) => void); return this; } /** @inheritdoc */ removeListener(eventName: TEventName, listener: (...args: HookEvents[TEventName]) => void): this { - this.options.hooks.removeListener(eventName, listener as (...args: any[]) => void); + this.hooks.removeListener(eventName, listener as (...args: any[]) => void); return this; } @@ -649,23 +653,23 @@ export class ConfigCatClient implements IConfigCatClient { /** @inheritdoc */ removeAllListeners(eventName?: keyof HookEvents): this { - this.options.hooks.removeAllListeners(eventName); + this.hooks.removeAllListeners(eventName); return this; } /** @inheritdoc */ listeners(eventName: keyof HookEvents): Function[] { - return this.options.hooks.listeners(eventName); + return this.hooks.listeners(eventName); } /** @inheritdoc */ listenerCount(eventName: keyof HookEvents): number { - return this.options.hooks.listenerCount(eventName); + return this.hooks.listenerCount(eventName); } /** @inheritdoc */ eventNames(): Array { - return this.options.hooks.eventNames(); + return this.hooks.eventNames(); } } diff --git a/src/ConfigCatClientOptions.ts b/src/ConfigCatClientOptions.ts index 4e67cdc..160bb5e 100644 --- a/src/ConfigCatClientOptions.ts +++ b/src/ConfigCatClientOptions.ts @@ -5,9 +5,11 @@ import { ConfigCatConsoleLogger, LoggerWrapper } from "./ConfigCatLogger"; import type { ClientCacheState } from "./ConfigServiceBase"; import { DefaultEventEmitter } from "./DefaultEventEmitter"; import type { IEventEmitter } from "./EventEmitter"; +import { NullEventEmitter } from "./EventEmitter"; import type { FlagOverrides } from "./FlagOverrides"; -import type { IProvidesHooks } from "./Hooks"; +import type { HookEvents, IProvidesHooks, SafeHooksWrapper } from "./Hooks"; import { Hooks } from "./Hooks"; +import { getWeakRefStub, isWeakRefAvailable } from "./Polyfills"; import { ProjectConfig } from "./ProjectConfig"; import type { User } from "./RolloutEvaluator"; import { sha1 } from "./Sha1"; @@ -155,7 +157,7 @@ export abstract class OptionsBase { offline: boolean = false; - hooks: Hooks; + hooks: SafeHooksWrapper; readyPromise: Promise; @@ -181,8 +183,17 @@ export abstract class OptionsBase { } const eventEmitter = eventEmitterFactory?.() ?? new DefaultEventEmitter(); - this.hooks = new Hooks(eventEmitter); - this.readyPromise = new Promise(resolve => this.hooks.once("clientReady", resolve)); + const hooks = new Hooks(eventEmitter); + const hooksWeakRef = new (isWeakRefAvailable() ? WeakRef : getWeakRefStub())(hooks); + this.hooks = }>{ + hooks, // stored only temporarily, will be deleted by `yieldHooks()` + hooksWeakRef, + emit(eventName: TEventName, ...args: HookEvents[TEventName]): boolean { + return this.hooksWeakRef.deref()?.emit(eventName, ...args) ?? false; + } + }; + + this.readyPromise = new Promise(resolve => hooksWeakRef.deref()?.once("clientReady", resolve)); let logger: IConfigCatLogger | null | undefined; let cache: IConfigCatCache | null | undefined; @@ -220,7 +231,7 @@ export abstract class OptionsBase { this.offline = options.offline; } - options.setupHooks?.(this.hooks); + options.setupHooks?.(hooks); } this.logger = new LoggerWrapper(logger ?? new ConfigCatConsoleLogger(), this.hooks); @@ -230,6 +241,13 @@ export abstract class OptionsBase { : (defaultCacheFactory ? defaultCacheFactory(this) : new InMemoryConfigCache()); } + yieldHooks(): Hooks { + const hooksWrapper = this.hooks as unknown as { hooks?: Hooks }; + const hooks = hooksWrapper.hooks; + delete hooksWrapper.hooks; + return hooks ?? new Hooks(new NullEventEmitter()); + } + getUrl(): string { return this.baseUrl + "/configuration-files/" + this.apiKey + "/" + OptionsBase.configFileName + "?sdk=" + this.clientVersion; } diff --git a/src/ConfigCatLogger.ts b/src/ConfigCatLogger.ts index 4e8a5a7..a450118 100644 --- a/src/ConfigCatLogger.ts +++ b/src/ConfigCatLogger.ts @@ -1,4 +1,4 @@ -import type { Hooks } from "./Hooks"; +import type { SafeHooksWrapper } from "./Hooks"; import { errorToString } from "./Utils"; /** @@ -81,7 +81,7 @@ export class LoggerWrapper implements IConfigCatLogger { constructor( private readonly logger: IConfigCatLogger, - private readonly hooks?: Hooks) { + private readonly hooks?: SafeHooksWrapper) { } private isLogLevelEnabled(logLevel: LogLevel): boolean { diff --git a/src/Hooks.ts b/src/Hooks.ts index 0ad4da8..ca38141 100644 --- a/src/Hooks.ts +++ b/src/Hooks.ts @@ -101,3 +101,13 @@ export class Hooks implements IProvidesHooks, IEventEmitter { return this.eventEmitter.emit(eventName, ...args); } } + +// Strong back-references to the client instance must be avoided so GC can collect it when user doesn't have references to it any more. +// E.g. if a strong reference chain like AutoPollConfigService -> ... -> ConfigCatClient existed, the client instance could not be collected +// because the background polling loop would keep the AutoPollConfigService alive indefinetely, which in turn would keep alive ConfigCatClient. +// We need to break such strong reference chains with a weak reference somewhere. As consumers are free to add hook event handlers which +// close over the client instance (e.g. `client.on("configChanged", cfg => { client.GetValue(...) }`), that is, a chain like +// AutoPollConfigService -> Hooks -> event handler -> ConfigCatClient can be created, it is the hooks reference that we need to make weak. +export type SafeHooksWrapper = { + emit(eventName: TEventName, ...args: HookEvents[TEventName]): boolean; +} diff --git a/test/ConfigCatClientTests.ts b/test/ConfigCatClientTests.ts index d800b89..f930f17 100644 --- a/test/ConfigCatClientTests.ts +++ b/test/ConfigCatClientTests.ts @@ -1055,11 +1055,53 @@ describe("ConfigCatClient", () => { // Assert assert.equal(2, instanceCount1); - assert.equal(0, instanceCount2); if (isFinalizationRegistryAvailable) { + assert.equal(0, instanceCount2); assert.equal(2, logger.messages.filter(([, , msg]) => msg.indexOf("finalize() called") >= 0).length); } + else { + // When finaliation is not available, Auto Polling won't be stopped. + assert.equal(1, instanceCount2); + } + }); + + it("GC should be able to collect cached instances when hook handler closes over client instance and no strong references are left", async function() { + // Arrange + + setupPolyfills(); + if (!isWeakRefAvailable() || typeof FinalizationRegistry === "undefined" || typeof gc === "undefined") { + this.skip(); + } + + const sdkKey1 = "test1"; + + const configCatKernel: FakeConfigCatKernel = { configFetcher: new FakeConfigFetcher(), sdkType: "common", sdkVersion: "1.0.0" }; + + function createClients() { + const client = ConfigCatClient.get(sdkKey1, PollingMode.ManualPoll, {}, configCatKernel); + client.on("configChanged", () => client.getValueAsync("flag", null)); + + return ConfigCatClient["instanceCache"].getAliveCount(); + } + + // Act + + const instanceCount1 = createClients(); + + // We need to allow the event loop to run so the runtime can detect there's no more strong references to the created clients. + await allowEventLoop(); + gc(); + + // We need to allow the finalizer callbacks to execute. + await allowEventLoop(10); + + const instanceCount2 = ConfigCatClient["instanceCache"].getAliveCount(); + + // Assert + + assert.equal(1, instanceCount1); + assert.equal(0, instanceCount2); }); // For these tests we need to choose a ridiculously large poll interval/ cache TTL to make sure that config is fetched only once.