[logback-dev] svn commit: r2046 - logback/trunk/logback-core/src/main/java/ch/qos/logback/core/spi

[noreply.ceki at qos.ch]

Author: ceki
Date: Tue Dec  2 20:37:44 2008
New Revision: 2046

Modified:
   logback/trunk/logback-core/src/main/java/ch/qos/logback/core/spi/AppenderAttachableImpl.java

Log:
Fixed LBCORE-67

I prefer
  try {
    r.lock();
    doSomething();
  } finally  {
    r.unlock();
  }
  
instead of 

  r.lock();
  try {
    doSomething();  
  } finally  {
    r.unlock();
  }

  because I think that the lock is held for a shorter time (however small it may be).
  In case r.lock() throws an exception, we are probably screwed in both approaches.

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	Tue Dec  2 20:37:44 2008
@@ -1,11 +1,11 @@
 /**
- * LOGBack: the generic, reliable, fast and flexible logging framework.
- *
- * Copyright (C) 1999-2006, 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.
+ * 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;
 
@@ -19,7 +19,8 @@
 import ch.qos.logback.core.Appender;
 
 /**
- * A straightforward implementation of the {@link AppenderAttachable} interface.
+ * A ReentrantReadWriteLock based implementation of the
+ * {@link AppenderAttachable} interface.
  * 
  * @author Ceki Gülcü
  */
@@ -38,11 +39,14 @@
     if (newAppender == null) {
       throw new IllegalArgumentException("Null argument disallowed");
     }
-    w.lock();
-    if (!appenderList.contains(newAppender)) {
-      appenderList.add(newAppender);
+    try {
+      w.lock();
+      if (!appenderList.contains(newAppender)) {
+        appenderList.add(newAppender);
+      }
+    } finally {
+      w.unlock();
     }
-    w.unlock();
   }
 
   /**
@@ -50,8 +54,8 @@
    */
   public int appendLoopOnAppenders(E e) {
     int size = 0;
-    r.lock();
     try {
+      r.lock();
       for (Appender<E> appender : appenderList) {
         appender.doAppend(e);
         size++;
@@ -70,33 +74,40 @@
    */
   public Iterator<Appender<E>> iteratorForAppenders() {
     List<Appender<E>> copy;
-    r.lock();
-    copy = new ArrayList<Appender<E>>(appenderList);
-    r.unlock();
+    try {
+      r.lock();
+      copy = new ArrayList<Appender<E>>(appenderList);
+    } finally {
+      r.unlock();
+    }
     return copy.iterator();
   }
 
   /**
    * Look for an attached appender named as <code>name</code>.
    * 
-   * <p>
-   * Return the appender with that name if in the list. Return null otherwise.
+   * <p> Return the appender with that name if in the list. Return null
+   * otherwise.
    * 
    */
   public Appender<E> getAppender(String name) {
     if (name == null) {
       return null;
     }
-    r.lock();
+    Appender<E> found = null;
 
-    for (Appender<E> appender : appenderList) {
-      if (name.equals(appender.getName())) {
-        r.unlock();
-        return appender;
+    try {
+      r.lock();
+      for (Appender<E> appender : appenderList) {
+        if (name.equals(appender.getName())) {
+          found = appender;
+          break;
+        }
       }
+    } finally {
+      r.unlock();
     }
-    r.unlock();
-    return null;
+    return found;
   }
 
   /**
@@ -109,15 +120,19 @@
     if (appender == null) {
       return false;
     }
-    r.lock();
-    for (Appender<E> a : appenderList) {
-      if (a == appender) {
-        r.unlock();
-        return true;
+    boolean attached = false;
+    try {
+      r.lock();
+      for (Appender<E> a : appenderList) {
+        if (a == appender) {
+          attached = true;
+          break;
+        }
       }
+    } finally {
+      r.unlock();
     }
-    r.unlock();
-    return false;
+    return attached;
   }
 
   /**
@@ -125,11 +140,11 @@
    */
   public void detachAndStopAllAppenders() {
     try {
-     w.lock();
+      w.lock();
       for (Appender<E> a : appenderList) {
-       a.stop();
-     }
-    appenderList.clear();
+        a.stop();
+      }
+      appenderList.clear();
     } finally {
       w.unlock();
     }
@@ -143,9 +158,13 @@
     if (appender == null) {
       return false;
     }
-    w.lock();
-    boolean result = appenderList.remove(appender);
-    w.unlock();
+    boolean result;
+    try {
+      w.lock();
+      result = appenderList.remove(appender);
+    } finally {
+      w.unlock();
+    }
     return result;
   }
 
@@ -157,14 +176,18 @@
     if (name == null) {
       return false;
     }
-    w.lock();
-    for (Appender<E> a : appenderList) {
-      if (name.equals((a).getName())) {
-        w.unlock();
-        return appenderList.remove(a);
+    boolean removed = false;
+    try {
+      w.lock();
+      for (Appender<E> a : appenderList) {
+        if (name.equals((a).getName())) {
+          removed = appenderList.remove(a);
+          break;
+        }
       }
+    } finally {
+      w.unlock();
     }
-    w.unlock();
-    return false;
+    return removed;
   }
 }