RFR: 8357708: com.sun.jndi.ldap.Connection ignores queued LDAP replies if Connection is subsequently closed [v3]

[Jaikiran Pai]

On Wed, 6 Aug 2025 05:15:46 GMT, Shaojin Wen <swen at openjdk.org> wrote:

>> Jaikiran Pai has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
>> 
>>  - merge latest from master branch
>>  - merge latest from master branch
>>  - add test
>>  - 8357708: com.sun.jndi.ldap.Connection ignores queued LDAP replies if Connection is subsequently closed
>
> src/java.naming/share/classes/com/sun/jndi/ldap/LdapRequest.java line 87:
> 
>> 85:                 ber.parseSeq(null);
>> 86:                 ber.parseInt();
>> 87:                 isLdapResResult = (ber.peekByte() == LdapClient.LDAP_REP_RESULT);
> 
> Other boolean type variables in the LdapRequest class do not have the `is` prefix. The local variable `isLdapResResult` here should also use the same style. We should not use two styles in one class.

Calling it `ldapResResult` instead of `isLdapResResult` isn't appropriate here, so I've let this stay in its current form.

> src/java.naming/share/classes/com/sun/jndi/ldap/LdapRequest.java line 147:
> 
>> 145:         }
>> 146:         if (result == CANCELLED_MARKER) {
>> 147:             throw new CommunicationException("Request: " + msgId +
> 
> The current LdapRequest class should use a unified style to construct exception information, either using string concatenation or String.format. A class should not mix the two styles.

Hello Shaojin, these are pre-existing messages. I have however updated the PR to use a uniform style.

> test/jdk/javax/naming/ldap/LdapClientConnTest.java line 110:
> 
>> 108:         try (final ExecutorService executor = Executors.newCachedThreadPool()) {
>> 109:             for (int i = 1; i <= numTasks; i++) {
>> 110:                 final String taskName = "task-" + i;
> 
> Suggestion:
> 
>                 String taskName = "task-" + i;
> 
> Variables used only in the current block do not need to be final

This a more a personal preference and since this is a new test file, I let it stay in this form. If there's a strong preference to remove it, I'll do so.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25449#discussion_r2332405116
PR Review Comment: https://git.openjdk.org/jdk/pull/25449#discussion_r2332402149
PR Review Comment: https://git.openjdk.org/jdk/pull/25449#discussion_r2332413316