Mon Oct 20 20:51:03 CEST 2008

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;
+  }
+
+}
