Fix subtle memory leak issue with hook event handlers which closes ov…

…er the client instance

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<T>;
     }
 
-    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<TEventName extends keyof HookEvents>(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<TEventName extends keyof HookEvents>(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<TEventName extends keyof HookEvents>(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<keyof HookEvents> {
-    return this.options.hooks.eventNames();
+    return this.hooks.eventNames();
   }
 }
 

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<ClientCacheState>;
 
@@ -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 = <SafeHooksWrapper & { hooksWeakRef: WeakRef<Hooks> }>{
+      hooks, // stored only temporarily, will be deleted by `yieldHooks()`
+      hooksWeakRef,
+      emit<TEventName extends keyof HookEvents>(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;
   }

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 {

src/Hooks.ts

@@ -101,3 +101,13 @@ export class Hooks implements IProvidesHooks, IEventEmitter<HookEvents> {
     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<TEventName extends keyof HookEvents>(eventName: TEventName, ...args: HookEvents[TEventName]): boolean;
+}

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.