On Thu, 3 Aug 2023 17:32:43 GMT, Weibing Xiao <d...@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
