On Fri, 12 Sep 2025 06:36:31 GMT, Jaikiran Pai <[email protected]> wrote:
>> Can I please get a review of this change which proposes to address the issue >> noted in https://bugs.openjdk.org/browse/JDK-8357708? >> >> As noted in the issue, the current code in >> `com.sun.jndi.ldap.Connection.readReply()` is susceptible to throwing a >> `ServiceUnavailableException` even when the LDAP replies have already been >> received and queued for processing. The JBS issue has additional details >> about how that can happen. >> >> The commit in this PR simplifies the code in `com.sun.jndi.ldap.LdapRequest` >> to make sure it always gives out the replies that have been queued when the >> `LdapRequest.getReplyBer()` gets invoked. One of those queued values could >> be markers for a cancelled or closed request. In that case, the >> `getReplyBer()`, like previously, continues to throw the right exception. >> With this change, the call to `replies.take()` or `replies.poll()` (with an >> infinite timeout) is no longer expected to hang forever, if the `Connection` >> is closed (or the request cancelled). This then allows us to remove the >> connection closure (`sock == null`) check in `Connection.readReply()`. >> >> A new jtreg test has been introduced to reproduce this issue and verify the >> fix. The test reproduces this issue consistently when the source fix isn't >> present. With the fix present, even after several thousand runs of this >> test, the issue no longer reproduces. >> >> tier1, tier2 and tier3 tests continue to pass with this change. I've marked >> the fix version of this issue for 26 and I don't plan to push this for 25. > > 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 eight additional > commits since the last revision: > > - Daniel's review - make the test othervm > - merge latest from master branch > - remove format() call in exception message creation > - merge latest from master branch > - 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 Hello Jaikiran, The fix and its logic looks correct to me. I have a couple of suggestions regarding the new test: 1. The test can be modified to remove the dependency on the internal `LdapCtx` class. Something like: @@ -43,11 +43,14 @@ import java.util.concurrent.Future; import java.util.concurrent.atomic.AtomicInteger; -import com.sun.jndi.ldap.LdapCtx; import jdk.test.lib.net.URIBuilder; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; + +import javax.naming.Context; +import javax.naming.InitialContext; + import static java.nio.charset.StandardCharsets.US_ASCII; import static org.junit.jupiter.api.Assertions.fail; @@ -56,7 +59,6 @@ * @bug 8357708 * @summary verify that com.sun.jndi.ldap.Connection does not ignore the LDAP replies * that were received before the Connection was closed. - * @modules java.naming/com.sun.jndi.ldap * @library /test/lib * @build jdk.test.lib.net.URIBuilder * @run junit/othervm LdapClientConnTest @@ -381,17 +383,22 @@ @Override public Void call() throws Exception { - LdapCtx ldapCtx = null; + Context ldapCtx = null; try { final InetSocketAddress serverAddr = server.getAddress(); final Hashtable<String, String> envProps = new Hashtable<>(); // explicitly set LDAP version to 3 to prevent LDAP BIND requests // during LdapCtx instantiation envProps.put("java.naming.ldap.version", "3"); - ldapCtx = new LdapCtx("", - serverAddr.getAddress().getHostAddress(), - serverAddr.getPort(), - envProps, false); + envProps.put(Context.INITIAL_CONTEXT_FACTORY, + "com.sun.jndi.ldap.LdapCtxFactory"); + String providerUrl = URIBuilder.newBuilder() + .scheme("ldap") + .host(serverAddr.getAddress()) + .port(serverAddr.getPort()) + .build().toString(); + envProps.put(Context.PROVIDER_URL, providerUrl); + ldapCtx = new InitialContext(envProps); final String name = SEARCH_REQ_DN_PREFIX + taskName + SEARCH_REQ_DN_SUFFIX; // trigger the LDAP SEARCH requests through the lookup call. we are not // interested in the returned value and are merely interested in a normal 2. Maybe move the new test to the `test/jdk/com/sun/jndi/ldap` folder where other tests for internal LDAP classes live. 3. I like the dynamic approach on generating and parsing the LDAP messages. For future tests in JNDI/LDAP area such approach could be beneficial. Therefore, a suggestion - move `private static final byte BER_TYPE_*` constants to the `test/jdk/com/sun/jndi/ldap/lib/LDAPTestUtils.java`. ------------- PR Comment: https://git.openjdk.org/jdk/pull/25449#issuecomment-3285576752
