Index: logback-core/pom.xml
===================================================================
--- logback-core/pom.xml	(revision 1953)
+++ logback-core/pom.xml	(working copy)
@@ -51,6 +51,13 @@
       <scope>compile</scope>
       <optional>true</optional>
     </dependency>
+    <dependency>
+        <groupId>org.easymock</groupId>
+        <artifactId>easymock</artifactId>
+        <version>2.4</version>
+        <scope>test</scope>
+    </dependency>
+    
   </dependencies>
 
 
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 1953)
+++ logback-core/src/main/java/ch/qos/logback/core/spi/AppenderAttachableImpl.java	(working copy)
@@ -39,10 +39,13 @@
       throw new IllegalArgumentException("Null argument disallowed");
     }
     w.lock();
-    if (!appenderList.contains(newAppender)) {
-      appenderList.add(newAppender);
+    try {
+      if (!appenderList.contains(newAppender)) {
+        appenderList.add(newAppender);
+      }
+    } finally {
+      w.unlock();
     }
-    w.unlock();
   }
 
   /**
@@ -71,8 +74,11 @@
   public Iterator<Appender<E>> iteratorForAppenders() {
     List<Appender<E>> copy;
     r.lock();
-    copy = new ArrayList<Appender<E>>(appenderList);
-    r.unlock();
+    try {
+      copy = new ArrayList<Appender<E>>(appenderList);
+    } finally {
+      r.unlock();
+    }
     return copy.iterator();
   }
 
@@ -88,15 +94,18 @@
       return null;
     }
     r.lock();
-
-    for (Appender<E> appender : appenderList) {
-      if (name.equals(appender.getName())) {
-        r.unlock();
-        return appender;
+    Appender<E> found=null;
+    try {
+      for (Appender<E>appender : appenderList) {
+        if (name.equals(appender.getName())) {
+          found=appender;
+          break;
+        }
       }
+    } finally {
+      r.unlock();
     }
-    r.unlock();
-    return null;
+    return found;
   }
 
   /**
@@ -110,26 +119,30 @@
       return false;
     }
     r.lock();
-    for (Appender<E> a : appenderList) {
-      if (a == appender) {
-        r.unlock();
-        return true;
+    boolean attached=false;
+    try {
+      for (Appender<E> a : appenderList) {
+        if (a == appender) {
+          attached=true;
+          break;
+        }
       }
+    } finally {
+      r.unlock();
     }
-    r.unlock();
-    return false;
+    return attached;
   }
 
   /**
    * Remove and stop all previously attached appenders.
    */
   public void detachAndStopAllAppenders() {
+    w.lock();
     try {
-     w.lock();
       for (Appender<E> a : appenderList) {
-       a.stop();
-     }
-    appenderList.clear();
+        a.stop();
+      }
+      appenderList.clear();
     } finally {
       w.unlock();
     }
@@ -144,9 +157,13 @@
       return false;
     }
     w.lock();
-    boolean result = appenderList.remove(appender);
-    w.unlock();
-    return result;
+    boolean removed;
+    try {
+	    removed = appenderList.remove(appender);
+    } finally {
+      w.unlock();
+    }
+    return removed;
   }
 
   /**
@@ -158,13 +175,17 @@
       return false;
     }
     w.lock();
-    for (Appender<E> a : appenderList) {
-      if (name.equals((a).getName())) {
-        w.unlock();
-        return appenderList.remove(a);
+    boolean removed=false;
+    try {
+      for (Appender<E> a : appenderList) {
+        if (name.equals((a).getName())) {
+          removed=appenderList.remove(a);
+          break;
+        }
       }
+    } finally {
+      w.unlock();
     }
-    w.unlock();
-    return false;
+    return removed;
   }
 }
Index: logback-core/src/test/java/ch/qos/logback/core/spi/AppenderAttachableImplLockTest.java
===================================================================
--- logback-core/src/test/java/ch/qos/logback/core/spi/AppenderAttachableImplLockTest.java	(revision 0)
+++ logback-core/src/test/java/ch/qos/logback/core/spi/AppenderAttachableImplLockTest.java	(revision 0)
@@ -0,0 +1,101 @@
+package ch.qos.logback.core.spi;
+
+import org.junit.Before;
+import org.junit.Test;
+import static org.easymock.EasyMock.createStrictMock;
+import static org.easymock.EasyMock.expectLastCall;
+import static org.easymock.EasyMock.replay;
+import static org.easymock.EasyMock.verify;
+import static org.easymock.EasyMock.makeThreadSafe;
+import ch.qos.logback.core.Appender;
+
+import java.io.IOException;
+
+/**
+ * This test shows the general problem I described in LBCORE-67.
+ *
+ * In the two testcases below I'm using an appender that throws an OutOfMemoryError while
+ * getName is called - but this is just an example to show the general problem.
+ *
+ * The tests below fail without my change and work if my patch is applied.
+ *
+ * Additionally, the following, probably more realistic, situations could happen:
+ *
+ * -addAppender: appenderList.add() could throw OutOfMemoryError. This could only be shown
+ *  by using an appenderList mock but appenderList does not (and should not) have a setter.
+ *  This would leave the write lock locked.
+ *
+ * -iteratorForAppenders: new ArrayList() could throw an OutOfMemoryError,
+ * leaving the read lock locked.
+ *
+ * I can't imagine a bad situation in isAttached, detachAppender(Appender) or
+ * detachAppender(String) but I'd change the code anyway for consistency. I'm also
+ * pretty sure that something stupid can happen at any time so it's best to just stick
+ * to conventions.
+ */
+public class AppenderAttachableImplLockTest
+{
+  private AppenderAttachableImpl<Integer> instance;
+
+  @Before
+  public void init()
+  {
+    instance=new AppenderAttachableImpl<Integer>();
+  }
+
+  @Test(timeout=1000)
+  public void getAppenderBoom()
+  {
+    Appender<Integer> mockAppender1=createStrictMock(Appender.class);
+    mockAppender1.getName();
+    expectLastCall().andThrow(new OutOfMemoryError("ups"));
+    replay(mockAppender1);
+
+    Appender<Integer> mockAppender2=createStrictMock(Appender.class);
+
+    instance.addAppender(mockAppender1);
+    try {
+      instance.getAppender("foo");
+    }
+    catch(OutOfMemoryError e) {
+      // this leaves the read lock locked.
+    }
+
+    // the next call will lock
+    instance.addAppender(mockAppender2);
+    verify(mockAppender1);    
+  }
+
+  @Test(timeout=1000)
+  public void detachAppenderBoom() throws InterruptedException
+  {
+    Appender<Integer> mockAppender=createStrictMock(Appender.class);
+    makeThreadSafe(mockAppender,true);
+    mockAppender.getName();
+    expectLastCall().andThrow(new OutOfMemoryError("ups"));
+    mockAppender.doAppend(17);
+    replay(mockAppender);
+
+    instance.addAppender(mockAppender);
+    Thread t=new Thread(new Runnable()
+    {
+
+      public void run()
+      {
+        try {
+          instance.detachAppender("foo");
+        }
+        catch(OutOfMemoryError e) {
+          // this leaves the write lock locked.
+        }
+      }
+    });
+    t.start();
+    t.join();
+
+    // the next call will lock
+    instance.appendLoopOnAppenders(17);
+    verify(mockAppender);
+  }
+
+}