RFR: 8313657 : com.sun.jndi.ldap.Connection.cleanup does not close connections on SocketTimeoutErrors

[Aleksei Efimov]

On Thu, 3 Aug 2023 17:32:43 GMT, Weibing Xiao <duke at openjdk.org> wrote:

> com.sun.jndi.ldap.Connection::leanup does not close the underlying socket if the is an IOException generation when the output stream was flushing the buffer.
> 
> Please refer to the bug https://bugs.openjdk.org/browse/JDK-8313657.

src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 652:

> 650:                     } catch (IOException ie) {
> 651:                         if (debug)
> 652:                             System.err.println("Connection: problem closing socket: " + ie);

How about combining the debug statement here with the one below and just print the socket isClosed state, something like

if (debug) {
    System.err.println("Connection: problem cleaning-up the connection: " + ie);
    System.err.println("Socket isClosed: " + sock.isClosed());
}

The text was changed here since the exception can also be thrown by `unpauseReader`

src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 664:

> 662:                         } catch (IOException ioe) {
> 663:                             if (debug)
> 664:                                 System.err.println("Connection::cleanup problem: " + ioe);

Maybe change the message here to highlight the fact that it's the 2nd attemp to clean-up the connection

test/jdk/javax/naming/InitialContext/SocketCloseTest.java line 28:

> 26: import javax.naming.directory.InitialDirContext;
> 27: import javax.net.SocketFactory;
> 28: import java.io.*;

Can we please use a set of class imports instead of package import here:

import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;

test/jdk/javax/naming/InitialContext/SocketCloseTest.java line 44:

> 42:  */
> 43: 
> 44: public class SocketCloseTest {

Can we change the test location from `test/javax/naming/InitialContext` to an LDAP-implementation specific folder - `test/jdk/com/sun/jndi/ldap`

test/jdk/javax/naming/InitialContext/SocketCloseTest.java line 53:

> 51:     };
> 52:     private static final int SEARCH_SIZE = 87;
> 53:     private static final byte[] SEARCH_RESPONSE = new byte[]{

`SEARCH_RESPONSE`, `SEARCH_SIZE` and `BIND_SIZE` are not used in the test. If they're not needed they can be removed.

test/jdk/javax/naming/InitialContext/SocketCloseTest.java line 71:

> 69: 
> 70:         props.put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.ldap.LdapCtxFactory");
> 71:         props.put(Context.PROVIDER_URL, "ldap://localhost:1389/o=example");

For future test maintainers - can we please clarify that the hostname and port do not matter here because the provided custom socket factory doesn't establish a connection to the specified provider URL

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/15143#discussion_r1284631833
PR Review Comment: https://git.openjdk.org/jdk/pull/15143#discussion_r1284633364
PR Review Comment: https://git.openjdk.org/jdk/pull/15143#discussion_r1284569423
PR Review Comment: https://git.openjdk.org/jdk/pull/15143#discussion_r1284579620
PR Review Comment: https://git.openjdk.org/jdk/pull/15143#discussion_r1284582686
PR Review Comment: https://git.openjdk.org/jdk/pull/15143#discussion_r1284587752