On Tue, 15 Aug 2023 17:30:54 GMT, Weibing Xiao <d...@openjdk.org> 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.
Hello Weibing,
The approach taken here seems resonable to me. Please find my comments on the
fix and the test below:
src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 284:
> 282:
> 283: Socket socket = null;
> 284: try {
This method is hard to read. Readability could be improved by adding/splitting
it into two new methods:
- one for creating a socket with a use of specified socket factory
- another one for creating a socket with `new Socket`
src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 369:
> 367: }
> 368: }
> 369: } catch (Exception e) {
The code wrapped in this try-catch block can throw unchecked exceptions, for
example `SecurityException` thrown by `Socket.connect`. For such cases the
newly created socket remain open.
src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 372:
> 370: // 8314063 the socket is not closed after the failure of
> handshake
> 371: if (socket != null && !socket.isClosed()) {
> 372: socket.close();
The original exception can be lost if `socket.close()` fails with `IOException`
here
test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 64:
> 62: setKeyStore();
> 63: // start the test server first.
> 64: TestServer server = new TestServer();
Since the `TestServer` implements `AutoClosable`, can we take advantage of that
and of try-with-resources to close the server socket. One possibility is to
move test server, url and environment table creation into try-catch block, and
convert it to try-with-resources (`TestServer`).
test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 66:
> 64: TestServer server = new TestServer();
> 65: server.start();
> 66: url = "ldaps://localhost:" + server.getPortNumber();
The `URIBuilder` utility class from a test library can be used to construct a
test server url here:
URI uri = URIBuilder.newBuilder()
.scheme("ldaps")
.loopback()
.port(server.getPortNumber())
.build();
test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 78:
> 76: try {
> 77: LdapContext ctx = new InitialLdapContext(env, null);
> 78: ctx.close();
The context can be closed in finally block
test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 151:
> 149: private volatile boolean exceptionThrown;
> 150:
> 151: TestServer() throws IOException {
`IOException` is not thrown by this constructor - can be removed
test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 154:
> 152: try {
> 153: SSLServerSocketFactory socketFactory =
> (SSLServerSocketFactory) SSLServerSocketFactory.getDefault();
> 154: serverSocket = socketFactory.createServerSocket(0);
The test server can be bound to the loopback address here and `URIBuilder` test
library class can be used to construct the `URL` as mentioned above:
`socketFactory.createServerSocket(0, 0, InetAddress.getLoopbackAddress());`
test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 166:
> 164: }
> 165:
> 166: public boolean isExceptionThrown() {
`isExceptionThrown` method is not used by the test
test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 172:
> 170: @Override
> 171: public void run() {
> 172: try (Socket socket = serverSocket.accept()) {
Can be beautified a bit by putting socket, IS and OS into one
try-with-resources statement. Something like:
try (Socket socket = serverSocket.accept();
InputStream in = socket.getInputStream();
OutputStream out = socket.getOutputStream()) {
-------------
PR Review: https://git.openjdk.org/jdk/pull/15294#pullrequestreview-1581104126
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1296223224
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1296269582
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1296224057
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1296226004
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1296228008
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1296229176
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1296230459
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1296232557
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1296235296
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1296236949
