src/NLog/Config/XmlLoggingConfiguration.cs (1)

564-737: Consider replacing the static dictionary with ConditionalWeakTable

Even with the leak fix above, a static cache keyed by LogFactory can grow unbounded in applications that create many transient factories (e.g., in unit tests).
ConditionalWeakTable<LogFactory, AutoReloadConfigFileWatcher> would automatically release entries when the factory becomes unreachable, without explicit bookkeeping.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

src/NLog/Config/XmlLoggingConfiguration.cs (4)

570-570: ⚠️ Potential issue

Thread-safety: make _isDisposing volatile

_isDisposing is read from multiple threads (timer thread, FileSystemWatcher thread) without a lock. Without the volatile modifier there is no guarantee other threads will observe the write performed in Dispose(), leading to possible use-after-dispose race conditions.

-            private bool _isDisposing;
+            private volatile bool _isDisposing;

---

576-578: ⚠️ Potential issue

Dead-code / missing subscription for LogFactory.ConfigurationChanged

The AutoReloadConfigFileWatcher class implements LogFactory_ConfigurationChanged, but the constructor never subscribes to the event, and Dispose() never unsubscribes.

Either wire up the event or remove the unused method:

            public AutoReloadConfigFileWatcher(LogFactory logFactory)
            {
                _logFactory = logFactory;
                _fileWatcher.FileChanged += FileWatcher_FileChanged;
+               _logFactory.ConfigurationChanged += LogFactory_ConfigurationChanged;
            }

And in the Dispose method:

            public void Dispose()
            {
                _isDisposing = true;
                _fileWatcher.FileChanged -= FileWatcher_FileChanged;
+               _logFactory.ConfigurationChanged -= LogFactory_ConfigurationChanged;
                lock (_lockObject)
                {
                    var reloadTimer = _reloadTimer;
                    _reloadTimer = null;
                    reloadTimer?.Dispose();
                }
                _fileWatcher.Dispose();
            }

---

56-56: ⚠️ Potential issue

Potential memory leak in the static _watchers cache

The _watchers dictionary maintains strong references to each LogFactory instance. When a factory is disposed, its watcher is disposed too, but the dictionary entry is never removed. This could prevent both the LogFactory and the AutoReloadConfigFileWatcher instances from being garbage-collected.

Add cleanup code to remove entries when no longer needed:

-        private static readonly Dictionary<LogFactory, AutoReloadConfigFileWatcher> _watchers = new Dictionary<LogFactory, AutoReloadConfigFileWatcher>();
+        private static readonly Dictionary<LogFactory, AutoReloadConfigFileWatcher> _watchers = new Dictionary<LogFactory, AutoReloadConfigFileWatcher>(WeakReferenceComparer.Default);

Or add code to remove entries when the watcher is disposed:

         private void OnConfigurationAssigned(LogFactory logFactory)
         {
             // ...
             if (logFactory is null || !AutoReload)
             {
                 if (fileWatcher != null)
                 {
                     InternalLogger.Info("AutoReload Config File Monitor stopping, since no active configuration");
                     fileWatcher.Dispose();
+                    lock (_watchers)
+                    {
+                        _watchers.Remove(configFactory);
+                    }
                 }
             }
             // ...
         }

---

220-221: ⚠️ Potential issue

Missing dictionary cleanup when disposing watchers

When the auto-reload feature is disabled or the LogFactory is null, the watcher is disposed but the entry remains in the dictionary. This could lead to a memory leak over time.

                    if (fileWatcher != null)
                    {
                        InternalLogger.Info("AutoReload Config File Monitor stopping, since no active configuration");
                        fileWatcher.Dispose();
+                        lock (_watchers)
+                        {
+                            _watchers.Remove(configFactory);
+                        }
                    }

src/NLog/Config/XmlLoggingConfiguration.cs (3)

610-631: Consider using CancellationTokenSource for better timer management

The current approach of creating and disposing Timer objects could be simplified using a CancellationTokenSource with a delay. This would make the code more maintainable and reduce the chance of resource leaks.

-            private Timer _reloadTimer;
+            private CancellationTokenSource _reloadCts;

             private void FileWatcher_FileChanged(object sender, System.IO.FileSystemEventArgs e)
             {
                 lock (_lockObject)
                 {
                     if (_isDisposing)
                         return;

-                    var reloadTimer = _reloadTimer;
-                    if (reloadTimer is null)
+                    var currentCts = _reloadCts;
+                    if (currentCts != null)
                     {
-                        var currentConfig = _logFactory.Configuration;
-                        if (currentConfig is null)
-                            return;
-
-                        _reloadTimer = new Timer((s) => ReloadTimer(s), currentConfig, 1000, Timeout.Infinite);
+                        currentCts.Cancel();
                     }
-                    else
+                    
+                    var currentConfig = _logFactory.Configuration;
+                    if (currentConfig is null)
+                        return;
+                        
+                    _reloadCts = new CancellationTokenSource();
+                    var token = _reloadCts.Token;
+                    Task.Delay(1000, token).ContinueWith(t => 
                     {
-                        reloadTimer.Change(1000, Timeout.Infinite);
+                        if (!t.IsCanceled)
+                            ReloadTimer(currentConfig);
+                    }, TaskContinuationOptions.NotOnCanceled);
                 }
             }

---

633-639: Use early return pattern consistently

The early return pattern is used inconsistently. For better readability, consider applying it more uniformly.

         private void ReloadTimer(object state)
         {
             if (_isDisposing)
-            {
-                return; //timer was disposed already.
-            }
+                return; //timer was disposed already.
 
             LoggingConfiguration oldConfig = (LoggingConfiguration)state;

---

716-718: Release all resources in Dispose method

Ensure all resources are properly released in the Dispose method, including any event subscriptions that might have been set up.

            public void Dispose()
            {
                _isDisposing = true;
                _fileWatcher.FileChanged -= FileWatcher_FileChanged;
+               // If you choose to implement ConfigurationChanged event subscription
+               // _logFactory.ConfigurationChanged -= LogFactory_ConfigurationChanged;
                lock (_lockObject)
                {
                    var reloadTimer = _reloadTimer;
                    _reloadTimer = null;
                    reloadTimer?.Dispose();
                }
                _fileWatcher.Dispose();
+               TryUnwatchConfigFile(); // Ensure file watching is stopped
            }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

src/NLog/Config/XmlLoggingConfiguration.cs (3)

575-575: ⚠️ Potential issue

Thread-safety: make _isDisposing volatile

_isDisposing is read from multiple threads (timer thread, FileSystemWatcher thread) without a lock. Without the volatile modifier there is no guarantee other threads will observe the write performed in Dispose(), leading to possible use-after-dispose race conditions.

-            private bool _isDisposing;
+            private volatile bool _isDisposing;

---

56-57: ⚠️ Potential issue

Fix potential memory leak in static _watchers dictionary

The _watchers dictionary maintains strong references to LogFactory instances, but entries are never removed when factories are disposed or auto-reload is disabled.

Update the OnConfigurationAssigned method to remove disposed entries:

                    if (fileWatcher != null)
                    {
                        InternalLogger.Info("AutoReload Config File Monitor stopping, since no active configuration");
                        fileWatcher.Dispose();
+                        lock (_watchers)
+                        {
+                            _watchers.Remove(configFactory);
+                        }
                    }

---

579-583: 🛠️ Refactor suggestion

Missing event subscription for LogFactory_ConfigurationChanged

The class implements a method for handling LogFactory.ConfigurationChanged events, but never subscribes to this event in the constructor or unsubscribes in Dispose().

Either wire up the event handler or remove the unused method to avoid confusion:

            public AutoReloadConfigFileWatcher(LogFactory logFactory)
            {
                _logFactory = logFactory;
                _fileWatcher.FileChanged += FileWatcher_FileChanged;
+                _logFactory.ConfigurationChanged += LogFactory_ConfigurationChanged;
            }

            // And in Dispose():
            public void Dispose()
            {
                _isDisposing = true;
                _fileWatcher.FileChanged -= FileWatcher_FileChanged;
+                _logFactory.ConfigurationChanged -= LogFactory_ConfigurationChanged;
                lock (_lockObject)
                {
                    var reloadTimer = _reloadTimer;
                    _reloadTimer = null;
                    reloadTimer?.Dispose();
                }
                _fileWatcher.Dispose();
            }

tests/NLog.UnitTests/Config/ReloadTests.cs (1)

530-533: Consider migrating remaining uses of obsolete property

While pragma directives correctly suppress the warning, for consistency and future maintenance it would be better to migrate these remaining usages to AutoReloadFileNames as well.

-#pragma warning disable CS0618 // Type or member is obsolete
-                Assert.Single(logFactory.Configuration.FileNamesToWatch);
-                Assert.Equal(mainConfigFilePath, logFactory.Configuration.FileNamesToWatch.FirstOrDefault());
-#pragma warning restore CS0618 // Type or member is obsolete
+                Assert.Single(((XmlLoggingConfiguration)logFactory.Configuration).AutoReloadFileNames);
+                Assert.Equal(mainConfigFilePath, ((XmlLoggingConfiguration)logFactory.Configuration).AutoReloadFileNames.FirstOrDefault());

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro