RFR: JDK-8277795: ldap connection timeout not honoured under contention
Daniel Fuchs
dfuchs at openjdk.java.net
Fri Nov 26 10:54:03 UTC 2021
On Thu, 25 Nov 2021 23:54:18 GMT, Rob McKenna <robm at openjdk.org> wrote:
> This fix attemps to resolve an issue where threads can stack up on each other while waiting to get a connection from the ldap pool to an unreachable server. It does this by having each thread start a countdown prior to holding the pools' lock. (which has been changed to a ReentrantLock) Once the lock has been grabbed, the timeout is adjusted to take the waiting time into account and the process of getting a connection from the pool or creating a new one commences.
>
> Note: this fix also changes the meaning of the connection pools initSize somewhat. In a situation where we have a large initSize and a small timeout the first thread could actually exhaust the timeout before creating all of its initial connections. Instead this fix simply creates a single connection per pool-connection-request. It continues to do so for subsequent requests regardless of whether an existing unused connection is available in the pool until initSize is exhausted. As such it may require a CSR.
What testing is there for this fix?
src/java.naming/share/classes/com/sun/jndi/ldap/LdapClientFactory.java line 71:
> 69: throws NamingException {
> 70: return new LdapClient(host, port, socketFactory,
> 71: (int)timeout, readTimeout, trace, pcb);
A blunt cast from `long` to `int` is a bit worrying as it could lead to positive values becoming negative, unless you have checks in place in the calling code that will ensure that the long value is never > Integer.MAX_VALUE? And it could also result in a large value becoming a small positive value.
I'd suggest to remove the inconsistency one way or the other - or add an explicit check to make it obvious that this case cannot happen.
src/java.naming/share/classes/com/sun/jndi/ldap/pool/Pool.java line 141:
> 139:
> 140: if (!conns.grabLock(remaining)) {
> 141: throw new NamingException("Timed out waiting for lock");
Is this the appropriate exception? I see in `checkRemaining`:
throw new CommunicationException(
"Timeout exceeded while waiting for a connection: " +
timeout + "ms");
-------------
PR: https://git.openjdk.java.net/jdk/pull/6568
More information about the core-libs-dev
mailing list