Log4net
  1. Log4net
  2. LOG4NET-283

OnlyOnceErrorHandler is not subclass-friendly

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.2.10
    • Fix Version/s: 1.2.12
    • Component/s: Core
    • Labels:
      None
    • Environment:
      Windows 7, IIS 7, .Net 3.5, C#

      Description

      If you ever try to use the ErrorHandler attribute with a custom error handler, it won't write out the messages. For example:

      <appender name="Database_Appender" type="log4net.Appender.AdoNetAppender">
      <errorHandler type="CHO.Next.Global.LogWriterErrorHandler, CHO.Next.Global" />

      Where the class is defined as:

      public class LogWriterErrorHandler : OnlyOnceErrorHandler
      {

      public new void Error(string message)

      { Error(message, null); }

      public new void Error(string message, Exception e)

      { Error(message, e, ErrorCode.GenericFailure); }

      public new void Error(string message, Exception e, ErrorCode errorCode)

      { // write to a file here }

      }

      This was specified as a fix on a few posts like this http://www.mail-archive.com/log4net-user@logging.apache.org/msg04378.html and there hasn't been anything to correct it.

      The reason this won't work is that Error is not virtual. Although the LogWriterErrorHandler is instantiated and the constructor is called, when the appender makes a call to this.ErrorHandler.Error, it calls the base class of OnlyOnceErrorHandler and not LogWriterErrorHandler.

      I would recommend you make the Error methods in AppenderSkeleton virtual so that they can be overriden. Otherwise, what is the value of even having the ErrorHandler attribute available on the appender?

        Activity

        Hide
        Stefan Bodewig added a comment -

        svn revision 1211480 introduces a virtual FirstError method.

        Show
        Stefan Bodewig added a comment - svn revision 1211480 introduces a virtual FirstError method.
        Hide
        Ron Grabowski added a comment -

        I could see sending an email when the appender goes off-line being handy: SendEmailOnlyOnceErrorHandler

        Show
        Ron Grabowski added a comment - I could see sending an email when the appender goes off-line being handy: SendEmailOnlyOnceErrorHandler
        Hide
        Stefan Bodewig added a comment -

        I don't think you want the error methods in AppenderSkeleton to be virtual but rather the ones in OnlyOnceErrorHandler.

        Your custom IErrorHandler should work perfectly well if you implement IErrorHandler directly without inheriting from OnlyOnceErrorHandler.

        Why would you want to extend OnlyOnceErrorHandler at all?

        If you want to retain the behavior of not logging more than once per appender you'd need access to the private m_firstTime field. It would be more logical to move the code that resides inside the if-block in the three-arg Error method in OnlyOnceErrorHandler into a new virtual method (FirstError or something like this) to make things subclass friendly.

        I don't see any benefit in extending OnlyOnceErrorHandler if you don't want to retain the "log once per appender" logic.

        I'll change the title and severity as well as the target release of this ticket accordingly.

        Show
        Stefan Bodewig added a comment - I don't think you want the error methods in AppenderSkeleton to be virtual but rather the ones in OnlyOnceErrorHandler. Your custom IErrorHandler should work perfectly well if you implement IErrorHandler directly without inheriting from OnlyOnceErrorHandler. Why would you want to extend OnlyOnceErrorHandler at all? If you want to retain the behavior of not logging more than once per appender you'd need access to the private m_firstTime field. It would be more logical to move the code that resides inside the if-block in the three-arg Error method in OnlyOnceErrorHandler into a new virtual method (FirstError or something like this) to make things subclass friendly. I don't see any benefit in extending OnlyOnceErrorHandler if you don't want to retain the "log once per appender" logic. I'll change the title and severity as well as the target release of this ticket accordingly.

          People

          • Assignee:
            Unassigned
            Reporter:
            Randar Puust
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development