Log4net
  1. Log4net
  2. LOG4NET-394

Lambda-based ILog-Extensions should catch errors

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.2.12
    • Fix Version/s: 1.2.13
    • Component/s: Core
    • Labels:
      None

      Description

      The new lambda syntax introduced with LOG4NET-290 allow wrapping log related code in a lambda to be executed only if necessary. In the log4net spirit of being a reliable logging system (see http://logging.apache.org/log4net/release/faq.html) also the log related code contained in the lambda should not block application by throwing exception and so the lambda execution shluld be wrapped in a try...catch

        Activity

        Hide
        Dominik Psenner added a comment - - edited

        What do you expect to happen if you write this somewhere in your application:

        ILog logger;
        [...]
        logger.DebugExt(null);

        or

        ILog logger;
        [...]
        logger.DebugExt(() => 123/0);

        As I see it, log4net should not silently ignore erroneous code which is part of a lambda expression since log4net is not responsible of the lambda expression just alike log4net is not responsible if someone writes this:

        ILog logger;
        [...]
        logger.Debug(123/0);

        The lambda expressions passed into the logger are not part of the framework and ignoring exceptions there may lead to one of those special cases where noone ever notices that nothing is logged because someone wrote something silly in a lambda expression.

        Show
        Dominik Psenner added a comment - - edited What do you expect to happen if you write this somewhere in your application: ILog logger; [...] logger.DebugExt(null); or ILog logger; [...] logger.DebugExt(() => 123/0); As I see it, log4net should not silently ignore erroneous code which is part of a lambda expression since log4net is not responsible of the lambda expression just alike log4net is not responsible if someone writes this: ILog logger; [...] logger.Debug(123/0); The lambda expressions passed into the logger are not part of the framework and ignoring exceptions there may lead to one of those special cases where noone ever notices that nothing is logged because someone wrote something silly in a lambda expression.
        Hide
        Gian Marco Gherardi added a comment - - edited

        Every exception raised during log processing is silenced. This code, for example, does not throw exceprtion:

        class Program
        {
            static void Main(string[] args)
            {
                BasicConfigurator.Configure();
                LogManager.GetLogger("Logger").FatalFormat("{0}", new Message());
            }
        }
        
        class Message
        {
            public override string ToString()
            {
                throw new ApplicationException();
            }
        }
        

        IMO catching application raised by the lambda is a more coherent behavior, consistent with the rest of the library.

        Show
        Gian Marco Gherardi added a comment - - edited Every exception raised during log processing is silenced. This code, for example, does not throw exceprtion: class Program { static void Main(string[] args) { BasicConfigurator.Configure(); LogManager.GetLogger( "Logger" ).FatalFormat( "{0}" , new Message()); } } class Message { public override string ToString() { throw new ApplicationException(); } } IMO catching application raised by the lambda is a more coherent behavior, consistent with the rest of the library.
        Hide
        Dominik Psenner added a comment - - edited

        Agreed. I'll look into patching this so that it will be in 1.2.13.

        Show
        Dominik Psenner added a comment - - edited Agreed. I'll look into patching this so that it will be in 1.2.13.
        Hide
        Dominik Psenner added a comment -

        Fixed as of revision: 1529109

        Show
        Dominik Psenner added a comment - Fixed as of revision: 1529109

          People

          • Assignee:
            Dominik Psenner
            Reporter:
            Gian Marco Gherardi
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development