On Fri, 26 Nov 2021 10:50:57 GMT, Daniel Fuchs <[email protected]> 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
