8169465: Deadlock in com.sun.jndi.ldap.pool.Connections
Reviewed-by: dfuchs, vtewari
--- a/jdk/src/java.naming/share/classes/com/sun/jndi/ldap/pool/Connections.java Fri Dec 16 09:42:33 2016 -0800
+++ b/jdk/src/java.naming/share/classes/com/sun/jndi/ldap/pool/Connections.java Fri Dec 16 19:15:37 2016 +0000
@@ -27,7 +27,6 @@
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 @@
* @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();
+ boolean expire(long threshold) {
+ List<ConnectionDesc> clonedConns;
+ synchronized(this) {
+ clonedConns = new ArrayList<>(conns);
+ }
+ List<ConnectionDesc> expired = new ArrayList<>();
+
+ for (ConnectionDesc entry : clonedConns) {
+ d("expire(): ", entry);
if (entry.expire(threshold)) {
- d("expire(): removing ", entry);
- td("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.
+ expired.add(entry);
+ td("expire(): Expired ", entry);
}
}
- 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'
+ }
}
/**
--- a/jdk/src/java.naming/share/classes/com/sun/jndi/ldap/pool/Pool.java Fri Dec 16 09:42:33 2016 -0800
+++ b/jdk/src/java.naming/share/classes/com/sun/jndi/ldap/pool/Pool.java Fri Dec 16 19:15:37 2016 +0000
@@ -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 @@
* and removed.
*/
public void expire(long threshold) {
+ Collection<ConnectionsRef> copy;
synchronized (map) {
- Iterator<ConnectionsRef> iter = map.values().iterator();
- Connections conns;
- while (iter.hasNext()) {
- conns = iter.next().getConnections();
- if (conns.expire(threshold)) {
- d("expire(): removing ", conns);
- iter.remove();
- }
+ copy = new ArrayList<>(map.values());
+ }
+
+ ArrayList<ConnectionsRef> removed = new ArrayList<>();
+ Connections conns;
+ for (ConnectionsRef ref : copy) {
+ conns = ref.getConnections();
+ if (conns.expire(threshold)) {
+ d("expire(): removing ", conns);
+ removed.add(ref);
}
}
+
+ synchronized (map) {
+ map.values().removeAll(removed);
+ }
+
expungeStaleConnections();
}