Author: ceki Date: Mon Oct 20 20:51:02 2008 New Revision: 1852 Added: logback/trunk/logback-core/src/test/java/ch/qos/logback/core/spi/ logback/trunk/logback-core/src/test/java/ch/qos/logback/core/spi/AppenderAttachableImplTest.java logback/trunk/logback-core/src/test/java/ch/qos/logback/core/spi/PackageTest.java Modified: logback/trunk/logback-access/src/main/java/ch/qos/logback/access/jetty/RequestLogImpl.java logback/trunk/logback-access/src/main/java/ch/qos/logback/access/spi/AccessContext.java logback/trunk/logback-access/src/main/java/ch/qos/logback/access/tomcat/LogbackValve.java logback/trunk/logback-classic/src/main/java/ch/qos/logback/classic/Logger.java logback/trunk/logback-classic/src/test/java/ch/qos/logback/classic/HLogger.java logback/trunk/logback-classic/src/test/java/ch/qos/logback/classic/control/ControlLogger.java logback/trunk/logback-core/src/main/java/ch/qos/logback/core/spi/AppenderAttachable.java logback/trunk/logback-core/src/main/java/ch/qos/logback/core/spi/AppenderAttachableImpl.java logback/trunk/logback-core/src/test/java/ch/qos/logback/core/AllCoreTest.java Log: - detachAppender in AppenderAttachable now returns a boolean instead of an Appender. - iteratorForAppenders() in AppenderAttachable now returns Iterator<Appender<E>> instead of just Iterator - modified Logger to invoke callAppender without synchronization on this - AppenderAttachableImpl modified to take advantage of the fact that reads occur much more often than writes - associated test cases Modified: logback/trunk/logback-access/src/main/java/ch/qos/logback/access/jetty/RequestLogImpl.java ============================================================================== --- logback/trunk/logback-access/src/main/java/ch/qos/logback/access/jetty/RequestLogImpl.java (original) +++ logback/trunk/logback-access/src/main/java/ch/qos/logback/access/jetty/RequestLogImpl.java Mon Oct 20 20:51:02 2008 @@ -166,7 +166,7 @@ getStatusManager().add( new InfoStatus("RequestLog added to RequestLogRegistry with name: " + getName(), this)); - + started = true; } catch (JoranException e) { @@ -198,7 +198,7 @@ public boolean isStopping() { return false; } - + public boolean isStopped() { return !started; } @@ -211,7 +211,7 @@ aai.addAppender(newAppender); } - public Iterator iteratorForAppenders() { + public Iterator<Appender<AccessEvent>> iteratorForAppenders() { return aai.iteratorForAppenders(); } @@ -232,7 +232,7 @@ return aai.detachAppender(appender); } - public Appender<AccessEvent> detachAppender(String name) { + public boolean detachAppender(String name) { return aai.detachAppender(name); } Modified: logback/trunk/logback-access/src/main/java/ch/qos/logback/access/spi/AccessContext.java ============================================================================== --- logback/trunk/logback-access/src/main/java/ch/qos/logback/access/spi/AccessContext.java (original) +++ logback/trunk/logback-access/src/main/java/ch/qos/logback/access/spi/AccessContext.java Mon Oct 20 20:51:02 2008 @@ -39,7 +39,7 @@ return aai.detachAppender(appender); } - public Appender<AccessEvent> detachAppender(String name) { + public boolean detachAppender(String name) { return aai.detachAppender(name); } @@ -51,7 +51,7 @@ return aai.isAttached(appender); } - public Iterator iteratorForAppenders() { + public Iterator<Appender<AccessEvent>> iteratorForAppenders() { return aai.iteratorForAppenders(); } Modified: logback/trunk/logback-access/src/main/java/ch/qos/logback/access/tomcat/LogbackValve.java ============================================================================== --- logback/trunk/logback-access/src/main/java/ch/qos/logback/access/tomcat/LogbackValve.java (original) +++ logback/trunk/logback-access/src/main/java/ch/qos/logback/access/tomcat/LogbackValve.java Mon Oct 20 20:51:02 2008 @@ -148,7 +148,7 @@ aai.addAppender(newAppender); } - public Iterator iteratorForAppenders() { + public Iterator<Appender<AccessEvent>> iteratorForAppenders() { return aai.iteratorForAppenders(); } @@ -169,7 +169,7 @@ return aai.detachAppender(appender); } - public Appender<AccessEvent> detachAppender(String name) { + public boolean detachAppender(String name) { return aai.detachAppender(name); } Modified: logback/trunk/logback-classic/src/main/java/ch/qos/logback/classic/Logger.java ============================================================================== --- logback/trunk/logback-classic/src/main/java/ch/qos/logback/classic/Logger.java (original) +++ logback/trunk/logback-classic/src/main/java/ch/qos/logback/classic/Logger.java Mon Oct 20 20:51:02 2008 @@ -66,6 +66,26 @@ */ private List<Logger> childrenList; + /** + * It is assumed that once the 'aai' variable is set to a non-null value, it + * will never be reset to null. it is further assumed that only place where + * the 'aai'ariable is set is within the addAppender method. This method is + * synchronized on 'this' (Logger) protecting against simultaneous + * re-configuration of this logger (a very unlikely scenario). + * + * <p> + * It is further assumed that the AppenderAttachableImpl is responsible for + * its internal synchronization and thread safety. Thus, we can get away with + * *not* synchronizing on the 'aai' (check null/ read) because + * <p> + * 1) the 'aai' variable is immutable once set to non-null + * <p> + * 2) 'aai' is getAndSet only within addAppender which is synchronized + * <p> + * 3) all the other methods check whether 'aai' is null + * <p> + * 4) AppenderAttachableImpl is thread safe + */ private transient AppenderAttachableImpl<LoggingEvent> aai; /** * Additivity is set to true by default, that is children inherit the @@ -193,7 +213,6 @@ if (level == null) { effectiveLevelInt = newParentLevel.levelInt; - // propagate the parent levelInt change to this logger's children if (childrenList != null) { int len = childrenList.size(); @@ -209,19 +228,21 @@ * Remove all previously added appenders from this logger instance. <p/> This * is useful when re-reading configuration information. */ - public synchronized void detachAndStopAllAppenders() { + public void detachAndStopAllAppenders() { if (aai != null) { aai.detachAndStopAllAppenders(); } } - public synchronized Appender<LoggingEvent> detachAppender(String name) { + public boolean detachAppender(String name) { if (aai == null) { - return null; + return false; } return aai.detachAppender(name); } + // this method MUST be synchronized. See comments on 'aai' field for further + // details. public synchronized void addAppender(Appender<LoggingEvent> newAppender) { if (aai == null) { aai = new AppenderAttachableImpl<LoggingEvent>(); @@ -236,7 +257,8 @@ return aai.isAttached(appender); } - public synchronized Iterator iteratorForAppenders() { + @SuppressWarnings("unchecked") + public Iterator<Appender<LoggingEvent>> iteratorForAppenders() { if (aai == null) { return Collections.EMPTY_LIST.iterator(); } @@ -254,22 +276,16 @@ * Invoke all the appenders of this logger. * * @param event - * The event to log + * The event to log */ public void callAppenders(LoggingEvent event) { int writes = 0; - // 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; } } - // No appenders in hierarchy if (writes == 0) { loggerContext.noAppenderDefinedWarning(this); @@ -277,17 +293,17 @@ } private int appendLoopOnAppenders(LoggingEvent event) { - int size = 0; if (aai != null) { - size = aai.appendLoopOnAppenders(event); + return aai.appendLoopOnAppenders(event); + } else { + return 0; } - return size; } /** * 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; } @@ -304,9 +320,9 @@ * logger. * * @param lastPart - * the suffix (i.e. last part) of the child logger - * name. This parameter may not include dots, i.e. the - * logger separator character. + * the suffix (i.e. last part) of the child logger name. This + * parameter may not include dots, i.e. the logger separator + * character. * @return */ Logger createChildByLastNamePart(final String lastPart) { Modified: logback/trunk/logback-classic/src/test/java/ch/qos/logback/classic/HLogger.java ============================================================================== --- logback/trunk/logback-classic/src/test/java/ch/qos/logback/classic/HLogger.java (original) +++ logback/trunk/logback-classic/src/test/java/ch/qos/logback/classic/HLogger.java Mon Oct 20 20:51:02 2008 @@ -25,6 +25,8 @@ public class HLogger extends MarkerIgnoringBase { + private static final long serialVersionUID = 1L; + static int instanceCount = 0; /** Modified: logback/trunk/logback-classic/src/test/java/ch/qos/logback/classic/control/ControlLogger.java ============================================================================== --- logback/trunk/logback-classic/src/test/java/ch/qos/logback/classic/control/ControlLogger.java (original) +++ logback/trunk/logback-classic/src/test/java/ch/qos/logback/classic/control/ControlLogger.java Mon Oct 20 20:51:02 2008 @@ -17,6 +17,8 @@ * See javadoc for ControlLoggerContext. */ public class ControlLogger extends MarkerIgnoringBase { + + private static final long serialVersionUID = 1L; final ControlLogger parent; final String name; Level level; Modified: logback/trunk/logback-core/src/main/java/ch/qos/logback/core/spi/AppenderAttachable.java ============================================================================== --- logback/trunk/logback-core/src/main/java/ch/qos/logback/core/spi/AppenderAttachable.java (original) +++ logback/trunk/logback-core/src/main/java/ch/qos/logback/core/spi/AppenderAttachable.java Mon Oct 20 20:51:02 2008 @@ -27,7 +27,7 @@ /** * Get an iterator for appenders contained in the parent object. */ - public Iterator iteratorForAppenders(); + public Iterator<Appender<E>> iteratorForAppenders(); /** * Get an appender by name. @@ -41,9 +41,9 @@ public boolean isAttached(Appender<E> appender); /** - * Detach all previously added appenders. + * Detach and stop all previously added appenders. */ - void detachAndStopAllAppenders(); + void detachAndStopAllAppenders(); /** * Detach the appender passed as parameter from the list of appenders. @@ -54,5 +54,5 @@ * Detach the appender with the name passed as parameter from the list of * appenders. */ - Appender<E> detachAppender(String name); + boolean detachAppender(String name); } Modified: logback/trunk/logback-core/src/main/java/ch/qos/logback/core/spi/AppenderAttachableImpl.java ============================================================================== --- logback/trunk/logback-core/src/main/java/ch/qos/logback/core/spi/AppenderAttachableImpl.java (original) +++ logback/trunk/logback-core/src/main/java/ch/qos/logback/core/spi/AppenderAttachableImpl.java Mon Oct 20 20:51:02 2008 @@ -9,9 +9,8 @@ */ 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,20 +21,17 @@ */ public class AppenderAttachableImpl<E> implements AppenderAttachable<E> { - final private List<Appender<E>> appenderList = new ArrayList<Appender<E>>(); + final private CopyOnWriteArrayList<Appender<E>> appenderList = new CopyOnWriteArrayList<Appender<E>>(); /** * Attach an appender. If the appender is already in the list in won't be * added again. */ public void addAppender(Appender<E> newAppender) { - // Null values for newAppender parameter are strictly forbidden. if (newAppender == null) { - throw new IllegalArgumentException("Cannot null as an appener"); - } - if (!appenderList.contains(newAppender)) { - appenderList.add(newAppender); + throw new IllegalArgumentException("Null argument disallowed"); } + appenderList.addIfAbsent(newAppender); } /** @@ -43,12 +39,9 @@ */ public int appendLoopOnAppenders(E e) { int size = 0; - Appender<E> appender; - - size = appenderList.size(); - for (int i = 0; i < size; i++) { - appender = (Appender<E>) appenderList.get(i); + for (Appender<E> appender : appenderList) { appender.doAppend(e); + size++; } return size; } @@ -57,9 +50,9 @@ * Get all attached appenders as an Enumeration. If there are no attached * appenders <code>null</code> is returned. * - * @return Enumeration An enumeration of attached appenders. + * @return Iterator An iterator of attached appenders. */ - public Iterator iteratorForAppenders() { + public Iterator<Appender<E>> iteratorForAppenders() { return appenderList.iterator(); } @@ -74,18 +67,11 @@ if (name == null) { return null; } - - int size = appenderList.size(); - Appender<E> appender; - - for (int i = 0; i < size; i++) { - appender = appenderList.get(i); - + for (Appender<E> appender : appenderList) { if (name.equals(appender.getName())) { return appender; } } - return null; } @@ -99,18 +85,11 @@ if (appender == null) { return false; } - - int size = appenderList.size(); - Appender a; - - for (int i = 0; i < size; i++) { - a = (Appender) appenderList.get(i); - + for (Appender<E> a : appenderList) { if (a == appender) { return true; } } - return false; } @@ -118,13 +97,9 @@ * 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); + for (Appender<E> a : appenderList) { a.stop(); } - appenderList.clear(); } @@ -143,18 +118,15 @@ * Remove the appender with the name passed as parameter form the list of * appenders. */ - public Appender<E> detachAppender(String name) { + public boolean detachAppender(String name) { if (name == null) { - return null; + return false; } - - int size = appenderList.size(); - - for (int i = 0; i < size; i++) { - if (name.equals((appenderList.get(i)).getName())) { - return appenderList.remove(i); + for (Appender<E> a : appenderList) { + if (name.equals((a).getName())) { + return appenderList.remove(a); } } - return null; + return false; } } Modified: logback/trunk/logback-core/src/test/java/ch/qos/logback/core/AllCoreTest.java ============================================================================== --- logback/trunk/logback-core/src/test/java/ch/qos/logback/core/AllCoreTest.java (original) +++ logback/trunk/logback-core/src/test/java/ch/qos/logback/core/AllCoreTest.java Mon Oct 20 20:51:02 2008 @@ -23,6 +23,7 @@ suite.addTest(ch.qos.logback.core.PackageTest.suite()); suite.addTest(ch.qos.logback.core.joran.PackageTest.suite()); suite.addTest(ch.qos.logback.core.appender.PackageTest.suite()); + suite.addTest(ch.qos.logback.core.spi.PackageTest.suite()); suite.addTest(ch.qos.logback.core.rolling.PackageTest.suite()); return suite; } Added: logback/trunk/logback-core/src/test/java/ch/qos/logback/core/spi/AppenderAttachableImplTest.java ============================================================================== --- (empty file) +++ logback/trunk/logback-core/src/test/java/ch/qos/logback/core/spi/AppenderAttachableImplTest.java Mon Oct 20 20:51:02 2008 @@ -0,0 +1,177 @@ +/** + * Logback: the generic, reliable, fast and flexible logging framework. + * + * Copyright (C) 2000-2008, QOS.ch + * + * This library is free software, you can redistribute it and/or modify it under + * the terms of the GNU Lesser General Public License as published by the Free + * Software Foundation. + */ +package ch.qos.logback.core.spi; + + +import static org.junit.Assert.*; +import ch.qos.logback.core.appender.NOPAppender; +import ch.qos.logback.core.Appender; +import ch.qos.logback.core.layout.NopLayout; + +import java.util.Iterator; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +/** + * This test case verifies all the methods of AppenderAttableImpl work properly. + * + * @author Raplh Goers + */ +public class AppenderAttachableImplTest { + + + private AppenderAttachableImpl<TestEvent> aai; + + @Before + public void setUp() throws Exception { + aai = new AppenderAttachableImpl<TestEvent>(); + } + + @After + public void tearDown() throws Exception { + aai = null; + } + + @Test + public void testAddAppender() throws Exception { + TestEvent event = new TestEvent(); + NOPAppender<TestEvent> ta = new NOPAppender<TestEvent>(); + ta.setLayout(new NopLayout<TestEvent>()); + ta.start(); + aai.addAppender(ta); + ta = new NOPAppender<TestEvent>(); + ta.setName("test"); + ta.setLayout(new NopLayout<TestEvent>()); + ta.start(); + aai.addAppender(ta); + int size = aai.appendLoopOnAppenders(event); + assertTrue("Incorrect number of appenders", size == 2); + } + + @Test + public void testIteratorForAppenders() throws Exception { + NOPAppender<TestEvent> ta = new NOPAppender<TestEvent>(); + ta.setLayout(new NopLayout<TestEvent>()); + ta.start(); + aai.addAppender(ta); + NOPAppender<TestEvent> tab = new NOPAppender<TestEvent>(); + tab.setName("test"); + tab.setLayout(new NopLayout<TestEvent>()); + tab.start(); + aai.addAppender(tab); + Iterator<Appender<TestEvent>> iter = aai.iteratorForAppenders(); + int size = 0; + while (iter.hasNext()) { + ++size; + Appender<TestEvent> app = iter.next(); + assertTrue("Bad Appender", app == ta || app == tab); + } + assertTrue("Incorrect number of appenders", size == 2); + } + + @Test + public void getGetAppender() throws Exception { + NOPAppender<TestEvent> test = new NOPAppender<TestEvent>(); + test.setLayout(new NopLayout<TestEvent>()); + test.setName("test"); + test.start(); + aai.addAppender(test); + + NOPAppender<TestEvent> testOther = new NOPAppender<TestEvent>(); + testOther.setName("testOther"); + testOther.setLayout(new NopLayout<TestEvent>()); + testOther.start(); + aai.addAppender(testOther); + + Appender a = aai.getAppender("testOther"); + assertNotNull("Could not find appender", a); + assertTrue("Wrong appender", a == testOther); + + a = aai.getAppender("test"); + assertNotNull("Could not find appender", a); + assertTrue("Wrong appender", a == test); + a = aai.getAppender("NotThere"); + assertNull("Appender was returned", a); + } + + @Test + public void testIsAttached() throws Exception { + NOPAppender<TestEvent> ta = new NOPAppender<TestEvent>(); + ta.setLayout(new NopLayout<TestEvent>()); + ta.start(); + aai.addAppender(ta); + NOPAppender<TestEvent> tab = new NOPAppender<TestEvent>(); + tab.setName("test"); + tab.setLayout(new NopLayout<TestEvent>()); + tab.start(); + aai.addAppender(tab); + assertTrue("Appender is not attached", aai.isAttached(ta)); + assertTrue("Appender is not attached", aai.isAttached(tab)); + } + + @Test + public void testDetachAndStopAllAppenders() throws Exception { + NOPAppender<TestEvent> ta = new NOPAppender<TestEvent>(); + ta.setLayout(new NopLayout<TestEvent>()); + ta.start(); + aai.addAppender(ta); + NOPAppender<TestEvent> tab = new NOPAppender<TestEvent>(); + tab.setName("test"); + tab.setLayout(new NopLayout<TestEvent>()); + tab.start(); + aai.addAppender(tab); + assertTrue("Appender was not started", tab.isStarted()); + aai.detachAndStopAllAppenders(); + assertNull("Appender was not removed", aai.getAppender("test")); + assertFalse("Appender was not stopped", tab.isStarted()); + } + + @Test + public void testDetachAppender() throws Exception { + NOPAppender<TestEvent> ta = new NOPAppender<TestEvent>(); + ta.setLayout(new NopLayout<TestEvent>()); + ta.start(); + aai.addAppender(ta); + NOPAppender<TestEvent> tab = new NOPAppender<TestEvent>(); + tab.setName("test"); + tab.setLayout(new NopLayout<TestEvent>()); + tab.start(); + aai.addAppender(tab); + assertTrue("Appender not detached", aai.detachAppender(tab)); + assertNull("Appender was not removed", aai.getAppender("test")); + assertFalse("Appender detach error", aai.detachAppender(tab)); + } + + @Test + public void testDetachAppenderByName() throws Exception { + NOPAppender<TestEvent> ta = new NOPAppender<TestEvent>(); + ta.setName("test1"); + ta.setLayout(new NopLayout<TestEvent>()); + ta.start(); + aai.addAppender(ta); + NOPAppender<TestEvent> tab = new NOPAppender<TestEvent>(); + tab.setName("test"); + tab.setLayout(new NopLayout<TestEvent>()); + tab.start(); + aai.addAppender(tab); + + assertTrue(aai.detachAppender("test")); + assertTrue(aai.detachAppender("test1")); + assertFalse( aai.detachAppender("test1")); + } + + private static class TestEvent { + + } + +} + Added: logback/trunk/logback-core/src/test/java/ch/qos/logback/core/spi/PackageTest.java ============================================================================== --- (empty file) +++ logback/trunk/logback-core/src/test/java/ch/qos/logback/core/spi/PackageTest.java Mon Oct 20 20:51:02 2008 @@ -0,0 +1,24 @@ +/** + * Logback: the generic, reliable, fast and flexible logging framework. + * + * Copyright (C) 2000-2008, QOS.ch + * + * This library is free software, you can redistribute it and/or modify it under + * the terms of the GNU Lesser General Public License as published by the Free + * Software Foundation. + */ +package ch.qos.logback.core.spi; + +import junit.framework.JUnit4TestAdapter; +import junit.framework.Test; +import junit.framework.TestSuite; + + +public class PackageTest { + public static Test suite() { + TestSuite suite = new TestSuite(); + suite.addTest(new JUnit4TestAdapter(AppenderAttachableImplTest.class)); + return suite; + } + +}