NLogLoggingConfiguration - Fix memory-leak when using AutoReload (#761)

NLog · Sep 12, 2024 · 320ae43 · 320ae43
1 parent c271301
commit 320ae43
Show file tree

Hide file tree

Showing 3 changed files with 69 additions and 40 deletions.
diff --git a/src/NLog.Extensions.Logging/Config/NLogLoggingConfiguration.cs b/src/NLog.Extensions.Logging/Config/NLogLoggingConfiguration.cs
@@ -14,7 +14,6 @@ public class NLogLoggingConfiguration : LoggingConfigurationParser
     {
         private readonly IConfigurationSection _originalConfigSection;
         private bool _autoReload;
-        private Action<object> _reloadConfiguration;
         private IDisposable _registerChangeCallback;
         private const string RootSectionKey = "NLog";
 
@@ -48,28 +47,30 @@ private NLogLoggingConfiguration(LogFactory logFactory, IConfigurationSection nl
             _originalConfigSection = nlogConfig;
         }
 
-        /// <summary>
-        /// Gets the collection of file names which should be watched for changes by NLog.
-        /// </summary>
-        public override IEnumerable<string> FileNamesToWatch
+        /// <inheritdoc />
+        public override LoggingConfiguration Reload()
         {
-            get
+            return ReloadLoggingConfiguration(_originalConfigSection);
+        }
+
+        /// <inheritdoc />
+        protected override void OnConfigurationAssigned(LogFactory logFactory)
+        {
+            _registerChangeCallback?.Dispose();
+
+            if (_autoReload)
             {
-                if (_autoReload && _reloadConfiguration is null)
+                if (logFactory is null)
                 {
-                    // Prepare for setting up reload notification handling
-                    _reloadConfiguration = state => ReloadConfigurationSection((IConfigurationSection)state);
-                    LogFactory.ConfigurationChanged += LogFactory_ConfigurationChanged;
+                    _autoReload = false;
+                }
+                else
+                {
+                    MonitorForReload(_originalConfigSection);
                 }
-
-                return Enumerable.Empty<string>();
             }
-        }
 
-        /// <inheritdoc />
-        public override LoggingConfiguration Reload()
-        {
-            return ReloadLoggingConfiguration(_originalConfigSection);
+            base.OnConfigurationAssigned(logFactory);
         }
 
         private LoggingConfiguration ReloadLoggingConfiguration(IConfigurationSection nlogConfig)
@@ -87,26 +88,6 @@ private void LoadConfigurationSection(IConfigurationSection nlogConfig)
             _autoReload = configElement.AutoReload;
         }
 
-        private void LogFactory_ConfigurationChanged(object sender, LoggingConfigurationChangedEventArgs e)
-        {
-            if (ReferenceEquals(e.DeactivatedConfiguration, this))
-            {
-                if (_autoReload)
-                {
-                    _autoReload = false; // Cannot unsubscribe to reload event, but we can stop reacting to it
-                    LogFactory.ConfigurationChanged -= LogFactory_ConfigurationChanged;
-                    _registerChangeCallback?.Dispose();
-                    _registerChangeCallback = null;
-                }
-            }
-            else if (ReferenceEquals(e.ActivatedConfiguration, this) && _autoReload && _reloadConfiguration != null)
-            {
-                // Setup reload notification
-                LogFactory.ConfigurationChanged += LogFactory_ConfigurationChanged;
-                MonitorForReload(_originalConfigSection);
-            }
-        }
-
         private void ReloadConfigurationSection(IConfigurationSection nlogConfig)
         {
             try
@@ -135,7 +116,7 @@ private void MonitorForReload(IConfigurationSection nlogConfig)
         {
             _registerChangeCallback?.Dispose();
             _registerChangeCallback = null;
-            _registerChangeCallback = nlogConfig.GetReloadToken().RegisterChangeCallback(_reloadConfiguration, nlogConfig);
+            _registerChangeCallback = new AutoReloadConfigChangeMonitor(nlogConfig, this);
         }
 
         /// <inheritdoc />
@@ -489,5 +470,30 @@ public override string ToString()
                 return Name;
             }
         }
+
+        private sealed class AutoReloadConfigChangeMonitor : IDisposable
+        {
+            private NLogLoggingConfiguration _nlogConfig;
+            private IDisposable _registerChangeCallback;
+
+            public AutoReloadConfigChangeMonitor(IConfigurationSection configSection, NLogLoggingConfiguration nlogConfig)
+            {
+                _nlogConfig = nlogConfig;
+                _registerChangeCallback = configSection.GetReloadToken().RegisterChangeCallback((s) => ReloadConfigurationSection((IConfigurationSection)s), configSection);
+            }
+
+            private void ReloadConfigurationSection(IConfigurationSection configSection)
+            {
+                _nlogConfig?.ReloadConfigurationSection(configSection);
+            }
+
+            public void Dispose()
+            {
+                _nlogConfig = null; // Disconnect to allow garbage collection
+                var registerChangeCallback = _registerChangeCallback;
+                _registerChangeCallback = null;
+                registerChangeCallback?.Dispose();
+            }
+        }
     }
 }
diff --git a/src/NLog.Extensions.Logging/NLog.Extensions.Logging.csproj b/src/NLog.Extensions.Logging/NLog.Extensions.Logging.csproj
@@ -87,7 +87,7 @@ List of major changes in NLog 5.0: https://nlog-project.org/2021/08/25/nlog-5-0-
   </PropertyGroup>
 
   <ItemGroup>
-    <PackageReference Include="NLog" Version="5.3.3" />
+    <PackageReference Include="NLog" Version="5.3.4" />
     <PackageReference Include="Microsoft.SourceLink.GitHub" Version="8.0.0" PrivateAssets="All" />
   </ItemGroup>
   <ItemGroup Condition=" '$(TargetFramework)' == 'net461' ">

diff --git a/test/NLog.Extensions.Logging.Tests/NLogLoggingConfigurationTests.cs b/test/NLog.Extensions.Logging.Tests/NLogLoggingConfigurationTests.cs
@@ -1,4 +1,5 @@
-using System.Collections.Generic;
+using System;
+using System.Collections.Generic;
 using System.IO;
 using System.Linq;
 using System.Text;
@@ -386,6 +387,28 @@ public void ReloadLogFactoryConfiguration()
             configuration.Reload(); // Nothing should happen
         }
 
+        [Fact]
+        public void ReloadLogFactoryConfigurationWithoutLeak()
+        {
+            var memoryConfig = CreateMemoryConfigConsoleTargetAndRule();
+            memoryConfig["NLog:Targets:file:type"] = "File";
+            memoryConfig["NLog:Targets:file:fileName"] = "hello.txt";
+            memoryConfig["NLog:AutoReload"] = "true";
+            var configuration = new ConfigurationBuilder().AddInMemoryCollection(memoryConfig).Build();
+            var logFactory = new LogFactory();
+            var newConfig = CreateConfiguration(logFactory, configuration);
+            configuration.Reload();
+            GC.Collect();
+            Assert.False(newConfig.TryGetTarget(out _));
+        }
+
+        static WeakReference<NLogLoggingConfiguration> CreateConfiguration(LogFactory logFactory, IConfigurationRoot config)
+        {
+            var nlogConfig = new NLogLoggingConfiguration(config.GetSection("NLog"), logFactory);
+            logFactory.Configuration = nlogConfig;
+            return new WeakReference<NLogLoggingConfiguration>(nlogConfig);
+        }
+
         [Fact]
         public void ReloadLogFactoryConfigurationKeepVariables()
         {