diff --git a/src/ConfigCat.Client.Tests/ConfigCatClientTests.cs b/src/ConfigCat.Client.Tests/ConfigCatClientTests.cs index fd4f5f5d..b7ccb4b9 100644 --- a/src/ConfigCat.Client.Tests/ConfigCatClientTests.cs +++ b/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 loggerMock, Mock fetcherMock, diff --git a/src/ConfigCatClient/ConfigCatClient.cs b/src/ConfigCatClient/ConfigCatClient.cs index f933806d..0a66cf61 100644 --- a/src/ConfigCatClient/ConfigCatClient.cs +++ b/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 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 { diff --git a/src/ConfigCatClient/ConfigService/AutoPollConfigService.cs b/src/ConfigCatClient/ConfigService/AutoPollConfigService.cs index 6e2a720e..899fe22e 100644 --- a/src/ConfigCatClient/ConfigService/AutoPollConfigService.cs +++ b/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; diff --git a/src/ConfigCatClient/ConfigService/ConfigServiceBase.cs b/src/ConfigCatClient/ConfigService/ConfigServiceBase.cs index 7f63683a..04f33d95 100644 --- a/src/ConfigCatClient/ConfigService/ConfigServiceBase.cs +++ b/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; } diff --git a/src/ConfigCatClient/ConfigService/LazyLoadConfigService.cs b/src/ConfigCatClient/ConfigService/LazyLoadConfigService.cs index 4dc4ab88..2ea94583 100644 --- a/src/ConfigCatClient/ConfigService/LazyLoadConfigService.cs +++ b/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() diff --git a/src/ConfigCatClient/ConfigService/ManualPollConfigService.cs b/src/ConfigCatClient/ConfigService/ManualPollConfigService.cs index 33290ce2..25057295 100644 --- a/src/ConfigCatClient/ConfigService/ManualPollConfigService.cs +++ b/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() diff --git a/src/ConfigCatClient/ConfigService/NullConfigService.cs b/src/ConfigCatClient/ConfigService/NullConfigService.cs index 8dc4355b..97c83327 100644 --- a/src/ConfigCatClient/ConfigService/NullConfigService.cs +++ b/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; diff --git a/src/ConfigCatClient/Configuration/ConfigCatClientOptions.cs b/src/ConfigCatClient/Configuration/ConfigCatClientOptions.cs index 5ebfdf4a..dfb374c9 100644 --- a/src/ConfigCatClient/Configuration/ConfigCatClientOptions.cs +++ b/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(); + /// /// The logger implementation to use for performing logging. /// If not set, with will be used by default.
@@ -88,7 +90,12 @@ public Uri BaseUrl /// 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) /// public event EventHandler? ClientReady { - add { Hooks.ClientReady += value; } - remove { Hooks.ClientReady -= value; } + add { this.hooks.ClientReady += value; } + remove { this.hooks.ClientReady -= value; } } /// public event EventHandler? FlagEvaluated { - add { Hooks.FlagEvaluated += value; } - remove { Hooks.FlagEvaluated -= value; } + add { this.hooks.FlagEvaluated += value; } + remove { this.hooks.FlagEvaluated -= value; } } /// public event EventHandler? ConfigChanged { - add { Hooks.ConfigChanged += value; } - remove { Hooks.ConfigChanged -= value; } + add { this.hooks.ConfigChanged += value; } + remove { this.hooks.ConfigChanged -= value; } } /// public event EventHandler? Error { - add { Hooks.Error += value; } - remove { Hooks.Error -= value; } + add { this.hooks.Error += value; } + remove { this.hooks.Error -= value; } } } diff --git a/src/ConfigCatClient/Hooks/Hooks.cs b/src/ConfigCatClient/Hooks/Hooks.cs index 61482eb3..f8f835e6 100644 --- a/src/ConfigCatClient/Hooks/Hooks.cs +++ b/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? 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(client); + this.client = client; } /// @@ -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); } /// @@ -71,13 +52,9 @@ public event EventHandler? 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)); } /// @@ -87,13 +64,9 @@ public event EventHandler? 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)); } /// @@ -103,13 +76,9 @@ public event EventHandler? 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 diff --git a/src/ConfigCatClient/Hooks/SafeHooksWrapper.cs b/src/ConfigCatClient/Hooks/SafeHooksWrapper.cs new file mode 100644 index 00000000..bb763323 --- /dev/null +++ b/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 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); + } + + [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; +} diff --git a/src/ConfigCatClient/Logging/LoggerWrapper.cs b/src/ConfigCatClient/Logging/LoggerWrapper.cs index c7ab971d..6d2d39e6 100644 --- a/src/ConfigCatClient/Logging/LoggerWrapper.cs +++ b/src/ConfigCatClient/Logging/LoggerWrapper.cs @@ -5,7 +5,7 @@ namespace ConfigCat.Client; internal sealed class LoggerWrapper : IConfigCatLogger { private readonly IConfigCatLogger logger; - private readonly Hooks hooks; + private readonly SafeHooksWrapper hooks; public LogLevel LogLevel { @@ -13,10 +13,10 @@ public LogLevel 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)