[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-core-libs-dev
Subject:    Re: RFR: JDK-8277795: ldap connection timeout not honoured under contention [v2]
From:       Rob McKenna <robm () openjdk ! java ! net>
Date:       2021-11-27 0:07:51
Message-ID: _GyBP0OGoeiT2LJf6FE6HRhoXxEvmblMbScoUuVe22Q=.8372fbc4-2928-4783-819e-711b1a5d014f () github ! com
[Download RAW message or body]

On Fri, 26 Nov 2021 10:50:57 GMT, Daniel Fuchs <dfuchs@openjdk.org> wrote:

> What testing is there for this fix?

I've just added a test modelled on LdapTimeoutTest.java. (with some whitespace issues \
which I'm about to 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.

Good point. I've added a check to the case. (this actually comes from \
LdapPoolManager.getLdapClient which takes an int for the connection timeout \
parameter, but it makes sense to be careful)

> 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");

I've changed this to a CommuncationException.

-------------

PR: https://git.openjdk.java.net/jdk/pull/6568


[prev in list] [next in list] [prev in thread] [next in thread]