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