On Fri, 26 Nov 2021 10:50:57 GMT, Daniel Fuchs <dfu...@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