Re: RFR: 8314063 : The socket is not closed in Connection::createSocket when the handshake failed for LDAP connection [v7]
> 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: refactor the code and test cases - Changes: - all: https://git.openjdk.org/jdk/pull/15294/files - new: https://git.openjdk.org/jdk/pull/15294/files/ac72c3d8..e9e0497f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15294&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15294&range=05-06 Stats: 121 lines in 3 files changed: 48 ins; 36 del; 37 mod Patch: https://git.openjdk.org/jdk/pull/15294.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15294/head:pull/15294 PR: https://git.openjdk.org/jdk/pull/15294
Re: RFR: 8314063 : The socket is not closed in Connection::createSocket when the handshake failed for LDAP connection [v7]
On Sat, 19 Aug 2023 02:15:06 GMT, Weibing Xiao 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:
>
> refactor the code and test cases
test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 77:
> 75: // start the test server first.
> 76: boolean serverSlowDown = false;
> 77: if(args.length ==2 ) {
Suggestion:
if (args.length ==2 ) {
test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 80:
> 78: serverSlowDown = Boolean.valueOf(args[1]);
> 79: } else {
> 80: if(args.length ==1 ) {
Suggestion:
if (args.length ==1) {
test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 98:
> 96: }
> 97: env.put("java.naming.ldap.version", "3");
> 98: if (args.length == 2 ) {
Suggestion:
if (args.length == 2) {
test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 99:
> 97: env.put("java.naming.ldap.version", "3");
> 98: if (args.length == 2 ) {
> 99: if( args[0].contains("LdapSSLHandshakeFailureTest")) {
Suggestion:
if (args[0].contains("LdapSSLHandshakeFailureTest")) {
test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 122:
> 120: }
> 121: } finally {
> 122: if(ctx != null)
Suggestion:
if (ctx != null)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1299160346
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1299160377
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1299160495
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1299160402
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1299160439
Re: RFR: 8314063 : The socket is not closed in Connection::createSocket when the handshake failed for LDAP connection [v7]
On Sat, 19 Aug 2023 02:15:06 GMT, Weibing Xiao 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:
>
> refactor the code and test cases
test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 84:
> 82: }
> 83: }
> 84: TestServer server = new TestServer( serverSlowDown );
The server instance can be used in try-with-resouces:
try (TestServer server = new TestServer(serverSlowDown)) {
server.start();
env.put(Context.PROVIDER_URL, URIBuilder.newBuilder()
.scheme("ldaps")
.loopback()
.port(server.getPortNumber())
.buildUnchecked().toString());
test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 94:
> 92: .buildUnchecked().toString());
> 93: if (args.length == 2 &&
> 94: args[0].contains("LdapSSLHandshakeFailureTest")) {
This `arg[0]` check is done twice in the test - it can be save to a boolean
variable:
boolean hasCustomSocketFactory = args[0]
.equals("LdapSSLHandshakeFailureTest$CustomSocketFactory");
and then used to the checks
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1300088395
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1300086412
Re: RFR: 8314063 : The socket is not closed in Connection::createSocket when the handshake failed for LDAP connection [v7]
On Sat, 19 Aug 2023 02:15:06 GMT, Weibing Xiao 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:
>
> refactor the code and test cases
test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 69:
> 67: public class LdapSSLHandshakeFailureTest {
> 68: private static String SOCKET_CLOSED_MSG = "The socket has been
> closed.";
> 69: private static String SOCKET_NOT_CLOSED_MSG = "The socket was not
> closed.";
`SOCKET_NOT_CLOSED_MSG` - not used and can be removed
test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 76:
> 74: setKeyStore();
> 75: // start the test server first.
> 76: boolean serverSlowDown = false;
`false` initializer is redundant here
test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 178:
> 176: }
> 177:
> 178: private static void setKeyStore() throws Exception {
`Exception` is not thrown by this method and can be removed
test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 211:
> 209: @Override
> 210: public void run() {
> 211: try (Socket socket = serverSocket.accept(); InputStream in =
> socket.getInputStream();
formatting: this can be made shorter by putting `InputStream in =
socket.getInputStream();` on a separate line
test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 216:
> 214: Thread.sleep(5000);
> 215: }
> 216: byte[] bindResponse = {0x30, 0x0C, 0x02, 0x01, 0x01,
> 0x61, 0x07, 0x0A, 0x01, 0x00, 0x04, 0x00, 0x04, 0x00};
formatting: can be splitted in two lines to make it shorter
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1300093748
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1300092781
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1300095645
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1300099068
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1300100447
Re: RFR: 8314063 : The socket is not closed in Connection::createSocket when the handshake failed for LDAP connection [v7]
On Sat, 19 Aug 2023 02:15:06 GMT, Weibing Xiao 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: > > refactor the code and test cases 1) Reformat the test code 2) Add one more comment in Connection.java 3) Updated the test code base on the review comment - PR Comment: https://git.openjdk.org/jdk/pull/15294#issuecomment-1686489715
