On Thu, 24 Aug 2023 00:10:54 GMT, Weibing Xiao <[email protected]> wrote:
>> Please refer to JDK-8314063.
>>
>> The failure scenario is due to the setting of connection timeout. It is
>> either too small or not an optimal value for the system. When the client
>> tries to connect to the server with LDAPs protocol. It requires the
>> handshake after the socket is created and connected, but it fails due to
>> connection timeout and leaves the socket open. It is not closed properly due
>> to the exception handling in the JDK code.
>>
>> The change is adding a try/catch block and closing the socket in the catch
>> block, and the format of the code got changed consequently.
>
> Weibing Xiao has updated the pull request incrementally with one additional
> commit since the last revision:
>
> refactoring the code according to the comment
The last change reformatted several lines unrelated to this change - I've
commented on a few occurrences. Could you please keep only changes related to
the scope of this PR?
src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 62:
> 60:
> 61: /**
> 62: * A thread that creates a connection to an LDAP server.
The formatting changes here is not related to the code changes under review in
this PR. Could you please revert them?
src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 128:
> 126:
> 127: public final String host; // used by LdapClient for generating
> exception messages
> 128: // used by StartTlsResponse when creating an SSL socket
Same - reformatting of unrelated code
src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 133:
> 131: // used by StartTlsResponse when creating an
> SSL socket
> 132: public final int port; // used by LdapClient for generating
> exception messages
> 133: // used by StartTlsResponse when creating an
> SSL socket
Same - reformatting of unrelated code
src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 246:
> 244:
> 245: CommunicationException ce =
> 246: new CommunicationException(host + ":" + port);
Same - reformatting of unrelated code
src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 254:
> 252: // Also catches all IO errors generated by socket creation.
> 253: CommunicationException ce =
> 254: new CommunicationException(host + ":" + port);
Same - reformatting of unrelated code
src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 354:
> 352: private void initialSSLHandshake(Socket socket, int connectTimeout)
> throws Exception {
> 353:
> 354: if (socket instanceof SSLSocket) {
instanceof pattern matching can be used here:
if (socket instanceof SSLSocket sslSocket) {
src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 387:
> 385:
> 386: LdapRequest writeRequest(BerEncoder ber, int msgId,
> 387: boolean pauseAfterReceipt) throws IOException {
Reformatting of unrelated code
src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 391:
> 389:
> 390: LdapRequest writeRequest(BerEncoder ber, int msgId,
> 391: boolean pauseAfterReceipt, int
> replyQueueCapacity) throws IOException {
Reformatting of unrelated code
src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 394:
> 392:
> 393: LdapRequest req =
> 394: new LdapRequest(msgId, pauseAfterReceipt,
> replyQueueCapacity);
Reformatting of unrelated code
src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 435:
> 433: if (sock == null) {
> 434: throw new ServiceUnavailableException(host + ":" + port +
> 435: "; socket closed");
Reformatting of unrelated code
src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 619:
> 617:
> 618: /**
> 619: * @param reqCtls Possibly null request controls that accompanies the
Reformatting of unrelated code
-------------
PR Review: https://git.openjdk.org/jdk/pull/15294#pullrequestreview-1593589706
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1304321768
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1304322767
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1304322965
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1304324031
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1304324165
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1304326820
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1304329352
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1304330163
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1304330374
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1304330800
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1304331645
