Uploaded image for project: 'Log4net'
  1. Log4net
  2. LOG4NET-447

MemoryAppender class is not thread safe

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.2.13
    • Fix Version/s: 1.2.14, 1.3.0
    • Component/s: Appenders
    • Labels:
      None
    • Environment:
      .NET Framework 4.0 (Full Profile)

      Description

      The memory appender cannot be used safely in an application that uses more than one thread to interact with logging.

      When using this code to retrieve and then clear events logged in the memory appender:
      var events = memoryAppender.GetEvents();
      memoryAppender.Clear();

      The executing thread may be preempted between these two calls and additional logging events logged. These events are then cleared when the thread resumes execution - and are never seen/processed.

      I expect to be be able to retrieve and clear the list of current events in one call that guarantees that no events are ever lost.

      1. MemoryAppender_ThreadSafety.diff
        2 kB
        Gregory Babski
      2. MemoryAppenderTest.cs
        2 kB
        Gregory Babski

        Activity

        Hide
        babskig Gregory Babski added a comment -

        This is an nUnit test case to reproduce the issue.

        Show
        babskig Gregory Babski added a comment - This is an nUnit test case to reproduce the issue.
        Hide
        babskig Gregory Babski added a comment -

        Added patch file that fixes the issue.

        The patch deprecates the Get and Clear methods. To retrieve and clear the event list use PopAllEvents. This performs the same logical operations internally, but does so while holding a lock on the event list's syncroot object.

        Show
        babskig Gregory Babski added a comment - Added patch file that fixes the issue. The patch deprecates the Get and Clear methods. To retrieve and clear the event list use PopAllEvents. This performs the same logical operations internally, but does so while holding a lock on the event list's syncroot object.
        Hide
        nachbarslumpi Dominik Psenner added a comment -

        Hi there,

        thanks for sharing your contribution. I've looked into the diff and it is straight forward and looks good. Two things that I would like to be discussed/changed before I will apply the patch:

        Marking the other methods as obsolete is probably not a wise decision since people might still want to get events without clearing them or clear events without wasting memory to get them. In general, the memory appender was not invented to be a threadsafe FIFO queue. This said, is there a special reason why you did not create a new class that extends the MemoryAppender and adds your functionality?

        Cheers

        Show
        nachbarslumpi Dominik Psenner added a comment - Hi there, thanks for sharing your contribution. I've looked into the diff and it is straight forward and looks good. Two things that I would like to be discussed/changed before I will apply the patch: Marking the other methods as obsolete is probably not a wise decision since people might still want to get events without clearing them or clear events without wasting memory to get them. In general, the memory appender was not invented to be a threadsafe FIFO queue. This said, is there a special reason why you did not create a new class that extends the MemoryAppender and adds your functionality? Cheers
        Hide
        babskig Gregory Babski added a comment -

        I did in fact implement a custom appender that extends MemoryAppender - based off of a much older version of log4net. I came to depend on the memory appender when our code is run in a manual test framework - redirecting log events to UI (rather than to file or syslog when the code runs ion production.) My implementation was initially aimed at fixing the ArrayOutOfBounds Exception, as I discovered is now fixed as described in https://issues.apache.org/jira/browse/LOG4NET-167.

        I was updating our log4net dependencies and saw that fix and realised I could offer my changes for consideration.

        Regarding the use obsolete tag - I defer to your experience, since I have never had to maintain widely used library.

        Thanks for the feedback!

        Show
        babskig Gregory Babski added a comment - I did in fact implement a custom appender that extends MemoryAppender - based off of a much older version of log4net. I came to depend on the memory appender when our code is run in a manual test framework - redirecting log events to UI (rather than to file or syslog when the code runs ion production.) My implementation was initially aimed at fixing the ArrayOutOfBounds Exception, as I discovered is now fixed as described in https://issues.apache.org/jira/browse/LOG4NET-167 . I was updating our log4net dependencies and saw that fix and realised I could offer my changes for consideration. Regarding the use obsolete tag - I defer to your experience, since I have never had to maintain widely used library. Thanks for the feedback!
        Hide
        bodewig Stefan Bodewig added a comment -

        added PopAllEvents with svn revision 1669935. I'm with Dominik there may be use cases where you only want to peek at the events - or only want to clear them.

        Show
        bodewig Stefan Bodewig added a comment - added PopAllEvents with svn revision 1669935. I'm with Dominik there may be use cases where you only want to peek at the events - or only want to clear them.

          People

          • Assignee:
            Unassigned
            Reporter:
            babskig Gregory Babski
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development