LoggingConfiguration - Modify and clone LoggingRules under lock

src/NLog/Config/LoggingConfiguration.cs

@@ -104,6 +104,9 @@ public LoggingConfiguration()
         /// </summary>
         public IList<LoggingRule> LoggingRules { get; private set; }
 
+        internal List<LoggingRule> GetLoggingRulesThreadSafe() {  lock (LoggingRules) return LoggingRules.ToList(); }
+        private void AddLoggingRulesThreadSafe(LoggingRule rule) { lock (LoggingRules) LoggingRules.Add(rule); }
+
         /// <summary>
         /// Gets or sets the default culture info to use as <see cref="LogEventInfo.FormatProvider"/>.
         /// </summary>
@@ -156,7 +159,7 @@ public int GetHashCode(Target obj)
         /// <exception cref="ArgumentNullException">when <paramref name="target"/> is <see langword="null"/></exception>
         public void AddTarget([NotNull] Target target)
         {
-            if (target == null) { throw new ArgumentNullException("target"); }
+            if (target == null) { throw new ArgumentNullException(nameof(target)); }
             AddTarget(target.Name, target);
         }
 
@@ -176,10 +179,10 @@ public void AddTarget(string name, Target target)
             if (name == null)
             {
                 // TODO: NLog 5 - The ArgumentException should be changed to ArgumentNullException for name parameter.
-                throw new ArgumentException("Target name cannot be null", "name");
+                throw new ArgumentException("Target name cannot be null", nameof(name));
             }
 
-            if (target == null) { throw new ArgumentNullException("target"); }
+            if (target == null) { throw new ArgumentNullException(nameof(target)); }
 
             InternalLogger.Debug("Registering target {0}: {1}", name, target.GetType().FullName);
             _targets[name] = target;
@@ -249,7 +252,8 @@ public void AddRule(LogLevel minLevel, LogLevel maxLevel, string targetName, str
         /// <param name="loggerNamePattern">Logger name pattern. It may include the '*' wildcard at the beginning, at the end or at both ends.</param>
         public void AddRule(LogLevel minLevel, LogLevel maxLevel, Target target, string loggerNamePattern = "*")
         {
-            LoggingRules.Add(new LoggingRule(loggerNamePattern, minLevel, maxLevel, target));
+            if (target == null) { throw new ArgumentNullException(nameof(target)); }
+            AddLoggingRulesThreadSafe(new LoggingRule(loggerNamePattern, minLevel, maxLevel, target));
         }
 
         /// <summary>
@@ -277,9 +281,10 @@ public void AddRuleForOneLevel(LogLevel level, string targetName, string loggerN
         /// <param name="loggerNamePattern">Logger name pattern. It may include the '*' wildcard at the beginning, at the end or at both ends.</param>
         public void AddRuleForOneLevel(LogLevel level, Target target, string loggerNamePattern = "*")
         {
+            if (target == null) { throw new ArgumentNullException(nameof(target)); }
             var loggingRule = new LoggingRule(loggerNamePattern, target);
             loggingRule.EnableLoggingForLevel(level);
-            LoggingRules.Add(loggingRule);
+            AddLoggingRulesThreadSafe(loggingRule);
         }
 
         /// <summary>
@@ -305,9 +310,10 @@ public void AddRuleForAllLevels(string targetName, string loggerNamePattern = "*
         /// <param name="loggerNamePattern">Logger name pattern. It may include the '*' wildcard at the beginning, at the end or at both ends.</param>
         public void AddRuleForAllLevels(Target target, string loggerNamePattern = "*")
         {
+            if (target == null) { throw new ArgumentNullException(nameof(target)); }
             var loggingRule = new LoggingRule(loggerNamePattern, target);
             loggingRule.EnableLoggingForLevels(LogLevel.MinLevel, LogLevel.MaxLevel);
-            LoggingRules.Add(loggingRule);
+            AddLoggingRulesThreadSafe(loggingRule);
         }
 
         /// <summary>
@@ -463,8 +469,7 @@ internal void Dump()
             }
 
             InternalLogger.Debug("Rules:");
-            var loggingRules = LoggingRules.ToList();
-            foreach (LoggingRule rule in loggingRules)
+            foreach (LoggingRule rule in GetLoggingRulesThreadSafe())
             {
                 InternalLogger.Debug("{0}", rule);
             }
@@ -481,8 +486,7 @@ internal void FlushAllTargets(AsyncContinuation asyncContinuation)
             InternalLogger.Trace("Flushing all targets...");
 
             var uniqueTargets = new List<Target>();
-            var loggingRules = LoggingRules.ToList();
-            foreach (var rule in loggingRules)
+            foreach (var rule in GetLoggingRulesThreadSafe())
             {
                 var targetList = rule.Targets.ToList();
                 foreach (var target in targetList)
@@ -504,8 +508,7 @@ internal void ValidateConfig()
         {
             var roots = new List<object>();
 
-            var loggingRules = LoggingRules.ToList();
-            foreach (LoggingRule rule in loggingRules)
+            foreach (LoggingRule rule in GetLoggingRulesThreadSafe())
             {
                 roots.Add(rule);
             }

src/NLog/Config/LoggingRule.cs

@@ -37,9 +37,10 @@ namespace NLog.Config
     using System.Collections.Generic;
     using System.Collections.ObjectModel;
     using System.Globalization;
+    using System.Linq;
     using System.Text;
-    using Filters;
-    using Targets;
+    using NLog.Filters;
+    using NLog.Targets;
 
     /// <summary>
     /// Represents a logging rule. An equivalent of &lt;logger /&gt; configuration element.
@@ -126,6 +127,8 @@ internal enum MatchMode
         /// </summary>
         public IList<LoggingRule> ChildRules { get; private set; }
 
+        internal List<LoggingRule> CloneChildRulesThreadSafe() { lock (ChildRules) return ChildRules.ToList(); }
+
         /// <summary>
         /// Gets a collection of filters to be checked before writing to targets.
         /// </summary>

src/NLog/Config/SimpleConfigurator.cs

@@ -33,7 +33,8 @@
 
 namespace NLog.Config
 {
-    using Targets;
+    using System;
+    using NLog.Targets;
 
     /// <summary>
     /// Provides simple programmatic configuration API used for trivial logging cases.
@@ -44,7 +45,7 @@ public static class SimpleConfigurator
     {
         /// <summary>
         /// Configures NLog for console logging so that all messages above and including
-        /// the <see cref="LogLevel.Info"/> level are output to the console.
+        /// the <see cref="NLog.LogLevel.Info"/> level are output to the console.
         /// </summary>
         public static void ConfigureForConsoleLogging()
         {
@@ -62,18 +63,18 @@ public static void ConfigureForConsoleLogging(LogLevel minLevel)
             ConsoleTarget consoleTarget = new ConsoleTarget();
 
             LoggingConfiguration config = new LoggingConfiguration();
-            LoggingRule rule = new LoggingRule("*", minLevel, consoleTarget);
-            config.LoggingRules.Add(rule);
+            config.AddRule(minLevel, LogLevel.MaxLevel, consoleTarget, "*");
             LogManager.Configuration = config;
         }
 
         /// <summary>
         /// Configures NLog for to log to the specified target so that all messages 
-        /// above and including the <see cref="LogLevel.Info"/> level are output.
+        /// above and including the <see cref="NLog.LogLevel.Info"/> level are output.
         /// </summary>
         /// <param name="target">The target to log all messages to.</param>
         public static void ConfigureForTargetLogging(Target target)
         {
+            if (target == null) { throw new ArgumentNullException(nameof(target)); }
             ConfigureForTargetLogging(target, LogLevel.Info);
         }
 
@@ -85,15 +86,15 @@ public static void ConfigureForTargetLogging(Target target)
         /// <param name="minLevel">The minimal logging level.</param>
         public static void ConfigureForTargetLogging(Target target, LogLevel minLevel)
         {
+            if (target == null) { throw new ArgumentNullException(nameof(target)); }
             LoggingConfiguration config = new LoggingConfiguration();
-            LoggingRule rule = new LoggingRule("*", minLevel, target);
-            config.LoggingRules.Add(rule);
+            config.AddRule(minLevel, LogLevel.MaxLevel, target, "*");
             LogManager.Configuration = config;
         }
 
         /// <summary>
         /// Configures NLog for file logging so that all messages above and including
-        /// the <see cref="LogLevel.Info"/> level are written to the specified file.
+        /// the <see cref="NLog.LogLevel.Info"/> level are written to the specified file.
         /// </summary>
         /// <param name="fileName">Log file name.</param>
         public static void ConfigureForFileLogging(string fileName)

src/NLog/Config/XmlLoggingConfiguration.cs

@@ -38,21 +38,20 @@ namespace NLog.Config
     using System;
     using System.Collections;
     using System.Collections.Generic;
-    using System.Diagnostics;
+    using System.Collections.ObjectModel;
     using System.Globalization;
-    using System.Linq;
     using System.IO;
+    using System.Linq;
     using System.Reflection;
     using System.Xml;
-    using Common;
-    using Filters;
-    using Internal;
-    using Layouts;
-    using Targets;
-    using Targets.Wrappers;
-    using LayoutRenderers;
-    using Time;
-    using System.Collections.ObjectModel;
+    using NLog.Common;
+    using NLog.Filters;
+    using NLog.Internal;
+    using NLog.LayoutRenderers;
+    using NLog.Layouts;
+    using NLog.Targets;
+    using NLog.Targets.Wrappers;
+    using NLog.Time;
 #if SILVERLIGHT
 // ReSharper disable once RedundantUsingDirective
     using System.Windows;
@@ -490,7 +489,7 @@ private void CheckUnusedTargets()
             ReadOnlyCollection<Target> configuredNamedTargets = ConfiguredNamedTargets; //assign to variable because `ConfiguredNamedTargets` computes a new list every time.
             InternalLogger.Debug("Unused target checking is started... Rule Count: {0}, Target Count: {1}", LoggingRules.Count, configuredNamedTargets.Count);
 
-            HashSet<string> targetNamesAtRules = new HashSet<string>(LoggingRules.SelectMany(r => r.Targets).Select(t => t.Name));
+            HashSet<string> targetNamesAtRules = new HashSet<string>(GetLoggingRulesThreadSafe().SelectMany(r => r.Targets).Select(t => t.Name));
             HashSet<string> wrappedTargetNames = new HashSet<string>(configuredNamedTargets.OfType<WrapperTargetBase>().Select(wt => wt.WrappedTarget.Name));
 
 
@@ -651,7 +650,6 @@ private void ParseNLogElement(NLogXmlElement nlogElement, string filePath, bool
                 }
             }
 
-
             foreach (var ruleChild in rulesList)
             {
                 ParseRulesElement(ruleChild, LoggingRules);
@@ -779,7 +777,10 @@ private void ParseLoggerElement(NLogXmlElement loggerElement, IList<LoggingRule>
                 }
             }
 
-            rulesCollection.Add(rule);
+            lock (rulesCollection)
+            {
+                rulesCollection.Add(rule);
+            }
         }
 
         private void ParseFilters(LoggingRule rule, NLogXmlElement filtersElement)

src/NLog/LogFactory.cs

@@ -848,10 +848,8 @@ internal void ReloadConfigOnTimer(object state)
             }
         }
 #endif
-        private void GetTargetsByLevelForLogger(string name, IEnumerable<LoggingRule> rules, TargetWithFilterChain[] targetsByLevel, TargetWithFilterChain[] lastTargetsByLevel, bool[] suppressedLevels)
+        private void GetTargetsByLevelForLogger(string name, List<LoggingRule> loggingRules, TargetWithFilterChain[] targetsByLevel, TargetWithFilterChain[] lastTargetsByLevel, bool[] suppressedLevels)
         {
-            //no "System.InvalidOperationException: Collection was modified"
-            var loggingRules = new List<LoggingRule>(rules);
             foreach (LoggingRule rule in loggingRules)
             {
                 if (!rule.NameMatches(name))
@@ -886,8 +884,10 @@ private void GetTargetsByLevelForLogger(string name, IEnumerable<LoggingRule> ru
                 }
 
                 // Recursively analyze the child rules.
-                GetTargetsByLevelForLogger(name, rule.ChildRules, targetsByLevel, lastTargetsByLevel, suppressedLevels);
-
+                if (rule.ChildRules.Count != 0)
+                {
+                    GetTargetsByLevelForLogger(name, rule.CloneChildRulesThreadSafe(), targetsByLevel, lastTargetsByLevel, suppressedLevels);
+                }
             }
 
             for (int i = 0; i <= LogLevel.MaxLevel.Ordinal; ++i)
@@ -908,7 +908,9 @@ internal LoggerConfiguration GetConfigurationForLogger(string name, LoggingConfi
 
             if (configuration != null && IsLoggingEnabled())
             {
-                GetTargetsByLevelForLogger(name, configuration.LoggingRules, targetsByLevel, lastTargetsByLevel, suppressedLevels);
+                //no "System.InvalidOperationException: Collection was modified"
+                var loggingRules = configuration.GetLoggingRulesThreadSafe();
+                GetTargetsByLevelForLogger(name, loggingRules, targetsByLevel, lastTargetsByLevel, suppressedLevels);
             }
 
             if (InternalLogger.IsDebugEnabled)