Index: logback-classic/src/test/java/org/dummy/DummyLBAppender.java =================================================================== --- logback-classic/src/test/java/org/dummy/DummyLBAppender.java (revision 1845) +++ logback-classic/src/test/java/org/dummy/DummyLBAppender.java (working copy) @@ -31,7 +31,7 @@ this.layout = layout; } - protected void append(LoggingEvent e) { + protected synchronized void append(LoggingEvent e) { list.add(e); if(layout != null) { String s = layout.doLayout(e); Index: logback-classic/src/test/java/ch/qos/logback/classic/StringListAppender.java =================================================================== --- logback-classic/src/test/java/ch/qos/logback/classic/StringListAppender.java (revision 1845) +++ logback-classic/src/test/java/ch/qos/logback/classic/StringListAppender.java (working copy) @@ -26,7 +26,7 @@ } @Override - protected void append(LoggingEvent eventObject) { + protected synchronized void append(LoggingEvent eventObject) { LoggingEvent le = (LoggingEvent) eventObject; String res = layout.doLayout(le); strList.add(res); Index: logback-classic/src/test/java/ch/qos/logback/classic/LoggerPerfTest.java =================================================================== --- logback-classic/src/test/java/ch/qos/logback/classic/LoggerPerfTest.java (revision 1845) +++ logback-classic/src/test/java/ch/qos/logback/classic/LoggerPerfTest.java (working copy) @@ -19,6 +19,7 @@ import ch.qos.logback.classic.spi.LoggingEvent; import ch.qos.logback.classic.turbo.NOPTurboFilter; import ch.qos.logback.core.appender.NOPAppender; +import ch.qos.logback.core.AppenderBase; public class LoggerPerfTest extends TestCase { @@ -56,6 +57,45 @@ long referencePerf = 48; BogoPerf.assertDuration(avg, referencePerf, REFERENCE_BIPS); } + + public void testThreadedLogging() { + LoggerContext lc = new LoggerContext(); + SleepAppender appender = new SleepAppender(); + appender.setDuration(250); + appender.start(); + Logger logger = lc.getLogger(this.getClass()); + logger.addAppender(appender); + logger.setLevel(Level.DEBUG); + long start = System.nanoTime(); + logger.debug("Toto"); + long end = System.nanoTime(); + long time = end - start; + int threadCount = 10; + int iterCount = 5; + TestRunner[] threads = new TestRunner[threadCount]; + for (int i=0; i < threads.length; ++i) { + threads[i] = new TestRunner(logger, iterCount); + } + start = System.nanoTime(); + for (Thread thread : threads) { + thread.start(); + } + for (TestRunner thread : threads) { + try { + thread.join(); + } catch (InterruptedException ie) { + // Ignore + } + } + end = System.nanoTime(); + double tolerance = threadCount * .125; // Very little thread contention should occur in this test. + double max = ((((double)time)/1000000000) * iterCount) * tolerance; + double serialized = (((double)time)/1000000000) * iterCount * threadCount; + double actual = ((double)(end-start))/1000000000; + System.out.printf("Sleep duration: %,.4f seconds. Max expected: %,.4f seconds, Serialized: %,.4f\n", + actual, max, serialized); + assertTrue("Exceeded maximum expected time.", actual < max); + } double basicDurationInNanos(long len) { LoggerContext lc = new LoggerContext(); @@ -110,5 +150,36 @@ long end = System.nanoTime(); return (end-start)/len; } - + + private static class TestRunner extends Thread { + private Logger logger; + private long len; + + public TestRunner(Logger logger, long len) { + this.logger = logger; + this.len = len; + } + public void run() { + Thread.yield(); + for(long i = 0; i < len; i++) { + logger.debug("Toto"); + } + } + } + public static class SleepAppender extends AppenderBase { + private static long duration = 500; + + public void setDuration(long millis) { + duration = millis; + } + + @Override + protected void append(E eventObject) { + try { + Thread.sleep(duration); + } catch (InterruptedException ie) { + // Ignore + } + } + } } Index: logback-classic/src/main/java/ch/qos/logback/classic/net/JMSTopicAppender.java =================================================================== --- logback-classic/src/main/java/ch/qos/logback/classic/net/JMSTopicAppender.java (revision 1845) +++ logback-classic/src/main/java/ch/qos/logback/classic/net/JMSTopicAppender.java (working copy) @@ -160,7 +160,7 @@ * This method called by {@link AppenderBase#doAppend} method to do most * of the real appending work. */ - public void append(LoggingEvent event) { + public synchronized void append(LoggingEvent event) { if (!isStarted()) { return; } Index: logback-classic/src/main/java/ch/qos/logback/classic/net/JMSQueueAppender.java =================================================================== --- logback-classic/src/main/java/ch/qos/logback/classic/net/JMSQueueAppender.java (revision 1845) +++ logback-classic/src/main/java/ch/qos/logback/classic/net/JMSQueueAppender.java (working copy) @@ -159,7 +159,7 @@ * This method called by {@link AppenderBase#doAppend} method to do most * of the real appending work. */ - public void append(LoggingEvent event) { + public synchronized void append(LoggingEvent event) { if (!isStarted()) { return; } Index: logback-classic/src/main/java/ch/qos/logback/classic/Logger.java =================================================================== --- logback-classic/src/main/java/ch/qos/logback/classic/Logger.java (revision 1845) +++ logback-classic/src/main/java/ch/qos/logback/classic/Logger.java (working copy) @@ -209,22 +209,25 @@ * Remove all previously added appenders from this logger instance.

This * is useful when re-reading configuration information. */ - public synchronized void detachAndStopAllAppenders() { + public void detachAndStopAllAppenders() { if (aai != null) { aai.detachAndStopAllAppenders(); } } - public synchronized Appender detachAppender(String name) { + public Appender detachAppender(String name) { if (aai == null) { return null; } return aai.detachAppender(name); } - public synchronized void addAppender(Appender newAppender) { - if (aai == null) { - aai = new AppenderAttachableImpl(); + public void addAppender(Appender newAppender) { + // Make sure AppenderAttableImpl is only created once. + synchronized(this) { + if (aai == null) { + aai = new AppenderAttachableImpl(); + } } aai.addAppender(newAppender); } @@ -236,7 +239,7 @@ return aai.isAttached(appender); } - public synchronized Iterator iteratorForAppenders() { + public Iterator iteratorForAppenders() { if (aai == null) { return Collections.EMPTY_LIST.iterator(); } @@ -261,12 +264,9 @@ // System.out.println("callAppenders"); for (Logger l = this; l != null; l = l.parent) { // System.out.println("l="+l.getName()); - // Protected against simultaneous call to addAppender, removeAppender,... - synchronized (l) { - writes += l.appendLoopOnAppenders(event); - if (!l.additive) { - break; - } + writes += l.appendLoopOnAppenders(event); + if (!l.additive) { + break; } } @@ -287,7 +287,7 @@ /** * Remove the appender passed as parameter form the list of appenders. */ - public synchronized boolean detachAppender(Appender appender) { + public boolean detachAppender(Appender appender) { if (aai == null) { return false; } Index: logback-core/src/test/java/ch/qos/logback/core/AllCoreTest.java =================================================================== --- logback-core/src/test/java/ch/qos/logback/core/AllCoreTest.java (revision 1845) +++ logback-core/src/test/java/ch/qos/logback/core/AllCoreTest.java (working copy) @@ -24,6 +24,7 @@ suite.addTest(ch.qos.logback.core.joran.PackageTest.suite()); suite.addTest(ch.qos.logback.core.appender.PackageTest.suite()); suite.addTest(ch.qos.logback.core.rolling.PackageTest.suite()); + suite.addTest(ch.qos.logback.core.spi.PackageTest.suite()); return suite; } } Index: logback-core/src/test/java/ch/qos/logback/core/spi/AppenderAttachableImplTest.java =================================================================== --- logback-core/src/test/java/ch/qos/logback/core/spi/AppenderAttachableImplTest.java (revision 0) +++ logback-core/src/test/java/ch/qos/logback/core/spi/AppenderAttachableImplTest.java (revision 0) @@ -0,0 +1,171 @@ +package ch.qos.logback.core.spi; + +import junit.framework.TestCase; +import junit.framework.Test; +import junit.framework.TestSuite; +import ch.qos.logback.core.appender.NOPAppender; +import ch.qos.logback.core.Appender; +import ch.qos.logback.core.layout.NopLayout; + +import java.util.Iterator; + +/** + * This test case verifies all the methods of AppenderAttableImpl work properly. + * + * @author Ceki + */ +public class AppenderAttachableImplTest extends TestCase { + /** + * To run test suite from command line. + */ + public static void main(String[] args) { + junit.textui.TestRunner.run(suite()); + } + + /** + * Set up a test suite. + */ + public static Test suite() { + return new TestSuite(AppenderAttachableImplTest.class); + } + + public AppenderAttachableImplTest(String testName) { + super(testName); + } + + private AppenderAttachableImpl at; + + protected void setUp() throws Exception { + super.setUp(); + at = new AppenderAttachableImpl(); + } + + protected void tearDown() throws Exception { + super.tearDown(); + at = null; + } + + public void testAddAppender() throws Exception { + TestEvent event = new TestEvent(); + NOPAppender ta = new NOPAppender(); + ta.setLayout(new NopLayout()); + ta.start(); + at.addAppender(ta); + ta = new NOPAppender(); + ta.setName("test"); + ta.setLayout(new NopLayout()); + ta.start(); + at.addAppender(ta); + int size = at.appendLoopOnAppenders(event); + assertTrue("Incorrect number of appenders", size == 2); + } + + public void testIteratorForAppenders() throws Exception { + NOPAppender ta = new NOPAppender(); + ta.setLayout(new NopLayout()); + ta.start(); + at.addAppender(ta); + NOPAppender tab = new NOPAppender(); + tab.setName("test"); + tab.setLayout(new NopLayout()); + tab.start(); + at.addAppender(tab); + Iterator> iter = at.iteratorForAppenders(); + int size = 0; + while (iter.hasNext()) { + ++size; + NOPAppender app = iter.next(); + assertTrue("Bad Appender", app == ta || app == tab); + } + assertTrue("Incorrect number of appenders", size == 2); + } + + public void getGetAppender() throws Exception { + NOPAppender ta = new NOPAppender(); + ta.setLayout(new NopLayout()); + ta.start(); + at.addAppender(ta); + NOPAppender tab = new NOPAppender(); + tab.setName("test"); + tab.setLayout(new NopLayout()); + tab.start(); + at.addAppender(tab); + Appender a = at.getAppender("test"); + assertNotNull("Could not find appender", a); + assertTrue("Wrong appender", a == tab); + a = at.getAppender("test1"); + assertNotNull("Could not find appender", a); + assertTrue("Wrong appender", a == ta); + a = at.getAppender("NotThere"); + assertNull("Appender was returned", a); + } + + public void testIsAttached() throws Exception { + NOPAppender ta = new NOPAppender(); + ta.setLayout(new NopLayout()); + ta.start(); + at.addAppender(ta); + NOPAppender tab = new NOPAppender(); + tab.setName("test"); + tab.setLayout(new NopLayout()); + tab.start(); + at.addAppender(tab); + assertTrue("Appender is not attached", at.isAttached(ta)); + assertTrue("Appender is not attached", at.isAttached(tab)); + } + + public void testDetachAndStopAllAppenders() throws Exception { + NOPAppender ta = new NOPAppender(); + ta.setLayout(new NopLayout()); + ta.start(); + at.addAppender(ta); + NOPAppender tab = new NOPAppender(); + tab.setName("test"); + tab.setLayout(new NopLayout()); + tab.start(); + at.addAppender(tab); + assertTrue("Appender was not started", tab.isStarted()); + at.detachAndStopAllAppenders(); + assertNull("Appender was not removed", at.getAppender("test")); + assertFalse("Appender was not stopped", tab.isStarted()); + } + + public void testDetachAppender() throws Exception { + NOPAppender ta = new NOPAppender(); + ta.setLayout(new NopLayout()); + ta.start(); + at.addAppender(ta); + NOPAppender tab = new NOPAppender(); + tab.setName("test"); + tab.setLayout(new NopLayout()); + tab.start(); + at.addAppender(tab); + assertTrue("Appender not detached", at.detachAppender(tab)); + assertNull("Appender was not removed", at.getAppender("test")); + assertFalse("Appender detach error", at.detachAppender(tab)); + } + + public void testDetachAppenderByName() throws Exception { + NOPAppender ta = new NOPAppender(); + ta.setName("test1"); + ta.setLayout(new NopLayout()); + ta.start(); + at.addAppender(ta); + NOPAppender tab = new NOPAppender(); + tab.setName("test"); + tab.setLayout(new NopLayout()); + tab.start(); + at.addAppender(tab); + Appender a = at.detachAppender("test"); + assertNotNull("Appender not detached", a); + a = at.detachAppender("test1"); + assertNotNull("Appender not detached", a); + a = at.detachAppender("test1"); + assertNull("Appender detach error", a); + } + + private static class TestEvent { + + } + +} \ No newline at end of file Index: logback-core/src/test/java/ch/qos/logback/core/spi/PackageTest.java =================================================================== --- logback-core/src/test/java/ch/qos/logback/core/spi/PackageTest.java (revision 0) +++ logback-core/src/test/java/ch/qos/logback/core/spi/PackageTest.java (revision 0) @@ -0,0 +1,15 @@ +package ch.qos.logback.core.spi; + +import junit.framework.TestCase; +import junit.framework.Test; +import junit.framework.TestSuite; + + +public class PackageTest extends TestCase { + + public static Test suite() { + TestSuite suite = new TestSuite(); + suite.addTestSuite(AppenderAttachableImplTest.class); + return suite; + } +} Index: logback-core/src/main/java/ch/qos/logback/core/db/DBAppenderBase.java =================================================================== --- logback-core/src/main/java/ch/qos/logback/core/db/DBAppenderBase.java (revision 1845) +++ logback-core/src/main/java/ch/qos/logback/core/db/DBAppenderBase.java (working copy) @@ -78,7 +78,7 @@ } @Override - public void append(E eventObject) { + public synchronized void append(E eventObject) { Connection connection = null; try { connection = connectionSource.getConnection(); Index: logback-core/src/main/java/ch/qos/logback/core/net/SocketAppenderBase.java =================================================================== --- logback-core/src/main/java/ch/qos/logback/core/net/SocketAppenderBase.java (revision 1845) +++ logback-core/src/main/java/ch/qos/logback/core/net/SocketAppenderBase.java (working copy) @@ -145,7 +145,7 @@ } @Override - protected void append(E event) { + protected synchronized void append(E event) { if (event == null) return; Index: logback-core/src/main/java/ch/qos/logback/core/net/SyslogAppenderBase.java =================================================================== --- logback-core/src/main/java/ch/qos/logback/core/net/SyslogAppenderBase.java (revision 1845) +++ logback-core/src/main/java/ch/qos/logback/core/net/SyslogAppenderBase.java (working copy) @@ -53,7 +53,7 @@ abstract public int getSeverityForEvent(Object eventObject); @Override - protected void append(E eventObject) { + protected synchronized void append(E eventObject) { if (!isStarted()) { return; } Index: logback-core/src/main/java/ch/qos/logback/core/net/SMTPAppenderBase.java =================================================================== --- logback-core/src/main/java/ch/qos/logback/core/net/SMTPAppenderBase.java (revision 1845) +++ logback-core/src/main/java/ch/qos/logback/core/net/SMTPAppenderBase.java (working copy) @@ -138,7 +138,7 @@ * Perform SMTPAppender specific appending actions, delegating some of them to * a subclass and checking if the event triggers an e-mail to be sent. */ - protected void append(E eventObject) { + protected synchronized void append(E eventObject) { if (!checkEntryConditions()) { return; Index: logback-core/src/main/java/ch/qos/logback/core/spi/AppenderAttachableImpl.java =================================================================== --- logback-core/src/main/java/ch/qos/logback/core/spi/AppenderAttachableImpl.java (revision 1845) +++ logback-core/src/main/java/ch/qos/logback/core/spi/AppenderAttachableImpl.java (working copy) @@ -9,9 +9,9 @@ */ package ch.qos.logback.core.spi; -import java.util.ArrayList; import java.util.Iterator; import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; import ch.qos.logback.core.Appender; @@ -22,7 +22,7 @@ */ public class AppenderAttachableImpl implements AppenderAttachable { - final private List> appenderList = new ArrayList>(); + final private List> appenderList = new CopyOnWriteArrayList>(); /** * Attach an appender. If the appender is already in the list in won't be @@ -43,12 +43,9 @@ */ public int appendLoopOnAppenders(E e) { int size = 0; - Appender appender; - - size = appenderList.size(); - for (int i = 0; i < size; i++) { - appender = (Appender) appenderList.get(i); + for (Appender appender : appenderList) { appender.doAppend(e); + ++size; } return size; } @@ -75,13 +72,8 @@ return null; } - int size = appenderList.size(); - Appender appender; - - for (int i = 0; i < size; i++) { - appender = appenderList.get(i); - - if (name.equals(appender.getName())) { + for (Appender appender : appenderList) { + if (name.equals(appender.getName())) { return appender; } } @@ -100,12 +92,7 @@ return false; } - int size = appenderList.size(); - Appender a; - - for (int i = 0; i < size; i++) { - a = (Appender) appenderList.get(i); - + for (Appender a : appenderList) { if (a == appender) { return true; } @@ -118,14 +105,11 @@ * Remove and stop all previously attached appenders. */ public void detachAndStopAllAppenders() { - int len = appenderList.size(); - for (int i = 0; i < len; i++) { - Appender a = (Appender) appenderList.get(i); - a.stop(); + for (Appender appender : appenderList) { + appenderList.remove(appender); + appender.stop(); } - - appenderList.clear(); } /** @@ -148,13 +132,13 @@ return null; } - int size = appenderList.size(); - - for (int i = 0; i < size; i++) { - if (name.equals((appenderList.get(i)).getName())) { - return appenderList.remove(i); + for (Appender appender : appenderList) { + if (name.equals(appender.getName())) { + appenderList.remove(appender); + return appender; } } + return null; } } Index: logback-core/src/main/java/ch/qos/logback/core/AppenderBase.java =================================================================== --- logback-core/src/main/java/ch/qos/logback/core/AppenderBase.java (revision 1845) +++ logback-core/src/main/java/ch/qos/logback/core/AppenderBase.java (working copy) @@ -16,6 +16,9 @@ import ch.qos.logback.core.spi.FilterReply; import ch.qos.logback.core.status.WarnStatus; +import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.concurrent.locks.ReadWriteLock; + /** * This class is used to manage base functionnalities of all appenders. * @@ -30,10 +33,20 @@ protected boolean started = false; /** + * This lock is used to prevent Appenders from stopping while appending + */ + + protected final ReadWriteLock statusLock = new ReentrantReadWriteLock(); + + /** * The guard prevents an appender from repeatedly calling its own doAppend * method. */ - private boolean guard = false; + private ThreadLocal guard = new ThreadLocal() { + @Override protected Boolean initialValue() { + return false; + } + }; /** * Appenders are named. @@ -52,17 +65,17 @@ static final int ALLOWED_REPEATS = 5; - public synchronized void doAppend(E eventObject) { + public void doAppend(E eventObject) { // WARNING: The guard check MUST be the first statement in the // doAppend() method. // prevent re-entry. - if (guard) { + if (guard.get()) { return; } - + statusLock.readLock().lock(); try { - guard = true; + guard.set(true); if (!this.started) { if (statusRepeatCount++ < ALLOWED_REPEATS) { @@ -85,7 +98,8 @@ addError("Appender ["+name+"] failed to append.", e); } } finally { - guard = false; + statusLock.readLock().unlock(); + guard.set(false); } } @@ -102,8 +116,15 @@ started = true; } + /** + * Once stop completes doAppend calls will return without attempting to process the event. + * The write lock insures that the appender is never marked as stopped while an append + * is in progress. + */ public void stop() { + statusLock.writeLock().lock(); started = false; + statusLock.writeLock().unlock(); } public boolean isStarted() { Index: logback-core/src/main/java/ch/qos/logback/core/read/CyclicBufferAppender.java =================================================================== --- logback-core/src/main/java/ch/qos/logback/core/read/CyclicBufferAppender.java (revision 1845) +++ logback-core/src/main/java/ch/qos/logback/core/read/CyclicBufferAppender.java (working copy) @@ -35,7 +35,7 @@ } @Override - protected void append(E eventObject) { + protected synchronized void append(E eventObject) { if (!isStarted()) { return; } Index: logback-core/src/main/java/ch/qos/logback/core/read/ListAppender.java =================================================================== --- logback-core/src/main/java/ch/qos/logback/core/read/ListAppender.java (revision 1845) +++ logback-core/src/main/java/ch/qos/logback/core/read/ListAppender.java (working copy) @@ -18,7 +18,7 @@ public List list = new ArrayList(); - protected void append(E e) { + protected synchronized void append(E e) { list.add(e); } } Index: logback-core/src/main/java/ch/qos/logback/core/WriterAppender.java =================================================================== --- logback-core/src/main/java/ch/qos/logback/core/WriterAppender.java (revision 1845) +++ logback-core/src/main/java/ch/qos/logback/core/WriterAppender.java (working copy) @@ -273,7 +273,7 @@ * * @since 0.9.0 */ - protected void subAppend(E event) { + protected synchronized void subAppend(E event) { if (!isStarted()) { return; } Index: logback-examples/src/main/java/chapter4/CountingConsoleAppender.java =================================================================== --- logback-examples/src/main/java/chapter4/CountingConsoleAppender.java (revision 1845) +++ logback-examples/src/main/java/chapter4/CountingConsoleAppender.java (working copy) @@ -43,7 +43,7 @@ super.start(); } - public void append(LoggingEvent event) { + public synchronized void append(LoggingEvent event) { if (counter >= limit) { return;