Uploaded image for project: 'logback'
  1. logback
  2. LOGBACK-1148

AsyncAppender thread safety issues

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Open
    • Priority: Minor
    • Resolution: Unresolved
    • Affects Version/s: 1.1.6
    • Fix Version/s: None
    • Component/s: logback-classic
    • Labels:
      None

      Description

      AsyncAppender in logback classic 1.1.6 is not entirely thread safe, and its design doesn't allow for its use in an entirely thread-safe way.


      #1. Visibility of the "started" flag in AsyncAppender's grandparent class (UnsynchronizedAppenderBase).

      The doAppend() method in UnsynchronizedAppenderBase checks the "started" boolean member's value (line 72) before appending; however no synchronization primitives are used to ensure that the "started" member's current value is visible to all threads once it is updated via start() or stop(). This could theoretically result in skipping certain log messages (if the appender was very recently started and the value was not visible to the logging thread). At the same time, such an occurance is probably unlikely since start() will typically be called very early in the application's lifecycle.

      The fix for this issue is probably to have AsyncAppender inherit from AppenderBase (which is thread safe and marks "started" as volatile) rather than UnsynchronizedAppenderBase. That being said, the performance impacts of such a change are presently unknown. I will come up with a possible patch and try to investigate the performance impacts further.


      #2. The current implementations of start() and stop() in AsyncAppenderBase are not idempotent nor are multiple (simultaneous) calls to these methods prevented. Since AsyncAppender is generally used by multiple threads, these methods should probably be made thread safe and possibly idempotent (since a caller cannot make use of isStarted() to make a start/stop decision in a thread safe way).

      The easy solution is probably the use of synchronized blocks in start() and stop(). Synchronization is a reasonable solution in this case since (I assume) the performance of AsyncAppender during start and stop is less critical.

        Attachments

          Issue Links

            Activity

              People

              Assignee:
              logback-dev Logback dev list
              Reporter:
              accwebs Aaron Curley
              Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

                Dates

                Created:
                Updated: