8169465: Deadlock in com.sun.jndi.ldap.pool.Connections

Reviewed-by: dfuchs, vtewari

src/share/classes/com/sun/jndi/ldap/pool/Connections.java

@@ -27,7 +27,6 @@ package com.sun.jndi.ldap.pool;
 
 import java.util.ArrayList; // JDK 1.2
 import java.util.List;
-import java.util.Iterator;
 
 import java.lang.ref.Reference;
 import java.lang.ref.SoftReference;
@@ -290,24 +289,29 @@ final class Connections implements PoolCallback {
      * @param threshold an entry idle since this time has expired.
      * @return true if no more connections in list
      */
-    synchronized boolean expire(long threshold) {
-        Iterator<ConnectionDesc> iter = conns.iterator();
-        ConnectionDesc entry;
-        while (iter.hasNext()) {
-            entry = iter.next();
-            if (entry.expire(threshold)) {
-                d("expire(): removing ", entry);
-                td("Expired ", entry);
+    boolean expire(long threshold) {
+        List<ConnectionDesc> clonedConns;
+        synchronized(this) {
+            clonedConns = new ArrayList<>(conns);
+        }
+        List<ConnectionDesc> expired = new ArrayList<>();
 
-                iter.remove();  // remove from pool
+        for (ConnectionDesc entry : clonedConns) {
+            d("expire(): ", entry);
+            if (entry.expire(threshold)) {
+                expired.add(entry);
+                td("expire(): Expired ", entry);
+            }
+        }
 
+        synchronized (this) {
+            conns.removeAll(expired);
             // Don't need to call notify() because we're
             // removing only idle connections. If there were
             // idle connections, then there should be no waiters.
-            }
-        }
             return conns.isEmpty();  // whether whole list has 'expired'
         }
+    }
 
     /**
      * Called when this instance of Connections has been removed from Pool.

src/share/classes/com/sun/jndi/ldap/pool/Pool.java

@@ -25,11 +25,11 @@
 
 package com.sun.jndi.ldap.pool;
 
+import java.util.ArrayList;
 import java.util.Map;
 import java.util.WeakHashMap;
 import java.util.Collection;
 import java.util.Collections;
-import java.util.Iterator;
 import java.util.LinkedList;
 
 import java.io.PrintStream;
@@ -166,17 +166,25 @@ final public class Pool {
      *          and removed.
      */
     public void expire(long threshold) {
+        Collection<ConnectionsRef> copy;
         synchronized (map) {
-            Iterator<ConnectionsRef> iter = map.values().iterator();
+            copy = new ArrayList<>(map.values());
+        }
+
+        ArrayList<ConnectionsRef> removed = new ArrayList<>();
         Connections conns;
-            while (iter.hasNext()) {
-                conns = iter.next().getConnections();
+        for (ConnectionsRef ref : copy) {
+            conns = ref.getConnections();
             if (conns.expire(threshold)) {
                 d("expire(): removing ", conns);
-                    iter.remove();
+                removed.add(ref);
             }
         }
+
+        synchronized (map) {
+            map.values().removeAll(removed);
         }
+
         expungeStaleConnections();
     }