RE: Better support for logging from an Appender
Appenders that log is immensely hard to implement if not separated entirely from the normal logging. Therefore there is the loglog class. It could be a feature to allow the configuration of loglog appenders under the precondition that those appenders do not append events back into the logging framework, even if events travel over remote sinks. On 26 Oct 2016 8:40 p.m., "Joe" wrote: > Thanks for the response. > > After more thought and some tests, this idea is a non-starter. > > 1. Hierarchy.Logger.CallAppenders acquires a ReaderWriterLockSlim instance > that doesn't allow recursion (the default LockRecursionPolicy.NoRecursion). > Thus if a synchronous appender attempts to log, it will fail with a > LogRecursionException. > > I'm not sure if this behaviour is intentional. Because when compiled for > .NET 1.x it uses a ReaderWriterLock which *does* allow recursion. > > Any thoughts on this? Should the log4net.Util.ReaderWriterLock class be > instantiating its ReaderWriterLockSlim with LockRecursionPolicy. > SupportsRecursion? > > Another point about ReaderWriterLockSlim is that it is IDisposable, which > means that strictly speaking classes that own an instance (e.g. > Hierarchy.Logger) should be IDisposable, and should be disposed when no > longer used. I doubt if this is a big deal in practice as most log4net > objects will live until the AppDomain is unloaded. > > 2. Even in the asynchronous case, there is a risk of infinite recursion. > Each Appender can ignore logging events generated by its own logger, but > consider the following, assuming appenders want verbose logging of every > LoggingEvent they see: > > - Appender A logs to logger A > - Appender B logs to logger B > - log4net is configured to route logging events to both appenders > > Every logging event received by A is logged which triggers a logging event > to B, which logs it and triggers a log event to A, ... > > So back to the drawing board. > > My main reason for wanting this is for the case of an asynchronous > appender, which will typically be forwarding events to a remote sink, the > connection to which may be unreliable. > In this case, it's desirable for errors to be logged in log4net even in a > production environment where log4net.internal.debug is false. Probably > using a "once-only" logging technique which only logs the error the first > time it occurs. > > > -----Original Message- > From: Stefan Bodewig [mailto:bode...@apache.org] > Sent: 22 October 2016 22:18 > To: Log4NET Dev > Subject: Re: Better support for logging from an Appender > > On 2016-10-21, Joe wrote: > > > From within DoAppend, there is a flag m_recursiveGuard which should > > prevent re-entrancy problems from a synchronous appender's Append > > method. For an asynchronous appender there would need to be an > > additional check since it will want to log from a different thread. > > We could store some kind of flag that acts like the recursive guard inside > the (logical) thread context and then check for its existence inside the > LoggingEvent's properties. > > > The following commit will give you an idea of what I'm thinking of: > > > https://github.com/apache/log4net/commit/9dc3ed9ae079d9abeacdf86c380b1 > > 7efff4b3a97?w=1 > > > Is this something we should support? > > Your idea works for the case where you know you are logging from inside an > appender. There is a different way an appender might receive messages that > originate from itself when the appender uses a library that again uses > log4net for logging. This wouldn't get caught by your approach but a > property on the event would work. > > This is just an idea, nothing I've actually tried. > > > BTW if you look at the above commit without the ?w=1 suffix, you'll > > see whitespace differences in the Flush method. I think the > > .gitattributes file needs to add rules for converting spaces to tabs. > > Indentation inside the code base is a mess and I'm sure I've contributed > to it by not using visual studio. Using .gitattributes could help if git > was our primary SCM. For people using svn it wouldn't have any effect. > > Maybe we need somebody to configure VS with a "proper configuration" > that we agree upon and re-indent the whole codebase once. > > Stefan >
RE: Better support for logging from an Appender
Thanks for the response. After more thought and some tests, this idea is a non-starter. 1. Hierarchy.Logger.CallAppenders acquires a ReaderWriterLockSlim instance that doesn't allow recursion (the default LockRecursionPolicy.NoRecursion). Thus if a synchronous appender attempts to log, it will fail with a LogRecursionException. I'm not sure if this behaviour is intentional. Because when compiled for .NET 1.x it uses a ReaderWriterLock which *does* allow recursion. Any thoughts on this? Should the log4net.Util.ReaderWriterLock class be instantiating its ReaderWriterLockSlim with LockRecursionPolicy.SupportsRecursion? Another point about ReaderWriterLockSlim is that it is IDisposable, which means that strictly speaking classes that own an instance (e.g. Hierarchy.Logger) should be IDisposable, and should be disposed when no longer used. I doubt if this is a big deal in practice as most log4net objects will live until the AppDomain is unloaded. 2. Even in the asynchronous case, there is a risk of infinite recursion. Each Appender can ignore logging events generated by its own logger, but consider the following, assuming appenders want verbose logging of every LoggingEvent they see: - Appender A logs to logger A - Appender B logs to logger B - log4net is configured to route logging events to both appenders Every logging event received by A is logged which triggers a logging event to B, which logs it and triggers a log event to A, ... So back to the drawing board. My main reason for wanting this is for the case of an asynchronous appender, which will typically be forwarding events to a remote sink, the connection to which may be unreliable. In this case, it's desirable for errors to be logged in log4net even in a production environment where log4net.internal.debug is false. Probably using a "once-only" logging technique which only logs the error the first time it occurs. -Original Message- From: Stefan Bodewig [mailto:bode...@apache.org] Sent: 22 October 2016 22:18 To: Log4NET Dev Subject: Re: Better support for logging from an Appender On 2016-10-21, Joe wrote: > From within DoAppend, there is a flag m_recursiveGuard which should > prevent re-entrancy problems from a synchronous appender's Append > method. For an asynchronous appender there would need to be an > additional check since it will want to log from a different thread. We could store some kind of flag that acts like the recursive guard inside the (logical) thread context and then check for its existence inside the LoggingEvent's properties. > The following commit will give you an idea of what I'm thinking of: > https://github.com/apache/log4net/commit/9dc3ed9ae079d9abeacdf86c380b1 > 7efff4b3a97?w=1 > Is this something we should support? Your idea works for the case where you know you are logging from inside an appender. There is a different way an appender might receive messages that originate from itself when the appender uses a library that again uses log4net for logging. This wouldn't get caught by your approach but a property on the event would work. This is just an idea, nothing I've actually tried. > BTW if you look at the above commit without the ?w=1 suffix, you'll > see whitespace differences in the Flush method. I think the > .gitattributes file needs to add rules for converting spaces to tabs. Indentation inside the code base is a mess and I'm sure I've contributed to it by not using visual studio. Using .gitattributes could help if git was our primary SCM. For people using svn it wouldn't have any effect. Maybe we need somebody to configure VS with a "proper configuration" that we agree upon and re-indent the whole codebase once. Stefan
Re: Better support for logging from an Appender
On 2016-10-21, Joe wrote: > From within DoAppend, there is a flag m_recursiveGuard which should > prevent re-entrancy problems from a synchronous appender's Append > method. For an asynchronous appender there would need to be an > additional check since it will want to log from a different thread. We could store some kind of flag that acts like the recursive guard inside the (logical) thread context and then check for its existence inside the LoggingEvent's properties. > The following commit will give you an idea of what I'm thinking of: > https://github.com/apache/log4net/commit/9dc3ed9ae079d9abeacdf86c380b17efff4b3a97?w=1 > Is this something we should support? Your idea works for the case where you know you are logging from inside an appender. There is a different way an appender might receive messages that originate from itself when the appender uses a library that again uses log4net for logging. This wouldn't get caught by your approach but a property on the event would work. This is just an idea, nothing I've actually tried. > BTW if you look at the above commit without the ?w=1 suffix, you'll > see whitespace differences in the Flush method. I think the > .gitattributes file needs to add rules for converting spaces to tabs. Indentation inside the code base is a mess and I'm sure I've contributed to it by not using visual studio. Using .gitattributes could help if git was our primary SCM. For people using svn it wouldn't have any effect. Maybe we need somebody to configure VS with a "proper configuration" that we agree upon and re-indent the whole codebase once. Stefan
