From 22a3b1d2b8edd4fe9b22a81ae8a349e2ecfbf4bf Mon Sep 17 00:00:00 2001 From: Jan Friedrich Date: Fri, 26 Jul 2024 10:37:28 +0200 Subject: [PATCH 1/4] #156 fixed Regression when creating nested loggers in reverse order --- src/log4net.Tests/Hierarchy/HierarchyTest.cs | 125 +++++++++++------- src/log4net/Repository/Hierarchy/Hierarchy.cs | 64 ++++++--- src/log4net/Repository/Hierarchy/LoggerKey.cs | 12 +- src/site/xdoc/release/release-notes.xml | 3 + 4 files changed, 128 insertions(+), 76 deletions(-) diff --git a/src/log4net.Tests/Hierarchy/HierarchyTest.cs b/src/log4net.Tests/Hierarchy/HierarchyTest.cs index 7cd39ae3..68c9451e 100644 --- a/src/log4net.Tests/Hierarchy/HierarchyTest.cs +++ b/src/log4net.Tests/Hierarchy/HierarchyTest.cs @@ -22,9 +22,7 @@ using System; using System.Xml; using log4net.Config; -using log4net.Core; using log4net.Repository; -using log4net.Repository.Hierarchy; using log4net.Tests.Appender; using NUnit.Framework; @@ -39,19 +37,19 @@ public void SetRepositoryPropertiesInConfigFile() // LOG4NET-53: Allow repository properties to be set in the config file XmlDocument log4netConfig = new XmlDocument(); log4netConfig.LoadXml(@" - - - - - - - - - - - - - "); + + + + + + + + + + + + + "); ILoggerRepository rep = LogManager.CreateRepository(Guid.NewGuid().ToString()); XmlConfigurator.Configure(rep, log4netConfig["log4net"]!); @@ -101,18 +99,18 @@ public void LoggerNameCanConsistOfASingleDot() { XmlDocument log4netConfig = new XmlDocument(); log4netConfig.LoadXml(@" - - - - - - - - - - - - "); + + + + + + + + + + + + "); ILoggerRepository rep = LogManager.CreateRepository(Guid.NewGuid().ToString()); XmlConfigurator.Configure(rep, log4netConfig["log4net"]!); @@ -123,18 +121,18 @@ public void LoggerNameCanConsistOfASingleNonDot() { XmlDocument log4netConfig = new XmlDocument(); log4netConfig.LoadXml(@" - - - - - - - - - - - - "); + + + + + + + + + + + + "); ILoggerRepository rep = LogManager.CreateRepository(Guid.NewGuid().ToString()); XmlConfigurator.Configure(rep, log4netConfig["log4net"]!); @@ -145,21 +143,46 @@ public void LoggerNameCanContainSequenceOfDots() { XmlDocument log4netConfig = new XmlDocument(); log4netConfig.LoadXml(@" - - - - - - - - - - - - "); + + + + + + + + + + + + "); ILoggerRepository rep = LogManager.CreateRepository(Guid.NewGuid().ToString()); XmlConfigurator.Configure(rep, log4netConfig["log4net"]!); } + + /// + /// https://github.com/apache/logging-log4net/issues/156 + /// Regression: Creating nested loggers in reverse order fails in 3.0.0-preview.1 + /// + [Test] + public void CreateNestedLoggersInReverseOrder() + { + XmlDocument log4netConfig = new XmlDocument(); + log4netConfig.LoadXml(@" + + + + + + + + + "); + + ILoggerRepository rep = LogManager.CreateRepository(Guid.NewGuid().ToString()); + XmlConfigurator.Configure(rep, log4netConfig["log4net"]!); + Assert.AreEqual("A.B.C", rep.GetLogger("A.B.C").Name); + Assert.AreEqual("A.B", rep.GetLogger("A.B").Name); + } } } diff --git a/src/log4net/Repository/Hierarchy/Hierarchy.cs b/src/log4net/Repository/Hierarchy/Hierarchy.cs index 8e7a9625..72f56612 100644 --- a/src/log4net/Repository/Hierarchy/Hierarchy.cs +++ b/src/log4net/Repository/Hierarchy/Hierarchy.cs @@ -50,7 +50,7 @@ namespace log4net.Repository.Hierarchy /// public class LoggerCreationEventArgs : EventArgs { - /// + /// /// Constructor /// /// The that has been created. @@ -660,43 +660,65 @@ public Logger GetLogger(string name, ILoggerFactory factory) var key = new LoggerKey(name); - // Synchronize to prevent write conflicts. Read conflicts (in - // GetEffectiveLevel() method) are possible only if variable - // assignments are non-atomic. + const int maxRetries = 5; + for (int i = 0; i < maxRetries; i++) + { + if (TryCreateLogger(key, factory) is Logger result) + { + return result; + } + } + throw new InvalidOperationException( + $"GetLogger failed, because possibly too many threads are messing with creating the logger {name}!"); + } + private Logger? TryCreateLogger(LoggerKey key, ILoggerFactory factory) + { if (!m_ht.TryGetValue(key, out object? node)) { - return CreateLogger(null); + Logger newLogger = CreateLogger(key.Name); + node = m_ht.GetOrAdd(key, newLogger); + if (node == newLogger) + { + RegisterLogger(newLogger); + } } - if (node is Logger nodeLogger) + if (node is Logger logger) { - return nodeLogger; + return logger; } - if (node is ProvisionNode nodeProvisionNode) + if (node is ProvisionNode provisionNode) { - return CreateLogger(l => UpdateChildren(nodeProvisionNode, l)); + Logger newLogger = CreateLogger(key.Name); + if (m_ht.TryUpdate(key, newLogger, node)) + { + UpdateChildren(provisionNode, newLogger); + RegisterLogger(newLogger); + return newLogger; + } + return null; } // It should be impossible to arrive here but let's keep the compiler happy. - return null!; + throw new InvalidOperationException("TryCreateLogger failed, because a node is neither a Logger nor a ProvisionNode!"); - Logger CreateLogger(Action? extraInit) + Logger CreateLogger(string name) { - // Use GetOrAdd in case the logger was added after checking above. - return (Logger)m_ht.GetOrAdd(key, _ => - { - Logger logger = factory.CreateLogger(this, name); - logger.Hierarchy = this; - extraInit?.Invoke(logger); - UpdateParents(logger); - OnLoggerCreationEvent(logger); - return logger; - }); + Logger result = factory.CreateLogger(this, name); + result.Hierarchy = this; + return result; + } + + void RegisterLogger(Logger logger) + { + UpdateParents(logger); + OnLoggerCreationEvent(logger); } } + /// /// Sends a logger creation event to all registered listeners /// diff --git a/src/log4net/Repository/Hierarchy/LoggerKey.cs b/src/log4net/Repository/Hierarchy/LoggerKey.cs index aaf553d0..0acc21af 100644 --- a/src/log4net/Repository/Hierarchy/LoggerKey.cs +++ b/src/log4net/Repository/Hierarchy/LoggerKey.cs @@ -40,7 +40,7 @@ namespace log4net.Repository.Hierarchy /// /// Nicko Cadell /// Gert Driesen - [DebuggerDisplay("{m_name}")] + [DebuggerDisplay("{Name}")] internal readonly struct LoggerKey { /// @@ -65,7 +65,7 @@ internal readonly struct LoggerKey /// The name of the logger. internal LoggerKey(string name) { - m_name = string.Intern(name); + Name = string.Intern(name); m_hashCache = name.GetHashCode(); } @@ -83,7 +83,11 @@ public override int GetHashCode() return m_hashCache; } - private readonly string m_name; + /// + /// Name of the Logger + /// + internal string Name { get; } + private readonly int m_hashCache; public static Comparer ComparerInstance { get; } = new(); @@ -92,7 +96,7 @@ public sealed class Comparer : IEqualityComparer { public bool Equals(LoggerKey x, LoggerKey y) { - return x.m_hashCache == y.m_hashCache && x.m_name == y.m_name; + return x.m_hashCache == y.m_hashCache && x.Name == y.Name; } public int GetHashCode(LoggerKey obj) => obj.m_hashCache; diff --git a/src/site/xdoc/release/release-notes.xml b/src/site/xdoc/release/release-notes.xml index ac04817a..274ecc1c 100644 --- a/src/site/xdoc/release/release-notes.xml +++ b/src/site/xdoc/release/release-notes.xml @@ -169,6 +169,9 @@ limitations under the License. Apache log4net 3.0.0 addresses reported issues:
From c0cffd213083f748dff55a89abba9954be1f3704 Mon Sep 17 00:00:00 2001 From: Jan Friedrich Date: Fri, 26 Jul 2024 14:30:47 +0200 Subject: [PATCH 2/4] #156 better names for private Hierarchy fields --- src/log4net/Repository/Hierarchy/Hierarchy.cs | 143 +++++------------- 1 file changed, 40 insertions(+), 103 deletions(-) diff --git a/src/log4net/Repository/Hierarchy/Hierarchy.cs b/src/log4net/Repository/Hierarchy/Hierarchy.cs index 72f56612..0a55dbeb 100644 --- a/src/log4net/Repository/Hierarchy/Hierarchy.cs +++ b/src/log4net/Repository/Hierarchy/Hierarchy.cs @@ -177,7 +177,7 @@ public Hierarchy(PropertiesDictionary properties, ILoggerFactory loggerFactory) throw new ArgumentNullException(nameof(loggerFactory)); } - m_defaultFactory = loggerFactory; + defaultFactory = loggerFactory; } /// @@ -203,22 +203,22 @@ public Logger Root { get { - if (m_root is null) + if (rootLogger is null) { lock (this) { - if (m_root is null) + if (rootLogger is null) { // Create the root logger - Logger root = m_defaultFactory.CreateLogger(this, null); + Logger root = defaultFactory.CreateLogger(this, null); root.Hierarchy = this; // Store root - m_root = root; + rootLogger = root; } } } - return m_root; + return rootLogger; } } @@ -232,11 +232,8 @@ public Logger Root /// public ILoggerFactory LoggerFactory { - get => m_defaultFactory; - set - { - m_defaultFactory = value ?? throw new ArgumentNullException(nameof(value)); - } + get => defaultFactory; + set => defaultFactory = value.EnsureNotNull(); } /// @@ -252,12 +249,7 @@ public ILoggerFactory LoggerFactory /// public override ILogger? Exists(string name) { - if (name is null) - { - throw new ArgumentNullException(nameof(name)); - } - - m_ht.TryGetValue(new LoggerKey(name), out object? o); + loggers.TryGetValue(new(name.EnsureNotNull()), out object? o); return o as Logger; } @@ -277,10 +269,10 @@ public override ILogger[] GetCurrentLoggers() // The accumulation in loggers is necessary because not all elements in // ht are Logger objects as there might be some ProvisionNodes // as well. - var loggers = new List(m_ht.Count); + var loggers = new List(this.loggers.Count); // Iterate through m_ht values - foreach (object node in m_ht.Values) + foreach (object node in this.loggers.Values) { if (node is Logger logger) { @@ -308,14 +300,7 @@ public override ILogger[] GetCurrentLoggers() /// The name of the logger to retrieve /// The logger object with the name specified public override ILogger GetLogger(string name) - { - if (name is null) - { - throw new ArgumentNullException(nameof(name)); - } - - return GetLogger(name, m_defaultFactory); - } + => GetLogger(name.EnsureNotNull(), defaultFactory); /// /// Shutting down a hierarchy will safely close and remove @@ -420,14 +405,10 @@ public override void ResetConfiguration() /// public override void Log(LoggingEvent logEvent) { - if (logEvent is null) - { - throw new ArgumentNullException(nameof(logEvent)); - } - + logEvent.EnsureNotNull(); if (logEvent.LoggerName is not null) { - GetLogger(logEvent.LoggerName, m_defaultFactory).Log(logEvent); + GetLogger(logEvent.LoggerName, defaultFactory).Log(logEvent); } } @@ -492,18 +473,14 @@ private static void CollectAppenders(List appenderList, IAppenderAtta /// /// the appender to use to log all logging events void IBasicRepositoryConfigurator.Configure(IAppender appender) - { - BasicRepositoryConfigure(appender); - } + => BasicRepositoryConfigure(appender); /// /// Initialize the log4net system using the specified appenders /// /// the appenders to use to log all logging events void IBasicRepositoryConfigurator.Configure(params IAppender[] appenders) - { - BasicRepositoryConfigure(appenders); - } + => BasicRepositoryConfigure(appenders); /// /// Initialize the log4net system using the specified appenders @@ -562,8 +539,7 @@ protected void XmlRepositoryConfigure(System.Xml.XmlElement element) using (new LogLog.LogReceivedAdapter(configurationMessages)) { - var config = new XmlHierarchyConfigurator(this); - config.Configure(element); + new XmlHierarchyConfigurator(this).Configure(element); } Configured = true; @@ -597,20 +573,12 @@ protected void XmlRepositoryConfigure(System.Xml.XmlElement element) /// public bool IsDisabled(Level level) { - if (level is null) - { - throw new ArgumentNullException(nameof(level)); - } - if (Configured) { - return Threshold > level; - } - else - { - // If not configured the hierarchy is effectively disabled - return true; + return Threshold > level.EnsureNotNull(); } + // If not configured the hierarchy is effectively disabled + return true; } /// @@ -627,10 +595,7 @@ public bool IsDisabled(Level level) /// invoking this method. /// /// - public void Clear() - { - m_ht.Clear(); - } + public void Clear() => loggers.Clear(); /// /// Returns a new logger instance named as the first parameter using @@ -649,14 +614,8 @@ public void Clear() /// public Logger GetLogger(string name, ILoggerFactory factory) { - if (name is null) - { - throw new ArgumentNullException(nameof(name)); - } - if (factory is null) - { - throw new ArgumentNullException(nameof(factory)); - } + name.EnsureNotNull(); + factory.EnsureNotNull(); var key = new LoggerKey(name); @@ -668,16 +627,16 @@ public Logger GetLogger(string name, ILoggerFactory factory) return result; } } - throw new InvalidOperationException( + throw new LogException( $"GetLogger failed, because possibly too many threads are messing with creating the logger {name}!"); } private Logger? TryCreateLogger(LoggerKey key, ILoggerFactory factory) { - if (!m_ht.TryGetValue(key, out object? node)) + if (!loggers.TryGetValue(key, out object? node)) { Logger newLogger = CreateLogger(key.Name); - node = m_ht.GetOrAdd(key, newLogger); + node = loggers.GetOrAdd(key, newLogger); if (node == newLogger) { RegisterLogger(newLogger); @@ -692,7 +651,7 @@ public Logger GetLogger(string name, ILoggerFactory factory) if (node is ProvisionNode provisionNode) { Logger newLogger = CreateLogger(key.Name); - if (m_ht.TryUpdate(key, newLogger, node)) + if (loggers.TryUpdate(key, newLogger, node)) { UpdateChildren(provisionNode, newLogger); RegisterLogger(newLogger); @@ -702,7 +661,7 @@ public Logger GetLogger(string name, ILoggerFactory factory) } // It should be impossible to arrive here but let's keep the compiler happy. - throw new InvalidOperationException("TryCreateLogger failed, because a node is neither a Logger nor a ProvisionNode!"); + throw new LogException("TryCreateLogger failed, because a node is neither a Logger nor a ProvisionNode!"); Logger CreateLogger(string name) { @@ -718,7 +677,6 @@ void RegisterLogger(Logger logger) } } - /// /// Sends a logger creation event to all registered listeners /// @@ -727,9 +685,7 @@ void RegisterLogger(Logger logger) /// Raises the logger creation event. /// protected virtual void OnLoggerCreationEvent(Logger logger) - { - LoggerCreatedEvent?.Invoke(this, new LoggerCreationEventArgs(logger)); - } + => LoggerCreatedEvent?.Invoke(this, new(logger)); /// /// Updates all the parents of the specified logger @@ -778,12 +734,12 @@ private void UpdateParents(Logger log) string substr = name.Substring(0, i); var key = new LoggerKey(substr); - m_ht.TryGetValue(key, out object? node); + loggers.TryGetValue(key, out object? node); // Create a provision node for a future parent. if (node is null) { - m_ht[key] = new ProvisionNode(log); + loggers[key] = new ProvisionNode(log); } else { @@ -800,7 +756,7 @@ private void UpdateParents(Logger log) } else { - LogLog.Error(declaringType, $"Unexpected object type [{node.GetType()}] in ht.", new LogException()); + LogLog.Error(declaringType, $"Unexpected object type [{node.GetType()}] in loggers.", new LogException()); } } if (i == 0) @@ -864,14 +820,8 @@ private static void UpdateChildren(ProvisionNode pn, Logger log) /// internal void AddLevel(LevelEntry levelEntry) { - if (levelEntry is null) - { - throw new ArgumentNullException(nameof(levelEntry)); - } - if (levelEntry.Name is null) - { - throw new ArgumentNullException("levelEntry.Name"); - } + levelEntry.EnsureNotNull(); + levelEntry.Name.EnsureNotNull(); // Lookup replacement value if (levelEntry.Value == -1) @@ -941,9 +891,7 @@ internal sealed class LevelEntry /// /// string info about this object public override string ToString() - { - return $"LevelEntry(Value={Value}, Name={Name}, DisplayName={DisplayName})"; - } + => $"LevelEntry(Value={Value}, Name={Name}, DisplayName={DisplayName})"; } /// @@ -959,23 +907,12 @@ public override string ToString() /// /// internal void AddProperty(PropertyEntry propertyEntry) - { - if (propertyEntry is null) - { - throw new ArgumentNullException(nameof(propertyEntry)); - } - if (propertyEntry.Key is null) - { - throw new ArgumentNullException("propertyEntry.Key"); - } - - Properties[propertyEntry.Key] = propertyEntry.Value; - } + => Properties[propertyEntry.Key.EnsureNotNull()] = propertyEntry.EnsureNotNull().Value; - private ILoggerFactory m_defaultFactory; + private ILoggerFactory defaultFactory; - private readonly ConcurrentDictionary m_ht = new(LoggerKey.ComparerInstance); - private Logger? m_root; + private readonly ConcurrentDictionary loggers = new(LoggerKey.ComparerInstance); + private Logger? rootLogger; /// /// The fully qualified type of the Hierarchy class. @@ -986,4 +923,4 @@ internal void AddProperty(PropertyEntry propertyEntry) /// private static readonly Type declaringType = typeof(Hierarchy); } -} +} \ No newline at end of file From 998216a50acdae155cd602faba3451ca15732f4f Mon Sep 17 00:00:00 2001 From: Jan Friedrich Date: Fri, 26 Jul 2024 16:01:17 +0200 Subject: [PATCH 3/4] #156 use Interlocked.CompareExchange instead of lokc(this) - shortened xml comments - Hierarchy.EmittedNoAppenderWarning is now internal --- src/log4net/Repository/Hierarchy/Hierarchy.cs | 235 ++++------------- src/log4net/Repository/Hierarchy/Logger.cs | 248 ++++++++---------- .../Repository/Hierarchy/ProvisionNode.cs | 9 +- src/log4net/assembly/bin.xml | 64 ----- src/site/xdoc/release/release-notes.xml | 4 + 5 files changed, 167 insertions(+), 393 deletions(-) delete mode 100644 src/log4net/assembly/bin.xml diff --git a/src/log4net/Repository/Hierarchy/Hierarchy.cs b/src/log4net/Repository/Hierarchy/Hierarchy.cs index 0a55dbeb..8cdae7c9 100644 --- a/src/log4net/Repository/Hierarchy/Hierarchy.cs +++ b/src/log4net/Repository/Hierarchy/Hierarchy.cs @@ -20,7 +20,9 @@ using System; using System.Collections.Concurrent; using System.Collections.Generic; +using System.ComponentModel; using System.Linq; +using System.Threading; using log4net.Appender; using log4net.Core; using log4net.Util; @@ -32,11 +34,6 @@ namespace log4net.Repository.Hierarchy /// /// The in which the has been created. /// The event args that hold the instance that has been created. - /// - /// - /// Delegate used to handle logger creation event notifications. - /// - /// public delegate void LoggerCreationEventHandler(object sender, LoggerCreationEventArgs e); /// @@ -54,28 +51,11 @@ public class LoggerCreationEventArgs : EventArgs /// Constructor /// /// The that has been created. - /// - /// - /// Initializes a new instance of the event argument - /// class,with the specified . - /// - /// - public LoggerCreationEventArgs(Logger log) - { - Logger = log; - } + public LoggerCreationEventArgs(Logger log) => Logger = log; /// /// Gets the that has been created. /// - /// - /// The that has been created. - /// - /// - /// - /// The that has been created. - /// - /// public Logger Logger { get; } } @@ -88,135 +68,91 @@ public LoggerCreationEventArgs(Logger log) /// directly. /// /// - /// This class is specialized in retrieving loggers by name and - /// also maintaining the logger hierarchy. Implements the - /// interface. + /// This class is specialized in retrieving loggers by name and also maintaining the logger + /// hierarchy. Implements the interface. /// /// /// The structure of the logger hierarchy is maintained by the - /// method. The hierarchy is such that children + /// method. The hierarchy is such that children /// link to their parent but parents do not have any references to their /// children. Moreover, loggers can be instantiated in any order, in /// particular descendant before ancestor. /// /// - /// In case a descendant is created before a particular ancestor, - /// then it creates a provision node for the ancestor and adds itself - /// to the provision node. Other descendants of the same ancestor add - /// themselves to the previously created provision node. + /// In case a descendant is created before a particular ancestor, then it creates a provision node + /// for the ancestor and adds itself to the provision node. Other descendants of the same ancestor + /// add themselves to the previously created provision node. /// /// /// Nicko Cadell /// Gert Driesen public class Hierarchy : LoggerRepositorySkeleton, IBasicRepositoryConfigurator, IXmlRepositoryConfigurator { + private readonly ConcurrentDictionary loggers = new(LoggerKey.ComparerInstance); + private ILoggerFactory defaultFactory; + private Logger? rootLogger; + /// - /// Event used to notify that a logger has been created. + /// The fully qualified type of the Hierarchy class. /// /// - /// - /// Event raised when a logger is created. - /// + /// Used by the internal logger to record the type of the log message. /// + private static readonly Type declaringType = typeof(Hierarchy); + + /// + /// Event used to notify that a logger has been created. + /// public event LoggerCreationEventHandler? LoggerCreatedEvent; /// /// Default constructor /// - /// - /// - /// Initializes a new instance of the class. - /// - /// public Hierarchy() : this(new DefaultLoggerFactory()) - { - } + { } /// /// Construct with properties /// /// The properties to pass to this repository. - /// - /// - /// Initializes a new instance of the class. - /// - /// public Hierarchy(PropertiesDictionary properties) : this(properties, new DefaultLoggerFactory()) - { - } + { } /// /// Construct with a logger factory /// /// The factory to use to create new logger instances. - /// - /// - /// Initializes a new instance of the class with - /// the specified . - /// - /// public Hierarchy(ILoggerFactory loggerFactory) : this(new PropertiesDictionary(), loggerFactory) - { - } + { } /// /// Construct with properties and a logger factory /// /// The properties to pass to this repository. /// The factory to use to create new logger instances. - /// - /// - /// Initializes a new instance of the class with - /// the specified . - /// - /// - public Hierarchy(PropertiesDictionary properties, ILoggerFactory loggerFactory) : base(properties) - { - if (loggerFactory is null) - { - throw new ArgumentNullException(nameof(loggerFactory)); - } - - defaultFactory = loggerFactory; - } + public Hierarchy(PropertiesDictionary properties, ILoggerFactory loggerFactory) + : base(properties) => defaultFactory = loggerFactory.EnsureNotNull(); /// /// Has no appender warning been emitted /// /// - /// - /// Flag to indicate if we have already issued a warning - /// about not having an appender warning. - /// + /// Flag to indicate if we have already issued a warning about not having an appender warning. /// - public bool EmittedNoAppenderWarning { get; set; } + internal bool EmittedNoAppenderWarning { get; set; } /// /// Get the root of this hierarchy /// - /// - /// - /// Get the root of this hierarchy. - /// - /// public Logger Root { get { if (rootLogger is null) { - lock (this) - { - if (rootLogger is null) - { - // Create the root logger - Logger root = defaultFactory.CreateLogger(this, null); - root.Hierarchy = this; - - // Store root - rootLogger = root; - } - } + Logger root = defaultFactory.CreateLogger(this, null); + root.Hierarchy = this; + Interlocked.CompareExchange(ref rootLogger, root, null); } return rootLogger; } @@ -244,7 +180,7 @@ public ILoggerFactory LoggerFactory /// /// /// Check if the named logger exists in the hierarchy. If so return - /// its reference, otherwise returns null. + /// its reference, otherwise returns . /// /// public override ILogger? Exists(string name) @@ -267,19 +203,8 @@ public ILoggerFactory LoggerFactory public override ILogger[] GetCurrentLoggers() { // The accumulation in loggers is necessary because not all elements in - // ht are Logger objects as there might be some ProvisionNodes - // as well. - var loggers = new List(this.loggers.Count); - - // Iterate through m_ht values - foreach (object node in this.loggers.Values) - { - if (node is Logger logger) - { - loggers.Add(logger); - } - } - return loggers.ToArray(); + // loggers are Logger objects as there might be some ProvisionNodes as well. + return loggers.Values.OfType().ToArray(); } /// @@ -287,15 +212,9 @@ public override ILogger[] GetCurrentLoggers() /// the default factory. /// /// - /// - /// Return a new logger instance named as the first parameter using - /// the default factory. - /// - /// /// If a logger of that name already exists, then it will be /// returned. Otherwise, a new logger will be instantiated and /// then linked with its existing ancestors as well as children. - /// /// /// The name of the logger to retrieve /// The logger object with the name specified @@ -317,7 +236,7 @@ public override ILogger GetLogger(string name) /// lost. /// /// - /// The Shutdown method is careful to close nested + /// The method is careful to close nested /// appenders before closing regular appenders. This allows /// configurations where a regular appender is attached to a logger /// and again to a nested appender. @@ -355,8 +274,8 @@ public override void Shutdown() /// /// Reset all values contained in this hierarchy instance to their /// default. This removes all appenders from all loggers, sets - /// the level of all non-root loggers to null, - /// sets their additivity flag to true and sets the level + /// the level of all non-root loggers to , + /// sets their additivity flag to and sets the level /// of the root logger to . Moreover, /// message disabling is set its default "off" value. /// @@ -399,7 +318,7 @@ public override void ResetConfiguration() /// using the method. /// /// - /// The logEvent is delivered to the appropriate logger and + /// The is delivered to the appropriate logger and /// that logger is then responsible for logging the event. /// /// @@ -428,7 +347,7 @@ public override void Log(LoggingEvent logEvent) /// public override IAppender[] GetAppenders() { - var appenderList = new List(); + var appenderList = new HashSet(); CollectAppenders(appenderList, Root); @@ -444,23 +363,18 @@ public override IAppender[] GetAppenders() /// Collect the appenders from an . /// The appender may also be a container. /// - private static void CollectAppender(List appenderList, IAppender appender) + private static void CollectAppender(HashSet appenderList, IAppender appender) { - if (!appenderList.Contains(appender)) + if (appenderList.Add(appender) && appender is IAppenderAttachable container) { - appenderList.Add(appender); - - if (appender is IAppenderAttachable container) - { - CollectAppenders(appenderList, container); - } + CollectAppenders(appenderList, container); } } /// /// Collect the appenders from an container /// - private static void CollectAppenders(List appenderList, IAppenderAttachable container) + private static void CollectAppenders(HashSet appenderList, IAppenderAttachable container) { foreach (IAppender appender in container.Appenders) { @@ -555,21 +469,11 @@ protected void XmlRepositoryConfigure(System.Xml.XmlElement element) /// /// The level to check against. /// - /// true if the repository is disabled for the level argument, false otherwise. + /// if the repository is disabled for the level argument, otherwise. /// /// - /// - /// If this hierarchy has not been configured then this method will - /// always return true. - /// - /// - /// This method will return true if this repository is - /// disabled for level object passed as parameter and - /// false otherwise. - /// - /// + /// If this hierarchy has not been configured then this method will always return . /// See also the property. - /// /// public bool IsDisabled(Level level) { @@ -591,10 +495,10 @@ public bool IsDisabled(Level level) /// logger hierarchy. /// /// - /// You should really know what you are doing before - /// invoking this method. + /// You should really know what you are doing before invoking this method. /// /// + [EditorBrowsable(EditorBrowsableState.Never)] public void Clear() => loggers.Clear(); /// @@ -716,8 +620,7 @@ protected virtual void OnLoggerCreationEvent(Logger logger) /// /// The entry is of type ProvisionNode for this potential parent. /// - /// We add to the list of children for this - /// potential parent. + /// We add to the list of children for this potential parent. /// /// /// @@ -814,9 +717,7 @@ private static void UpdateChildren(ProvisionNode pn, Logger log) /// /// the level values /// - /// /// Supports setting levels via the configuration file. - /// /// internal void AddLevel(LevelEntry levelEntry) { @@ -842,48 +743,25 @@ internal void AddLevel(LevelEntry levelEntry) /// /// A class to hold the value, name and display name for a level /// - /// - /// - /// A class to hold the value, name and display name for a level - /// - /// internal sealed class LevelEntry { /// /// Value of the level /// /// - /// /// If the value is not set (defaults to -1) the value will be looked /// up for the current level with the same name. - /// /// public int Value { get; set; } = -1; /// /// Name of the level /// - /// - /// The name of the level - /// - /// - /// - /// The name of the level. - /// - /// public string? Name { get; set; } /// /// Display name for the level /// - /// - /// The display name of the level - /// - /// - /// - /// The display name of the level. - /// - /// public string? DisplayName { get; set; } /// @@ -899,28 +777,9 @@ public override string ToString() /// /// the property value /// - /// - /// Set a Property using the values in the argument. - /// - /// /// Supports setting property values via the configuration file. - /// /// internal void AddProperty(PropertyEntry propertyEntry) => Properties[propertyEntry.Key.EnsureNotNull()] = propertyEntry.EnsureNotNull().Value; - - private ILoggerFactory defaultFactory; - - private readonly ConcurrentDictionary loggers = new(LoggerKey.ComparerInstance); - private Logger? rootLogger; - - /// - /// The fully qualified type of the Hierarchy class. - /// - /// - /// Used by the internal logger to record the Type of the - /// log message. - /// - private static readonly Type declaringType = typeof(Hierarchy); } } \ No newline at end of file diff --git a/src/log4net/Repository/Hierarchy/Logger.cs b/src/log4net/Repository/Hierarchy/Logger.cs index c946813d..baa41b27 100644 --- a/src/log4net/Repository/Hierarchy/Logger.cs +++ b/src/log4net/Repository/Hierarchy/Logger.cs @@ -52,6 +52,36 @@ namespace log4net.Repository.Hierarchy /// Douglas de la Torre public abstract class Logger : IAppenderAttachable, ILogger { + /// + /// The fully qualified type of the Logger class. + /// + private static readonly Type declaringType = typeof(Logger); + + /// + /// The parent of this logger. + /// + /// + /// + /// All loggers have at least one ancestor which is the root logger. + /// + /// + private Logger? parent; + + /// + /// Loggers need to know what Hierarchy they are in. + /// + private Hierarchy? hierarchy; + + /// + /// Helper implementation of the interface + /// + private AppenderAttachedImpl? appenderAttachedImpl; + + /// + /// Lock to protect AppenderAttachedImpl variable m_appenderAttachedImpl + /// + private readonly ReaderWriterLock appenderLock = new(); + /// /// This constructor created a new instance and /// sets its name. @@ -68,10 +98,8 @@ public abstract class Logger : IAppenderAttachable, ILogger /// logger creator. /// /// - protected Logger(string name) - { - Name = string.Intern(name); - } + protected Logger(string name) + => Name = string.Intern(name); /// /// Gets or sets the parent logger in the hierarchy. @@ -87,29 +115,29 @@ protected Logger(string name) /// public virtual Logger? Parent { - get => m_parent; - set => m_parent = value; + get => parent; + set => parent = value; } /// /// Gets or sets a value indicating if child loggers inherit their parent's appenders. /// /// - /// true if child loggers inherit their parent's appenders. + /// if child loggers inherit their parent's appenders. /// /// /// - /// Additivity is set to true by default, that is children inherit + /// Additivity is set to by default, that is children inherit /// the appenders of their ancestors by default. If this variable is - /// set to false then the appenders found in the + /// set to then the appenders found in the /// ancestors of this logger are not used. However, the children /// of this logger will inherit its appenders, unless the children - /// have their additivity flag set to false too. See + /// have their additivity flag set to too. See /// the user manual for more details. /// /// public virtual bool Additivity { get; set; } = true; - + /// /// Gets the effective level for this logger. /// @@ -125,11 +153,11 @@ public virtual Logger? Parent /// public virtual Level EffectiveLevel { - get + get { - for (Logger? c = this; c is not null; c = c.m_parent) + for (Logger? c = this; c is not null; c = c.parent) { - if (c.Level is Level level) + if (c.Level is Level level) { return level; } @@ -139,26 +167,17 @@ public virtual Level EffectiveLevel } /// - /// Gets or sets the where this - /// Logger instance is attached to. + /// Gets or sets the where this instance is attached to. /// - /// - /// - /// This logger must be attached to a single . - /// - /// public virtual Hierarchy? Hierarchy { - get => m_hierarchy; - set => m_hierarchy = value; + get => hierarchy; + set => hierarchy = value; } /// - /// Gets or sets the assigned , if any, for this Logger. + /// Gets or sets the assigned for this Logger. /// - /// - /// The of this logger. - /// public virtual Level? Level { get; set; } /// @@ -172,22 +191,19 @@ public virtual Hierarchy? Hierarchy /// appenders, then it won't be added again. /// /// - public virtual void AddAppender(IAppender newAppender) + public virtual void AddAppender(IAppender newAppender) { - if (newAppender is null) - { - throw new ArgumentNullException(nameof(newAppender)); - } + newAppender.EnsureNotNull(); - m_appenderLock.AcquireWriterLock(); + appenderLock.AcquireWriterLock(); try { - m_appenderAttachedImpl ??= new AppenderAttachedImpl(); - m_appenderAttachedImpl.AddAppender(newAppender); + appenderAttachedImpl ??= new AppenderAttachedImpl(); + appenderAttachedImpl.AddAppender(newAppender); } finally { - m_appenderLock.ReleaseWriterLock(); + appenderLock.ReleaseWriterLock(); } } @@ -199,49 +215,46 @@ public virtual void AddAppender(IAppender newAppender) /// A collection of the appenders in this logger. If no appenders /// can be found, then a is returned. /// - public virtual AppenderCollection Appenders + public virtual AppenderCollection Appenders { get { - m_appenderLock.AcquireReaderLock(); + appenderLock.AcquireReaderLock(); try { - if (m_appenderAttachedImpl is null) + if (appenderAttachedImpl is null) { return AppenderCollection.EmptyCollection; } - else - { - return m_appenderAttachedImpl.Appenders; - } + return appenderAttachedImpl.Appenders; } finally { - m_appenderLock.ReleaseReaderLock(); + appenderLock.ReleaseReaderLock(); } } } /// - /// Look for the appender named as name + /// Look for the appender named as /// /// The name of the appender to lookup - /// The appender with the name specified, or null. - public virtual IAppender? GetAppender(string? name) + /// The appender with the name specified, or . + public virtual IAppender? GetAppender(string? name) { - m_appenderLock.AcquireReaderLock(); + appenderLock.AcquireReaderLock(); try { - if (m_appenderAttachedImpl is null || name is null) + if (appenderAttachedImpl is null || name is null) { return null; } - return m_appenderAttachedImpl.GetAppender(name); + return appenderAttachedImpl.GetAppender(name); } finally { - m_appenderLock.ReleaseReaderLock(); + appenderLock.ReleaseReaderLock(); } } @@ -253,20 +266,20 @@ public virtual AppenderCollection Appenders /// This is useful when re-reading configuration information. /// /// - public virtual void RemoveAllAppenders() + public virtual void RemoveAllAppenders() { - m_appenderLock.AcquireWriterLock(); + appenderLock.AcquireWriterLock(); try { - if (m_appenderAttachedImpl is not null) + if (appenderAttachedImpl is not null) { - m_appenderAttachedImpl.RemoveAllAppenders(); - m_appenderAttachedImpl = null; + appenderAttachedImpl.RemoveAllAppenders(); + appenderAttachedImpl = null; } } finally { - m_appenderLock.ReleaseWriterLock(); + appenderLock.ReleaseWriterLock(); } } @@ -282,19 +295,19 @@ public virtual void RemoveAllAppenders() /// on the appender removed. /// /// - public virtual IAppender? RemoveAppender(IAppender? appender) + public virtual IAppender? RemoveAppender(IAppender? appender) { - m_appenderLock.AcquireWriterLock(); + appenderLock.AcquireWriterLock(); try { - if (appender is not null && m_appenderAttachedImpl is not null) + if (appender is not null && appenderAttachedImpl is not null) { - return m_appenderAttachedImpl.RemoveAppender(appender); + return appenderAttachedImpl.RemoveAppender(appender); } } finally { - m_appenderLock.ReleaseWriterLock(); + appenderLock.ReleaseWriterLock(); } return null; } @@ -311,23 +324,23 @@ public virtual void RemoveAllAppenders() /// on the appender removed. /// /// - public virtual IAppender? RemoveAppender(string? name) + public virtual IAppender? RemoveAppender(string? name) { - m_appenderLock.AcquireWriterLock(); + appenderLock.AcquireWriterLock(); try { - if (name is not null && m_appenderAttachedImpl is not null) + if (name is not null && appenderAttachedImpl is not null) { - return m_appenderAttachedImpl.RemoveAppender(name); + return appenderAttachedImpl.RemoveAppender(name); } } finally { - m_appenderLock.ReleaseWriterLock(); + appenderLock.ReleaseWriterLock(); } return null; } - + /// /// Gets the logger name. /// @@ -350,7 +363,7 @@ public virtual void RemoveAllAppenders() /// This method must not throw any exception to the caller. /// /// - public virtual void Log(Type? callerStackBoundaryDeclaringType, Level? level, object? message, Exception? exception) + public virtual void Log(Type? callerStackBoundaryDeclaringType, Level? level, object? message, Exception? exception) { try { @@ -401,7 +414,8 @@ public virtual void Log(LoggingEvent? logEvent) /// /// The level to check. /// - /// true if this logger is enabled for level, otherwise false. + /// if this logger is enabled for , + /// otherwise . /// /// /// @@ -414,7 +428,7 @@ public virtual bool IsEnabledFor(Level? level) { if (level is not null) { - if (m_hierarchy is not null && m_hierarchy.IsDisabled(level)) + if (hierarchy is not null && hierarchy.IsDisabled(level)) { return false; } @@ -430,9 +444,9 @@ public virtual bool IsEnabledFor(Level? level) /// /// Gets the where this - /// Logger instance is attached to. + /// instance is attached to. /// - public ILoggerRepository? Repository => m_hierarchy; + public ILoggerRepository? Repository => hierarchy; /// /// Deliver the to the attached appenders. @@ -440,9 +454,8 @@ public virtual bool IsEnabledFor(Level? level) /// The event to log. /// /// - /// Call the appenders in the hierarchy starting at - /// this. If no appenders could be found, emit a - /// warning. + /// Call the appenders in the hierarchy starting at . + /// If no appenders could be found, emit a warning. /// /// /// This method calls all the appenders inherited from the @@ -450,40 +463,37 @@ public virtual bool IsEnabledFor(Level? level) /// to log the particular log request. /// /// - protected virtual void CallAppenders(LoggingEvent loggingEvent) + protected virtual void CallAppenders(LoggingEvent loggingEvent) { - if (loggingEvent is null) - { - throw new ArgumentNullException(nameof(loggingEvent)); - } + loggingEvent.EnsureNotNull(); int writes = 0; - for(Logger? c = this; c is not null; c = c.m_parent) + for (Logger? c = this; c is not null; c = c.parent) { - if (c.m_appenderAttachedImpl is not null) + if (c.appenderAttachedImpl is not null) { // Protected against simultaneous call to addAppender, removeAppender,... - c.m_appenderLock.AcquireReaderLock(); + c.appenderLock.AcquireReaderLock(); try { - if (c.m_appenderAttachedImpl is not null) + if (c.appenderAttachedImpl is not null) { - writes += c.m_appenderAttachedImpl.AppendLoopOnAppenders(loggingEvent); + writes += c.appenderAttachedImpl.AppendLoopOnAppenders(loggingEvent); } } finally { - c.m_appenderLock.ReleaseReaderLock(); + c.appenderLock.ReleaseReaderLock(); } } - if (!c.Additivity) + if (!c.Additivity) { break; } } - + // No appenders in hierarchy, warn user only once. // // Note that by including the AppDomain values for the currently running @@ -493,9 +503,9 @@ protected virtual void CallAppenders(LoggingEvent loggingEvent) // or impossible to determine which .config file is missing appender // definitions. // - if (m_hierarchy is not null && !m_hierarchy.EmittedNoAppenderWarning && writes == 0) + if (hierarchy is not null && !hierarchy.EmittedNoAppenderWarning && writes == 0) { - m_hierarchy.EmittedNoAppenderWarning = true; + hierarchy.EmittedNoAppenderWarning = true; LogLog.Debug(declaringType, $"No appenders could be found for logger [{Name}] repository [{Repository?.Name}]"); LogLog.Debug(declaringType, "Please initialize the log4net system properly."); try @@ -505,7 +515,7 @@ protected virtual void CallAppenders(LoggingEvent loggingEvent) LogLog.Debug(declaringType, " FriendlyName : " + AppDomain.CurrentDomain.FriendlyName); LogLog.Debug(declaringType, " DynamicDirectory: " + AppDomain.CurrentDomain.DynamicDirectory); } - catch(System.Security.SecurityException) + catch (System.Security.SecurityException) { // Insufficient permissions to display info from the AppDomain } @@ -520,15 +530,15 @@ protected virtual void CallAppenders(LoggingEvent loggingEvent) /// Used to ensure that the appenders are correctly shutdown. /// /// - public virtual void CloseNestedAppenders() + public virtual void CloseNestedAppenders() { - m_appenderLock.AcquireWriterLock(); + appenderLock.AcquireWriterLock(); try { - if (m_appenderAttachedImpl is not null) + if (appenderAttachedImpl is not null) { - AppenderCollection appenders = m_appenderAttachedImpl.Appenders; - foreach(IAppender appender in appenders) + AppenderCollection appenders = appenderAttachedImpl.Appenders; + foreach (IAppender appender in appenders) { if (appender is IAppenderAttachable) { @@ -539,7 +549,7 @@ public virtual void CloseNestedAppenders() } finally { - m_appenderLock.ReleaseWriterLock(); + appenderLock.ReleaseWriterLock(); } } @@ -555,7 +565,7 @@ public virtual void CloseNestedAppenders() /// the . /// /// - public virtual void Log(Level level, object? message, Exception? exception) + public virtual void Log(Level level, object? message, Exception? exception) { if (IsEnabledFor(level)) { @@ -577,9 +587,11 @@ public virtual void Log(Level level, object? message, Exception? exception) /// appenders. /// /// - protected virtual void ForcedLog(Type callerStackBoundaryDeclaringType, Level? level, object? message, Exception? exception) + protected virtual void ForcedLog(Type callerStackBoundaryDeclaringType, Level? level, + object? message, Exception? exception) { - CallAppenders(new LoggingEvent(callerStackBoundaryDeclaringType, Hierarchy, Name, level, message, exception)); + CallAppenders(new LoggingEvent(callerStackBoundaryDeclaringType, Hierarchy, Name, level, + message, exception)); } /// @@ -591,7 +603,7 @@ protected virtual void ForcedLog(Type callerStackBoundaryDeclaringType, Level? l /// Delivers the logging event to the attached appenders. /// /// - protected virtual void ForcedLog(LoggingEvent logEvent) + protected virtual void ForcedLog(LoggingEvent logEvent) { // The logging event may not have been created by this logger // the Repository may not be correctly set on the event. This @@ -600,35 +612,5 @@ protected virtual void ForcedLog(LoggingEvent logEvent) CallAppenders(logEvent); } - - /// - /// The fully qualified type of the Logger class. - /// - private static readonly Type declaringType = typeof(Logger); - - /// - /// The parent of this logger. - /// - /// - /// - /// All loggers have at least one ancestor which is the root logger. - /// - /// - private Logger? m_parent; - - /// - /// Loggers need to know what Hierarchy they are in. - /// - private Hierarchy? m_hierarchy; - - /// - /// Helper implementation of the interface - /// - private AppenderAttachedImpl? m_appenderAttachedImpl; - - /// - /// Lock to protect AppenderAttachedImpl variable m_appenderAttachedImpl - /// - private readonly ReaderWriterLock m_appenderLock = new(); } -} +} \ No newline at end of file diff --git a/src/log4net/Repository/Hierarchy/ProvisionNode.cs b/src/log4net/Repository/Hierarchy/ProvisionNode.cs index 47628519..47b8c59e 100644 --- a/src/log4net/Repository/Hierarchy/ProvisionNode.cs +++ b/src/log4net/Repository/Hierarchy/ProvisionNode.cs @@ -31,8 +31,7 @@ namespace log4net.Repository.Hierarchy /// for that node. /// /// - /// A provision node holds a list of child loggers on behalf of - /// a logger that does not exist. + /// A provision node holds a list of child loggers on behalf of a logger that does not exist. /// /// /// Nicko Cadell @@ -43,12 +42,6 @@ internal sealed class ProvisionNode : List /// Create a new provision node with child node /// /// A child logger to add to this node. - /// - /// - /// Initializes a new instance of the class - /// with the specified child logger. - /// - /// internal ProvisionNode(Logger log) { Add(log); diff --git a/src/log4net/assembly/bin.xml b/src/log4net/assembly/bin.xml deleted file mode 100644 index c52bf0de..00000000 --- a/src/log4net/assembly/bin.xml +++ /dev/null @@ -1,64 +0,0 @@ - - - bin - - zip - tar.gz - - apache-log4j-${project.version} - true - - - - *.txt - *.sample - *.xml - INSTALL - KEYS - LICENSE - NOTICE - contribs/** - examples/** - src/assembly/** - src/changes/** - src/main/** - src/ntdll/** - src/performance/** - src/site/** - tests/README - tests/*.xml - tests/*.sample - tests/*.bat - tests/input/** - tests/resources/** - tests/src/** - tests/witness/** - - - - - - target/log4j-${project.version}.jar - - - target/NTEventLogAppender.dll - 0755 - - - diff --git a/src/site/xdoc/release/release-notes.xml b/src/site/xdoc/release/release-notes.xml index 274ecc1c..08eb9b4f 100644 --- a/src/site/xdoc/release/release-notes.xml +++ b/src/site/xdoc/release/release-notes.xml @@ -147,6 +147,10 @@ limitations under the License. log4net.Appender.MemoryAppender.m_eventsList (protected field) is now List<LoggingEvent> (instead of System.Collections.ArrayList before) The reason is an optimization as part of Add support for nullable annotations +
  • + log4net.Repository.Hierarchy.Hierarchy.EmittedNoAppenderWarning is now internal. + See fixed Regression when creating nested loggers in reverse order +


    • From 5f94dccab4b9d30737ddc939219119a3c5069037 Mon Sep 17 00:00:00 2001 From: Jan Friedrich Date: Fri, 26 Jul 2024 16:26:32 +0200 Subject: [PATCH 4/4] #158 added removal of NetSendAppender to release notes --- src/site/xdoc/release/release-notes.xml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/site/xdoc/release/release-notes.xml b/src/site/xdoc/release/release-notes.xml index 08eb9b4f..ca314526 100644 --- a/src/site/xdoc/release/release-notes.xml +++ b/src/site/xdoc/release/release-notes.xml @@ -128,10 +128,14 @@ limitations under the License.

    - Remove RemotingAppender