RE: Better support for logging from an Appender

2016-10-26 Thread Dominik Psenner
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

2016-10-26 Thread Joe
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

2016-10-22 Thread Stefan Bodewig
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