Fix subtle memory leak issue

src/AutoPollConfigService.ts

@@ -9,60 +9,58 @@ import { delay } from "./Utils";
 export class AutoPollConfigService extends ConfigServiceBase<AutoPollOptions> implements IConfigService {
 
   private initialized: boolean;
-  private readonly initialization: Promise<void>;
+  private readonly initializationPromise: Promise<boolean>;
   private signalInitialization: () => void = () => { /* Intentional no-op. */ };
   private workerTimerId?: ReturnType<typeof setTimeout>;
-  private readonly initTimerId?: ReturnType<typeof setTimeout>;
   private readonly pollIntervalMs: number;
+  readonly readyPromise: Promise<ClientCacheState>;
 
   constructor(configFetcher: IConfigFetcher, options: AutoPollOptions) {
 
     super(configFetcher, options);
 
     this.pollIntervalMs = options.pollIntervalSeconds * 1000;
 
-    const initialCacheSync = super.syncUpWithCache(true);
+    const initialCacheSyncUp = this.syncUpWithCache();
 
     if (options.maxInitWaitTimeSeconds !== 0) {
       this.initialized = false;
 
       // This promise will be resolved when
       // 1. the cache contains a valid config at startup (see startRefreshWorker) or
-      // 2. config json is fetched the first time, regardless of success or failure (see onConfigUpdated) or
-      // 3. maxInitWaitTimeSeconds > 0 and maxInitWaitTimeSeconds has passed (see the setTimeout call below).
-      this.initialization = new Promise(resolve => this.signalInitialization = () => {
+      // 2. config json is fetched the first time, regardless of success or failure (see onConfigUpdated).
+      const initSignalPromise = new Promise<void>(resolve => this.signalInitialization = resolve);
+
+      // This promise will be resolved when either initialization ready is signalled by signalInitialization() or maxInitWaitTimeSeconds pass.
+      this.initializationPromise = this.waitForInitializationAsync(initSignalPromise).then(success => {
         this.initialized = true;
-        clearTimeout(this.initTimerId);
-        resolve();
+        return success;
       });
-
-      this.initialization.then(() => options.hooks.emit("clientReady", this.getCacheState(options.cache.getInMemory())));
-
-      if (options.maxInitWaitTimeSeconds > 0) {
-        this.initTimerId = setTimeout(() => this.signalInitialization(), options.maxInitWaitTimeSeconds * 1000);
-      }
     }
     else {
       this.initialized = true;
-      this.initialization = Promise.resolve();
-      options.hooks.emit("clientReady", this.getCacheState(options.cache.getInMemory()));
+      this.initializationPromise = Promise.resolve(false);
     }
 
+    this.readyPromise = this.getReadyPromise(this.initializationPromise, async initializationPromise => {
+      await initializationPromise;
+      return this.getCacheState(this.options.cache.getInMemory());
+    });
+
     if (!options.offline) {
-      this.startRefreshWorker(initialCacheSync);
+      this.startRefreshWorker(initialCacheSyncUp);
     }
   }
 
-  private async waitForInitializationAsync(): Promise<boolean> {
+  private async waitForInitializationAsync(initSignalPromise: Promise<void>): Promise<boolean> {
     if (this.options.maxInitWaitTimeSeconds < 0) {
-      await this.initialization;
+      await initSignalPromise;
       return true;
     }
 
     const delayCleanup: { clearTimer?: () => void } = {};
-    // Simply awaiting the initialization promise would also work but we limit waiting to maxInitWaitTimeSeconds for maximum safety.
     const success = await Promise.race([
-      this.initialization.then(() => true),
+      initSignalPromise.then(() => true),
       delay(this.options.maxInitWaitTimeSeconds * 1000, delayCleanup).then(() => false)
     ]);
     delayCleanup.clearTimer!();
@@ -85,7 +83,7 @@ export class AutoPollConfigService extends ConfigServiceBase<AutoPollOptions> im
       }
 
       this.options.logger.debug("AutoPollConfigService.getConfig() - cache is empty or expired, waiting for initialization.");
-      await this.waitForInitializationAsync();
+      await this.initializationPromise;
     }
 
     cachedConfig = await this.options.cache.get(this.cacheKey);
@@ -125,12 +123,12 @@ export class AutoPollConfigService extends ConfigServiceBase<AutoPollOptions> im
     this.stopRefreshWorker();
   }
 
-  private async startRefreshWorker(initialCacheSync?: Promise<ProjectConfig>) {
+  private async startRefreshWorker(initialCacheSyncUp?: ProjectConfig | Promise<ProjectConfig>) {
     this.options.logger.debug("AutoPollConfigService.startRefreshWorker() called.");
 
     const delayMs = this.pollIntervalMs;
 
-    const latestConfig = await (initialCacheSync ?? this.options.cache.get(this.cacheKey));
+    const latestConfig = await (initialCacheSyncUp ?? this.options.cache.get(this.cacheKey));
     if (latestConfig.isExpired(this.pollIntervalMs)) {
       // Even if the service gets disposed immediately, we allow the first refresh for backward compatibility,
       // i.e. to not break usage patterns like this:

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;
@@ -563,7 +567,8 @@ export class ConfigCatClient implements IConfigCatClient {
   }
 
   waitForReady(): Promise<ClientCacheState> {
-    return this.options.readyPromise;
+    const configService = this.configService;
+    return configService ? configService.readyPromise : Promise.resolve(ClientCacheState.HasLocalOverrideFlagDataOnly);
   }
 
   snapshot(): IConfigCatClientSnapshot {
@@ -628,19 +633,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 +654,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

@@ -2,12 +2,13 @@ import type { IConfigCache, IConfigCatCache } from "./ConfigCatCache";
 import { ExternalConfigCache, InMemoryConfigCache } from "./ConfigCatCache";
 import type { IConfigCatLogger } from "./ConfigCatLogger";
 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,9 +156,7 @@ export abstract class OptionsBase {
 
   offline: boolean = false;
 
-  hooks: Hooks;
-
-  readyPromise: Promise<ClientCacheState>;
+  hooks: SafeHooksWrapper;
 
   constructor(apiKey: string, clientVersion: string, options?: IOptions | null,
     defaultCacheFactory?: ((options: OptionsBase) => IConfigCache) | null,
@@ -181,8 +180,15 @@ 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;
+      }
+    };
 
     let logger: IConfigCatLogger | null | undefined;
     let cache: IConfigCatCache | null | undefined;
@@ -220,7 +226,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 +236,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/ConfigServiceBase.ts

@@ -42,6 +42,8 @@ export enum ClientCacheState {
 }
 
 export interface IConfigService {
+  readonly readyPromise: Promise<ClientCacheState>;
+
   getConfig(): Promise<ProjectConfig>;
 
   refreshConfigAsync(): Promise<[RefreshResult, ProjectConfig]>;
@@ -70,6 +72,8 @@ export abstract class ConfigServiceBase<TOptions extends OptionsBase> {
 
   protected readonly cacheKey: string;
 
+  abstract readonly readyPromise: Promise<ClientCacheState>;
+
   constructor(
     protected readonly configFetcher: IConfigFetcher,
     protected readonly options: TOptions) {
@@ -296,11 +300,13 @@ export abstract class ConfigServiceBase<TOptions extends OptionsBase> {
 
   abstract getCacheState(cachedConfig: ProjectConfig): ClientCacheState;
 
-  protected async syncUpWithCache(suppressEmitClientReady = false): Promise<ProjectConfig> {
-    const cachedConfig = await this.options.cache.get(this.cacheKey);
-    if (!suppressEmitClientReady) {
-      this.options.hooks.emit("clientReady", this.getCacheState(cachedConfig));
-    }
-    return cachedConfig;
+  protected syncUpWithCache(): ProjectConfig | Promise<ProjectConfig> {
+    return this.options.cache.get(this.cacheKey);
+  }
+
+  protected async getReadyPromise<TState>(state: TState, waitForReadyAsync: (state: TState) => Promise<ClientCacheState>): Promise<ClientCacheState> {
+    const cacheState = await waitForReadyAsync(state);
+    this.options.hooks.emit("clientReady", cacheState);
+    return cacheState;
   }
 }

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;
+}

src/LazyLoadConfigService.ts

@@ -8,13 +8,16 @@ import type { ProjectConfig } from "./ProjectConfig";
 export class LazyLoadConfigService extends ConfigServiceBase<LazyLoadOptions> implements IConfigService {
 
   private readonly cacheTimeToLiveMs: number;
+  readonly readyPromise: Promise<ClientCacheState>;
 
   constructor(configFetcher: IConfigFetcher, options: LazyLoadOptions) {
 
     super(configFetcher, options);
 
     this.cacheTimeToLiveMs = options.cacheTimeToLiveSeconds * 1000;
-    super.syncUpWithCache();
+
+    const initialCacheSyncUp = this.syncUpWithCache();
+    this.readyPromise = this.getReadyPromise(initialCacheSyncUp, async initialCacheSyncUp => this.getCacheState(await initialCacheSyncUp));
   }
 
   async getConfig(): Promise<ProjectConfig> {