8277795: ldap connection timeout not honoured under contention

Reviewed-by: dfuchs, aefimov
This commit is contained in:
Rob McKenna 2022-02-04 13:07:03 +00:00
parent 51b53a821b
commit 3d926dd66e
5 changed files with 397 additions and 145 deletions

View File

@ -65,7 +65,23 @@ final class LdapClientFactory implements PooledConnectionFactory {
connTimeout, readTimeout, trace, pcb);
}
public PooledConnection createPooledConnection(PoolCallback pcb, long timeout)
throws NamingException {
return new LdapClient(host, port, socketFactory,
guardedIntegerCast(timeout),
readTimeout, trace, pcb);
}
public String toString() {
return host + ":" + port;
}
private int guardedIntegerCast(long timeout) {
if (timeout < Integer.MIN_VALUE) {
return Integer.MIN_VALUE;
} else if (timeout > Integer.MAX_VALUE) {
return Integer.MAX_VALUE;
}
return (int) timeout;
}
}

View File

@ -27,6 +27,9 @@ package com.sun.jndi.ldap.pool;
import java.util.ArrayList; // JDK 1.2
import java.util.List;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.locks.Condition;
import java.util.concurrent.locks.ReentrantLock;
import java.lang.ref.Reference;
import java.lang.ref.SoftReference;
@ -68,13 +71,19 @@ final class Connections implements PoolCallback {
com.sun.jndi.ldap.LdapPoolManager.trace;
private static final int DEFAULT_SIZE = 10;
final private int initSize;
private final int maxSize;
private final int prefSize;
private final List<ConnectionDesc> conns;
final private PooledConnectionFactory factory;
private boolean closed = false; // Closed for business
private Reference<Object> ref; // maintains reference to id to prevent premature GC
private boolean initialized = false;
private final ReentrantLock lock;
private final Condition connectionsAvailable;
/**
* @param id the identity (connection request) of the connections in the list
* @param initSize the number of connections to create initially
@ -87,105 +96,73 @@ final class Connections implements PoolCallback {
* when one is removed.
* @param factory The factory responsible for creating a connection
*/
Connections(Object id, int initSize, int prefSize, int maxSize,
PooledConnectionFactory factory) throws NamingException {
Connections(Object id, int initSize, int prefSize, int maxSize, PooledConnectionFactory factory,
ReentrantLock lock) throws NamingException {
this.maxSize = maxSize;
this.lock = lock;
this.connectionsAvailable = lock.newCondition();
this.factory = factory;
if (maxSize > 0) {
// prefSize and initSize cannot exceed specified maxSize
this.prefSize = Math.min(prefSize, maxSize);
initSize = Math.min(initSize, maxSize);
this.initSize = Math.min(initSize, maxSize);
} else {
this.prefSize = prefSize;
this.initSize = initSize;
}
conns = new ArrayList<>(maxSize > 0 ? maxSize : DEFAULT_SIZE);
this.conns = new ArrayList<>(maxSize > 0 ? maxSize : DEFAULT_SIZE);
this.initialized = initSize <= 0;
// Maintain soft ref to id so that this Connections' entry in
// Pool doesn't get GC'ed prematurely
ref = new SoftReference<>(id);
this.ref = new SoftReference<>(id);
d("init size=", initSize);
d("max size=", maxSize);
d("preferred size=", prefSize);
}
// Create initial connections
PooledConnection conn;
for (int i = 0; i < initSize; i++) {
conn = factory.createPooledConnection(this);
td("Create ", conn ,factory);
conns.add(new ConnectionDesc(conn)); // Add new idle conn to pool
void waitForAvailableConnection() throws InterruptedNamingException {
try {
d("get(): waiting");
connectionsAvailable.await();
} catch (InterruptedException e) {
throw new InterruptedNamingException(
"Interrupted while waiting for a connection");
}
}
/**
* Retrieves a PooledConnection from this list of connections.
* Use an existing one if one is idle, or create one if the list's
* max size hasn't been reached. If max size has been reached, wait
* for a PooledConnection to be returned, or one to be removed (thus
* not reaching the max size any longer).
*
* @param timeout if > 0, msec to wait until connection is available
* @param factory creates the PooledConnection if one needs to be created
*
* @return A non-null PooledConnection
* @throws NamingException PooledConnection cannot be created, because this
* thread was interrupted while it waited for an available connection,
* or if it timed out while waiting, or the creation of a connection
* resulted in an error.
*/
synchronized PooledConnection get(long timeout,
PooledConnectionFactory factory) throws NamingException {
PooledConnection conn;
long start = (timeout > 0 ? System.currentTimeMillis() : 0);
long waittime = timeout;
d("get(): before");
while ((conn = getOrCreateConnection(factory)) == null) {
if (timeout > 0 && waittime <= 0) {
throw new CommunicationException(
"Timeout exceeded while waiting for a connection: " +
timeout + "ms");
}
try {
d("get(): waiting");
if (waittime > 0) {
wait(waittime); // Wait until one is released or removed
} else {
wait();
}
} catch (InterruptedException e) {
throw new InterruptedNamingException(
void waitForAvailableConnection(long waitTime) throws InterruptedNamingException {
try {
d("get(): waiting");
connectionsAvailable.await(waitTime, TimeUnit.MILLISECONDS);
} catch (InterruptedException e) {
throw new InterruptedNamingException(
"Interrupted while waiting for a connection");
}
// Check whether we timed out
if (timeout > 0) {
long now = System.currentTimeMillis();
waittime = timeout - (now - start);
}
}
d("get(): after");
return conn;
}
/**
* Retrieves an idle connection from this list if one is available.
* If none is available, create a new one if maxSize hasn't been reached.
* If maxSize has been reached, return null.
* Always called from a synchronized method.
*/
private PooledConnection getOrCreateConnection(
PooledConnectionFactory factory) throws NamingException {
PooledConnection getAvailableConnection(long timeout) throws NamingException {
if (!initialized) {
PooledConnection conn = createConnection(factory, timeout);
if (conns.size() >= initSize) {
this.initialized = true;
}
return conn;
}
int size = conns.size(); // Current number of idle/nonidle conns
PooledConnection conn = null;
if (prefSize <= 0 || size >= prefSize) {
// If no prefSize specified, or list size already meets or
// exceeds prefSize, then first look for an idle connection
ConnectionDesc entry;
for (int i = 0; i < size; i++) {
entry = conns.get(i);
for (ConnectionDesc connectionDesc : conns) {
PooledConnection conn;
entry = connectionDesc;
if ((conn = entry.tryUse()) != null) {
d("get(): use ", conn);
td("Use ", conn);
@ -193,17 +170,25 @@ final class Connections implements PoolCallback {
}
}
}
return null;
}
// Check if list size already at maxSize specified
if (maxSize > 0 && size >= maxSize) {
return null; // List size is at limit; cannot create any more
/*
* Creates a new Connection if maxSize hasn't been reached.
* If maxSize has been reached, return null.
* Caller must hold the ReentrantLock.
*/
PooledConnection createConnection(PooledConnectionFactory factory, long timeout)
throws NamingException {
int size = conns.size(); // Current number of idle/non-idle connections
if (maxSize == 0 || size < maxSize) {
PooledConnection conn = factory.createPooledConnection(this, timeout);
td("Create and use ", conn, factory);
conns.add(new ConnectionDesc(conn, true)); // Add new conn to pool
return conn;
}
conn = factory.createPooledConnection(this);
td("Create and use ", conn, factory);
conns.add(new ConnectionDesc(conn, true)); // Add new conn to pool
return conn;
return null;
}
/**
@ -211,43 +196,45 @@ final class Connections implements PoolCallback {
* If the list size is below prefSize, the connection may be reused.
* If the list size exceeds prefSize, then the connection is closed
* and removed from the list.
*
* <p>
* public because implemented as part of PoolCallback.
*/
public synchronized boolean releasePooledConnection(PooledConnection conn) {
ConnectionDesc entry;
int loc = conns.indexOf(entry=new ConnectionDesc(conn));
public boolean releasePooledConnection(PooledConnection conn) {
lock.lock();
try {
ConnectionDesc entry;
int loc = conns.indexOf(entry = new ConnectionDesc(conn));
d("release(): ", conn);
d("release(): ", conn);
if (loc >= 0) {
// Found entry
if (loc >= 0) {
// Found entry
if (closed || (prefSize > 0 && conns.size() > prefSize)) {
// If list size exceeds prefSize, close connection
if (closed || (prefSize > 0 && conns.size() > prefSize)) {
// If list size exceeds prefSize, close connection
d("release(): closing ", conn);
td("Close ", conn);
d("release(): closing ", conn);
td("Close ", conn);
// size must be >= 2 so don't worry about empty list
conns.remove(entry);
conn.closeConnection();
} else {
d("release(): release ", conn);
td("Release ", conn);
// Get ConnectionDesc from list to get correct state info
entry = conns.get(loc);
// Return connection to list, ready for reuse
entry.release();
// size must be >= 2 so don't worry about empty list
conns.remove(entry);
conn.closeConnection();
} else {
d("release(): release ", conn);
td("Release ", conn);
// Get ConnectionDesc from list to get correct state info
entry = conns.get(loc);
// Return connection to list, ready for reuse
entry.release();
}
connectionsAvailable.signalAll();
d("release(): notify");
return true;
}
notifyAll();
d("release(): notify");
return true;
} else {
return false;
} finally {
lock.unlock();
}
return false;
}
/**
@ -257,29 +244,34 @@ final class Connections implements PoolCallback {
* when using the connection and wants it removed from the pool.
*
* @return true if conn removed; false if it was not in pool
*
* <p>
* public because implemented as part of PoolCallback.
*/
public synchronized boolean removePooledConnection(PooledConnection conn) {
if (conns.remove(new ConnectionDesc(conn))) {
d("remove(): ", conn);
public boolean removePooledConnection(PooledConnection conn) {
lock.lock();
try {
if (conns.remove(new ConnectionDesc(conn))) {
d("remove(): ", conn);
notifyAll();
connectionsAvailable.signalAll();
d("remove(): notify");
td("Remove ", conn);
d("remove(): notify");
td("Remove ", conn);
if (conns.isEmpty()) {
// Remove softref to make pool entry eligible for GC.
// Once ref has been removed, it cannot be reinstated.
ref = null;
if (conns.isEmpty()) {
// Remove softref to make pool entry eligible for GC.
// Once ref has been removed, it cannot be reinstated.
ref = null;
}
return true;
} else {
d("remove(): not found ", conn);
}
return true;
} else {
d("remove(): not found ", conn);
return false;
} finally {
lock.unlock();
}
return false;
}
/**
@ -291,8 +283,11 @@ final class Connections implements PoolCallback {
*/
boolean expire(long threshold) {
List<ConnectionDesc> clonedConns;
synchronized(this) {
lock.lock();
try {
clonedConns = new ArrayList<>(conns);
} finally {
lock.unlock();
}
List<ConnectionDesc> expired = new ArrayList<>();
@ -304,12 +299,15 @@ final class Connections implements PoolCallback {
}
}
synchronized (this) {
lock.lock();
try {
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'
} finally {
lock.unlock();
}
}
@ -355,6 +353,29 @@ final class Connections implements PoolCallback {
+ "; idle=" + idle + "; expired=" + expired;
}
boolean grabLock(long timeout) throws InterruptedNamingException {
final long start = System.nanoTime();
long current = start;
long remaining = timeout;
boolean locked = false;
while (!locked && remaining > 0) {
try {
locked = lock.tryLock(remaining, TimeUnit.MILLISECONDS);
remaining -= TimeUnit.NANOSECONDS.toMillis(current - start);
} catch (InterruptedException ignore) {
throw new InterruptedNamingException(
"Interrupted while waiting for the connection pool lock");
}
current = System.nanoTime();
remaining -= TimeUnit.NANOSECONDS.toMillis(current - start);
}
return locked;
}
void unlock() {
lock.unlock();
}
private void d(String msg, Object o1) {
if (debug) {
d(msg + o1);

View File

@ -31,10 +31,13 @@ import java.util.WeakHashMap;
import java.util.Collection;
import java.util.Collections;
import java.util.LinkedList;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.locks.ReentrantLock;
import java.io.PrintStream;
import java.lang.ref.Reference;
import java.lang.ref.ReferenceQueue;
import javax.naming.CommunicationException;
import javax.naming.NamingException;
/**
@ -117,45 +120,109 @@ public final class Pool {
public PooledConnection getPooledConnection(Object id, long timeout,
PooledConnectionFactory factory) throws NamingException {
final long start = System.nanoTime();
long remaining = timeout;
d("get(): ", id);
if (debug) {
synchronized (map) {
d("size: ", map.size());
}
remaining = checkRemaining(start, remaining);
}
expungeStaleConnections();
Connections conns;
synchronized (map) {
conns = getConnections(id);
if (conns == null) {
d("get(): creating new connections list for ", id);
Connections conns = getOrCreateConnections(factory, id);
d("get(): size after: ", map.size());
remaining = checkRemaining(start, remaining);
// No connections for this id so create a new list
conns = new Connections(id, initSize, prefSize, maxSize,
factory);
ConnectionsRef connsRef = new ConnectionsRef(conns);
map.put(id, connsRef);
// Create a weak reference to ConnectionsRef
Reference<ConnectionsRef> weakRef =
new ConnectionsWeakRef(connsRef, queue);
// Keep the weak reference through the element of a linked list
weakRefs.add(weakRef);
}
d("get(): size after: ", map.size());
if (!conns.grabLock(remaining)) {
throw new CommunicationException("Timed out waiting for lock");
}
return conns.get(timeout, factory); // get one connection from list
try {
remaining = checkRemaining(start, remaining);
PooledConnection conn = null;
while (remaining > 0 && conn == null) {
conn = getOrCreatePooledConnection(factory, conns, start, remaining);
// don't loop if the timeout has expired
remaining = checkRemaining(start, timeout);
}
return conn;
} finally {
conns.unlock();
}
}
private Connections getConnections(Object id) {
ConnectionsRef ref = map.get(id);
return (ref != null) ? ref.getConnections() : null;
private Connections getOrCreateConnections(PooledConnectionFactory factory, Object id)
throws NamingException {
Connections conns;
synchronized (map) {
ConnectionsRef ref = map.get(id);
if (ref != null) {
return ref.getConnections();
}
d("get(): creating new connections list for ", id);
// No connections for this id so create a new list
conns = new Connections(id, initSize, prefSize, maxSize,
factory, new ReentrantLock());
ConnectionsRef connsRef = new ConnectionsRef(conns);
map.put(id, connsRef);
// Create a weak reference to ConnectionsRef
Reference<ConnectionsRef> weakRef = new ConnectionsWeakRef(connsRef, queue);
// Keep the weak reference through the element of a linked list
weakRefs.add(weakRef);
}
return conns;
}
private PooledConnection getOrCreatePooledConnection(
PooledConnectionFactory factory, Connections conns, long start, long timeout)
throws NamingException {
PooledConnection conn = conns.getAvailableConnection(timeout);
if (conn != null) {
return conn;
}
// no available cached connection
// check if list size already at maxSize before creating a new one
conn = conns.createConnection(factory, timeout);
if (conn != null) {
return conn;
}
// max number of connections already created,
// try waiting around for one to become available
if (timeout <= 0) {
conns.waitForAvailableConnection();
} else {
long remaining = checkRemaining(start, timeout);
conns.waitForAvailableConnection(remaining);
}
return null;
}
// Check whether we timed out
private long checkRemaining(long start, long timeout) throws CommunicationException {
if (timeout > 0) {
long current = System.nanoTime();
long remaining = timeout - TimeUnit.NANOSECONDS.toMillis(current - start);
if (remaining <= 0) {
throw new CommunicationException(
"Timeout exceeded while waiting for a connection: " +
timeout + "ms");
}
return remaining;
}
return Long.MAX_VALUE;
}
/**
* Goes through the connections in this Pool and expires ones that
* have been idle before 'threshold'. An expired connection is closed

View File

@ -44,4 +44,13 @@ public interface PooledConnectionFactory {
*/
public abstract PooledConnection createPooledConnection(PoolCallback pcb)
throws NamingException;
/**
* Creates a pooled connection.
* @param pcb callback responsible for removing and releasing the pooled
* connection from the pool.
* @param timeout the connection timeout
*/
public abstract PooledConnection createPooledConnection(PoolCallback pcb, long timeout)
throws NamingException;
};

View File

@ -0,0 +1,139 @@
/*
* Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
/*
* @test
* @bug 8277795
* @summary Multi-threaded client timeout tests for ldap pool
* @library /test/lib
* lib/
* @run testng/othervm LdapPoolTimeoutTest
*/
import org.testng.Assert;
import org.testng.annotations.Test;
import java.io.IOException;
import javax.naming.Context;
import javax.naming.NamingException;
import javax.naming.directory.InitialDirContext;
import java.util.ArrayList;
import java.util.Hashtable;
import java.util.List;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.TimeoutException;
import java.util.concurrent.TimeUnit;
import static jdk.test.lib.Utils.adjustTimeout;
import static org.testng.Assert.assertTrue;
import static org.testng.Assert.expectThrows;
public class LdapPoolTimeoutTest {
/*
* Practical representation of an infinite timeout.
*/
private static final long INFINITY_MILLIS = adjustTimeout(20_000);
/*
* The acceptable variation in timeout measurements.
*/
private static final long TOLERANCE = adjustTimeout( 3_500);
private static final long CONNECT_MILLIS = adjustTimeout( 3_000);
private static final long READ_MILLIS = adjustTimeout(10_000);
static {
// a series of checks to make sure this timeouts configuration is
// consistent and the timeouts do not overlap
assert (TOLERANCE >= 0);
// context creation
assert (2 * CONNECT_MILLIS + TOLERANCE < READ_MILLIS);
// context creation immediately followed by search
assert (2 * CONNECT_MILLIS + READ_MILLIS + TOLERANCE < INFINITY_MILLIS);
}
@Test
public void test() throws Exception {
List<Future<?>> futures = new ArrayList<>();
ExecutorService executorService = Executors.newCachedThreadPool();
Hashtable<Object, Object> env = new Hashtable<>();
env.put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.ldap.LdapCtxFactory");
env.put("com.sun.jndi.ldap.read.timeout", String.valueOf(READ_MILLIS));
env.put("com.sun.jndi.ldap.connect.timeout", String.valueOf(CONNECT_MILLIS));
env.put("com.sun.jndi.ldap.connect.pool", "true");
env.put(Context.PROVIDER_URL, "ldap://example.com:1234");
try {
futures.add(executorService.submit(() -> { attemptConnect(env); return null; }));
futures.add(executorService.submit(() -> { attemptConnect(env); return null; }));
futures.add(executorService.submit(() -> { attemptConnect(env); return null; }));
futures.add(executorService.submit(() -> { attemptConnect(env); return null; }));
futures.add(executorService.submit(() -> { attemptConnect(env); return null; }));
futures.add(executorService.submit(() -> { attemptConnect(env); return null; }));
futures.add(executorService.submit(() -> { attemptConnect(env); return null; }));
futures.add(executorService.submit(() -> { attemptConnect(env); return null; }));
} finally {
executorService.shutdown();
}
int failedCount = 0;
for (var f : futures) {
try {
f.get();
} catch (ExecutionException e) {
failedCount++;
e.getCause().printStackTrace(System.out);
}
}
if (failedCount > 0)
throw new RuntimeException(failedCount + " (sub)tests failed");
}
private static void attemptConnect(Hashtable<Object, Object> env) throws Exception {
try {
LdapTimeoutTest.assertCompletion(CONNECT_MILLIS - 1000,
2 * CONNECT_MILLIS + TOLERANCE,
() -> new InitialDirContext(env));
} catch (RuntimeException e) {
String msg = e.getCause() == null ? e.getMessage() : e.getCause().getMessage();
System.err.println("MSG RTE: " + msg);
// assertCompletion may wrap a CommunicationException in an RTE
assertTrue(msg != null && msg.contains("Network is unreachable"));
} catch (NamingException ex) {
String msg = ex.getCause() == null ? ex.getMessage() : ex.getCause().getMessage();
System.err.println("MSG: " + msg);
assertTrue(msg != null &&
(msg.contains("Network is unreachable")
|| msg.contains("Timed out waiting for lock")
|| msg.contains("Connect timed out")
|| msg.contains("Timeout exceeded while waiting for a connection")));
} catch (Throwable t) {
throw new RuntimeException(t);
}
}
}