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

…er the client instance (#82)

src/ConfigCat.Client.Tests/ConfigCatClientTests.cs

@@ -1307,6 +1307,42 @@ static void CreateClients(out int instanceCount)
         Assert.AreEqual(0, instanceCount2);
     }
 
+    [TestMethod]
+    [DoNotParallelize]
+    public void CachedInstancesCanBeGCdWhenHookHandlerClosesOverClientInstance()
+    {
+        // Arrange
+
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        static void CreateClients(out int instanceCount)
+        {
+            var client = ConfigCatClient.Get("test1", options => options.PollingMode = PollingModes.AutoPoll(maxInitWaitTime: TimeSpan.Zero));
+
+            client.ConfigChanged += (_, e) =>
+            {
+                client.GetValue("flag", false);
+            };
+
+            instanceCount = ConfigCatClient.Instances.GetAliveCount();
+
+            GC.KeepAlive(client);
+        }
+
+        // Act
+
+        CreateClients(out var instanceCount1);
+
+        GC.Collect();
+        GC.WaitForPendingFinalizers();
+
+        var instanceCount2 = ConfigCatClient.Instances.GetAliveCount();
+
+        // Assert
+
+        Assert.AreEqual(1, instanceCount1);
+        Assert.AreEqual(0, instanceCount2);
+    }
+
     private static IConfigCatClient CreateClientWithMockedFetcher(string cacheKey,
         Mock<IConfigCatLogger> loggerMock,
         Mock<IConfigFetcher> fetcherMock,

src/ConfigCatClient/ConfigCatClient.cs

@@ -44,10 +44,13 @@ public LogLevel LogLevel
     internal ConfigCatClient(string sdkKey, ConfigCatClientOptions options)
     {
         this.sdkKey = sdkKey;
-        this.hooks = options.Hooks;
+        this.hooks = options.YieldHooks();
         this.hooks.SetSender(this);
 
-        this.logger = new LoggerWrapper(options.Logger ?? ConfigCatClientOptions.CreateDefaultLogger(), this.hooks);
+        // To avoid possible memory leaks, the components of the client should not hold a strong reference to the hooks object (see also SafeHooksWrapper).
+        var hooksWrapper = new SafeHooksWrapper(this.hooks);
+
+        this.logger = new LoggerWrapper(options.Logger ?? ConfigCatClientOptions.CreateDefaultLogger(), hooksWrapper);
         this.configEvaluator = new RolloutEvaluator(this.logger);
 
         var cacheParameters = new CacheParameters
@@ -79,25 +82,19 @@ internal ConfigCatClient(string sdkKey, ConfigCatClientOptions options)
                     cacheParameters,
                     this.logger,
                     options.Offline,
-                    this.hooks)
-            : new NullConfigService(this.logger, this.hooks);
+                    hooksWrapper)
+            : new NullConfigService(this.logger, hooksWrapper);
     }
 
     // For test purposes only
     internal ConfigCatClient(IConfigService configService, IConfigCatLogger logger, IRolloutEvaluator evaluator, Hooks? hooks = null)
     {
-        if (hooks is not null)
-        {
-            this.hooks = hooks;
-            this.hooks.SetSender(this);
-        }
-        else
-        {
-            this.hooks = NullHooks.Instance;
-        }
+        this.hooks = hooks ?? NullHooks.Instance;
+        this.hooks.SetSender(this);
+        var hooksWrapper = new SafeHooksWrapper(this.hooks);
 
         this.configService = configService;
-        this.logger = new LoggerWrapper(logger, this.hooks);
+        this.logger = new LoggerWrapper(logger, hooksWrapper);
         this.configEvaluator = evaluator;
     }
 
@@ -645,7 +642,7 @@ async Task<SettingsWithRemoteConfig> GetRemoteConfigAsync(CancellationToken canc
         }
     }
 
-    private static IConfigService DetermineConfigService(PollingMode pollingMode, HttpConfigFetcher fetcher, CacheParameters cacheParameters, LoggerWrapper logger, bool isOffline, Hooks hooks)
+    private static IConfigService DetermineConfigService(PollingMode pollingMode, HttpConfigFetcher fetcher, CacheParameters cacheParameters, LoggerWrapper logger, bool isOffline, SafeHooksWrapper hooks)
     {
         return pollingMode switch
         {

src/ConfigCatClient/ConfigService/AutoPollConfigService.cs

@@ -19,7 +19,7 @@ internal AutoPollConfigService(
         CacheParameters cacheParameters,
         LoggerWrapper logger,
         bool isOffline = false,
-        Hooks? hooks = null) : this(options, configFetcher, cacheParameters, logger, startTimer: true, isOffline, hooks)
+        SafeHooksWrapper hooks = default) : this(options, configFetcher, cacheParameters, logger, startTimer: true, isOffline, hooks)
     { }
 
     // For test purposes only
@@ -30,7 +30,7 @@ internal AutoPollConfigService(
         LoggerWrapper logger,
         bool startTimer,
         bool isOffline = false,
-        Hooks? hooks = null) : base(configFetcher, cacheParameters, logger, isOffline, hooks)
+        SafeHooksWrapper hooks = default) : base(configFetcher, cacheParameters, logger, isOffline, hooks)
     {
         this.pollInterval = options.PollInterval;
         this.maxInitWaitTime = options.MaxInitWaitTime >= TimeSpan.Zero ? options.MaxInitWaitTime : Timeout.InfiniteTimeSpan;

src/ConfigCatClient/ConfigService/ConfigServiceBase.cs

@@ -27,15 +27,15 @@ protected internal enum Status
     protected readonly ConfigCache ConfigCache;
     protected readonly LoggerWrapper Logger;
     protected readonly string CacheKey;
-    protected readonly Hooks Hooks;
+    protected readonly SafeHooksWrapper Hooks;
 
-    protected ConfigServiceBase(IConfigFetcher configFetcher, CacheParameters cacheParameters, LoggerWrapper logger, bool isOffline, Hooks? hooks)
+    protected ConfigServiceBase(IConfigFetcher configFetcher, CacheParameters cacheParameters, LoggerWrapper logger, bool isOffline, SafeHooksWrapper hooks)
     {
         this.ConfigFetcher = configFetcher;
         this.ConfigCache = cacheParameters.ConfigCache;
         this.CacheKey = cacheParameters.CacheKey;
         this.Logger = logger;
-        this.Hooks = hooks ?? NullHooks.Instance;
+        this.Hooks = hooks;
         this.status = isOffline ? Status.Offline : Status.Online;
     }
 

src/ConfigCatClient/ConfigService/LazyLoadConfigService.cs

@@ -9,12 +9,12 @@ internal sealed class LazyLoadConfigService : ConfigServiceBase, IConfigService
 {
     private readonly TimeSpan cacheTimeToLive;
 
-    internal LazyLoadConfigService(IConfigFetcher configFetcher, CacheParameters cacheParameters, LoggerWrapper logger, TimeSpan cacheTimeToLive, bool isOffline = false, Hooks? hooks = null)
+    internal LazyLoadConfigService(IConfigFetcher configFetcher, CacheParameters cacheParameters, LoggerWrapper logger, TimeSpan cacheTimeToLive, bool isOffline = false, SafeHooksWrapper hooks = default)
         : base(configFetcher, cacheParameters, logger, isOffline, hooks)
     {
         this.cacheTimeToLive = cacheTimeToLive;
 
-        hooks?.RaiseClientReady();
+        hooks.RaiseClientReady();
     }
 
     public ProjectConfig GetConfig()

src/ConfigCatClient/ConfigService/ManualPollConfigService.cs

@@ -6,10 +6,10 @@ namespace ConfigCat.Client.ConfigService;
 
 internal sealed class ManualPollConfigService : ConfigServiceBase, IConfigService
 {
-    internal ManualPollConfigService(IConfigFetcher configFetcher, CacheParameters cacheParameters, LoggerWrapper logger, bool isOffline = false, Hooks? hooks = null)
+    internal ManualPollConfigService(IConfigFetcher configFetcher, CacheParameters cacheParameters, LoggerWrapper logger, bool isOffline = false, SafeHooksWrapper hooks = default)
         : base(configFetcher, cacheParameters, logger, isOffline, hooks)
     {
-        hooks?.RaiseClientReady();
+        hooks.RaiseClientReady();
     }
 
     public ProjectConfig GetConfig()

src/ConfigCatClient/ConfigService/NullConfigService.cs

@@ -7,11 +7,11 @@ internal sealed class NullConfigService : IConfigService
 {
     private readonly LoggerWrapper logger;
 
-    public NullConfigService(LoggerWrapper logger, Hooks? hooks = null)
+    public NullConfigService(LoggerWrapper logger, SafeHooksWrapper hooks = default)
     {
         this.logger = logger;
 
-        hooks?.RaiseClientReady();
+        hooks.RaiseClientReady();
     }
 
     public ProjectConfig GetConfig() => ProjectConfig.Empty;

src/ConfigCatClient/Configuration/ConfigCatClientOptions.cs

@@ -15,6 +15,8 @@ public class ConfigCatClientOptions : IProvidesHooks
 
     internal static readonly Uri BaseUrlEu = new("https://cdn-eu.configcat.com");
 
+    private Hooks hooks = new();
+
     /// <summary>
     /// The logger implementation to use for performing logging.
     /// If not set, <see cref="ConsoleLogger"/> with <see cref="LogLevel.Warning"/> will be used by default.<br/>
@@ -88,7 +90,12 @@ public Uri BaseUrl
     /// </summary>
     public bool Offline { get; set; }
 
-    internal Hooks Hooks { get; } = new Hooks();
+    internal Hooks YieldHooks()
+    {
+        var hooks = this.hooks;
+        this.hooks = NullHooks.Instance;
+        return hooks;
+    }
 
     internal Uri CreateUri(string sdkKey)
     {
@@ -109,28 +116,28 @@ internal Uri CreateUri(string sdkKey)
     /// <inheritdoc/>
     public event EventHandler? ClientReady
     {
-        add { Hooks.ClientReady += value; }
-        remove { Hooks.ClientReady -= value; }
+        add { this.hooks.ClientReady += value; }
+        remove { this.hooks.ClientReady -= value; }
     }
 
     /// <inheritdoc/>
     public event EventHandler<FlagEvaluatedEventArgs>? FlagEvaluated
     {
-        add { Hooks.FlagEvaluated += value; }
-        remove { Hooks.FlagEvaluated -= value; }
+        add { this.hooks.FlagEvaluated += value; }
+        remove { this.hooks.FlagEvaluated -= value; }
     }
 
     /// <inheritdoc/>
     public event EventHandler<ConfigChangedEventArgs>? ConfigChanged
     {
-        add { Hooks.ConfigChanged += value; }
-        remove { Hooks.ConfigChanged -= value; }
+        add { this.hooks.ConfigChanged += value; }
+        remove { this.hooks.ConfigChanged -= value; }
     }
 
     /// <inheritdoc/>
     public event EventHandler<ConfigCatClientErrorEventArgs>? Error
     {
-        add { Hooks.Error += value; }
-        remove { Hooks.Error -= value; }
+        add { this.hooks.Error += value; }
+        remove { this.hooks.Error -= value; }
     }
 }

src/ConfigCatClient/Hooks/Hooks.cs

@@ -1,5 +1,4 @@
 using System;
-using System.Runtime.CompilerServices;
 using System.Threading;
 
 namespace ConfigCat.Client;
@@ -9,7 +8,7 @@ internal class Hooks : IProvidesHooks
     private static readonly EventHandlers DisconnectedEventHandlers = new();
 
     private volatile EventHandlers eventHandlers;
-    private WeakReference<IConfigCatClient>? clientWeakRef; // should be null only in case of testing
+    private IConfigCatClient? client; // should be null only in case of testing
 
     protected Hooks(EventHandlers eventHandlers)
     {
@@ -29,23 +28,9 @@ public virtual bool TryDisconnect()
         return !ReferenceEquals(originalEventHandlers, DisconnectedEventHandlers);
     }
 
-    private bool TryGetSender(out IConfigCatClient? client)
-    {
-        if (this.clientWeakRef is null)
-        {
-            client = null;
-            return true;
-        }
-
-        return this.clientWeakRef.TryGetTarget(out client);
-    }
-
     public virtual void SetSender(IConfigCatClient client)
     {
-        // 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.
-        // (There are cases - like AutoPollConfigService or LocalFileDataSource - where the background work keeps the service object alive,
-        // so if that had a strong reference to the client object, it would be kept alive as well, which would create a memory leak.)
-        this.clientWeakRef = new WeakReference<IConfigCatClient>(client);
+        this.client = client;
     }
 
     /// <inheritdoc/>
@@ -55,13 +40,9 @@ public event EventHandler? ClientReady
         remove { this.eventHandlers.ClientReady -= value; }
     }
 
-    [MethodImpl(MethodImplOptions.NoInlining)]
     internal void RaiseClientReady()
     {
-        if (this.eventHandlers.ClientReady is { } clientReady && TryGetSender(out var client))
-        {
-            clientReady(client, EventArgs.Empty);
-        }
+        this.eventHandlers.ClientReady?.Invoke(this.client, EventArgs.Empty);
     }
 
     /// <inheritdoc/>
@@ -71,13 +52,9 @@ public event EventHandler<FlagEvaluatedEventArgs>? FlagEvaluated
         remove { this.eventHandlers.FlagEvaluated -= value; }
     }
 
-    [MethodImpl(MethodImplOptions.NoInlining)]
     internal void RaiseFlagEvaluated(EvaluationDetails evaluationDetails)
     {
-        if (this.eventHandlers.FlagEvaluated is { } flagEvaluated && TryGetSender(out var client))
-        {
-            flagEvaluated(client, new FlagEvaluatedEventArgs(evaluationDetails));
-        }
+        this.eventHandlers.FlagEvaluated?.Invoke(this.client, new FlagEvaluatedEventArgs(evaluationDetails));
     }
 
     /// <inheritdoc/>
@@ -87,13 +64,9 @@ public event EventHandler<ConfigChangedEventArgs>? ConfigChanged
         remove { this.eventHandlers.ConfigChanged -= value; }
     }
 
-    [MethodImpl(MethodImplOptions.NoInlining)]
     internal void RaiseConfigChanged(IConfig newConfig)
     {
-        if (this.eventHandlers.ConfigChanged is { } configChanged && TryGetSender(out var client))
-        {
-            configChanged(client, new ConfigChangedEventArgs(newConfig));
-        }
+        this.eventHandlers.ConfigChanged?.Invoke(this.client, new ConfigChangedEventArgs(newConfig));
     }
 
     /// <inheritdoc/>
@@ -103,13 +76,9 @@ public event EventHandler<ConfigCatClientErrorEventArgs>? Error
         remove { this.eventHandlers.Error -= value; }
     }
 
-    [MethodImpl(MethodImplOptions.NoInlining)]
     internal void RaiseError(string message, Exception? exception)
     {
-        if (this.eventHandlers.Error is { } error && TryGetSender(out var client))
-        {
-            error(client, new ConfigCatClientErrorEventArgs(message, exception));
-        }
+        this.eventHandlers.Error?.Invoke(this.client, new ConfigCatClientErrorEventArgs(message, exception));
     }
 
     protected class EventHandlers

src/ConfigCatClient/Hooks/SafeHooksWrapper.cs

@@ -0,0 +1,36 @@
+using System;
+using System.Runtime.CompilerServices;
+
+namespace ConfigCat.Client;
+
+// 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.ConfigChanged += (_, e) => { client.GetValue(...) }), that is, a chain like
+// AutoPollConfigService -> Hooks -> EventHandler.Target -> ConfigCatClient can be created, it is the hooks reference that we need to make weak.
+internal readonly struct SafeHooksWrapper
+{
+    private readonly WeakReference<Hooks> hooksWeakRef;
+
+    private Hooks Hooks => this.hooksWeakRef is { } hooksWeakRef && hooksWeakRef.TryGetTarget(out var hooks) ? hooks! : NullHooks.Instance;
+
+    public SafeHooksWrapper(Hooks hooks)
+    {
+        this.hooksWeakRef = new WeakReference<Hooks>(hooks);
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    public void RaiseClientReady() => Hooks.RaiseClientReady();
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    public void RaiseFlagEvaluated(EvaluationDetails evaluationDetails) => Hooks.RaiseFlagEvaluated(evaluationDetails);
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    public void RaiseConfigChanged(IConfig newConfig) => Hooks.RaiseConfigChanged(newConfig);
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    public void RaiseError(string message, Exception? exception) => Hooks.RaiseError(message, exception);
+
+    public static implicit operator SafeHooksWrapper(Hooks? hooks) => hooks is not null ? new SafeHooksWrapper(hooks) : default;
+}

src/ConfigCatClient/Logging/LoggerWrapper.cs

@@ -5,18 +5,18 @@ namespace ConfigCat.Client;
 internal sealed class LoggerWrapper : IConfigCatLogger
 {
     private readonly IConfigCatLogger logger;
-    private readonly Hooks hooks;
+    private readonly SafeHooksWrapper hooks;
 
     public LogLevel LogLevel
     {
         get => this.logger.LogLevel;
         set => this.logger.LogLevel = value;
     }
 
-    internal LoggerWrapper(IConfigCatLogger logger, Hooks? hooks = null)
+    internal LoggerWrapper(IConfigCatLogger logger, SafeHooksWrapper hooks = default)
     {
         this.logger = logger;
-        this.hooks = hooks ?? NullHooks.Instance;
+        this.hooks = hooks;
     }
 
     private bool TargetLogEnabled(LogLevel targetTrace)