Details
-
Bug
-
Resolution: Unresolved
-
Minor
-
None
-
1.1.6
-
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
- relates to (out)
-
LOGBACK-277 Implement AsyncAppender
- Resolved