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

Reviewed-by: dfuchs, vtewari

jdk/src/java.naming/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,23 +289,28 @@ 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) {
+    boolean expire(long threshold) {
-        Iterator<ConnectionDesc> iter = conns.iterator();
+        List<ConnectionDesc> clonedConns;
-        ConnectionDesc entry;
+        synchronized(this) {
-        while (iter.hasNext()) {
+            clonedConns = new ArrayList<>(conns);
-            entry = iter.next();
+        }
+        List<ConnectionDesc> expired = new ArrayList<>();
+
+        for (ConnectionDesc entry : clonedConns) {
+            d("expire(): ", entry);
             if (entry.expire(threshold)) {
-                d("expire(): removing ", entry);
+                expired.add(entry);
-                td("Expired ", entry);
+                td("expire(): Expired ", entry);
-
-                iter.remove();  // remove from pool
-
-                // 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'
+
+        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'
+        }
     }
 
     /**

jdk/src/java.naming/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());
-            Connections conns;
+        }
-            while (iter.hasNext()) {
+
-                conns = iter.next().getConnections();
+        ArrayList<ConnectionsRef> removed = new ArrayList<>();
-                if (conns.expire(threshold)) {
+        Connections conns;
-                    d("expire(): removing ", conns);
+        for (ConnectionsRef ref : copy) {
-                    iter.remove();
+            conns = ref.getConnections();
-                }
+            if (conns.expire(threshold)) {
+                d("expire(): removing ", conns);
+                removed.add(ref);
             }
         }
+
+        synchronized (map) {
+            map.values().removeAll(removed);
+        }
+
         expungeStaleConnections();
     }