Hi Ceki,
I just made an initial attempt to improve AsyncAppender's thread safety. I provided a link to the commit in my fork (see the latest comment on LOGBACK-1148).
This is just an initial shot at fixing the more obvious problems. Of course, I think more work/thought would be required before we could consider merging this.
Overall, my approach was threefold:
1. make as much stuff final as possible (which ensures safe publication of values to all threads)
2. use locks to protect any mutable members that **did not** need to be accessed during a doAppend() event
3. make any remaining mutable members / member references volatile. For any volatile mutable object members, I also double-checked that the mutable object being referenced was thread safe. Thankfully, they appear to be so.
A couple of specific notes:
A. doAppend() can acquire locks, but only in cases of errors. There is no way to eliminate this synchronization due to the various counter variable increment operations that take place during error conditions. (Volatile only helps in cases of atomic operations)
B. The trickiest part of my change is the stop() method -- my first instinct was to place all of stop()'s code inside a synchronized block (like with my changes to the other non-performance-critical pieces of code); however, I was worried about deadlocks resulting from the wait in worker.join() call and the worker potentially needing to acquire a lock (such as in an error condition). This would cause the worker to block until the worker.interrupt() call timed out (if worker.interrupt() was called within a synchronized block). To work around the problem, I created a "stopping" flag which I set in a synchronized block at the beginning of stop() and clear in another block at the end of stop(). In the middle of the two synchronized blocks, the call to worker.interrupt() occurs unsynchronized.
At your convenience, can you take a look at what I did and send me any thoughts/feedback? Obviously, no rush on this. ;-)