On Thu, 14 Mar 2024 21:59:54 GMT, Christoph Langer <clan...@openjdk.org> wrote:
>> During analysing a customer case I figured out that we have an inconsistency >> between documentation and actual behavior in class >> com.sun.jndi.ldap.Connection. The [method documentation of >> com.sun.jndi.ldap.Connection::createSocket](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L281) >> states: "If a timeout is supplied but unconnected sockets are not supported >> then the timeout is ignored and a connected socket is created." >> >> This, however does not happen. If a SocketFactory would not support >> unconnected sockets, it would likely throw a SocketException in >> [SocketFactory::createSocket()](https://github.com/openjdk/jdk/blob/6303c0e7136436a2d3cb6043b88edf788c0067cc/src/java.base/share/classes/javax/net/SocketFactory.java#L123). >> And since [the >> code](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L336) >> does not check for this behavior, a connection with timeout value through a >> SocketFactory that does not support unconnected sockets would simply fail >> with an IOException. >> >> So we should either make the code adhere to what is documented or adapt the >> documentation to the actual behavior. >> >> I hereby try to fix the connect coding. Alternatively, we could also adapt >> the description - I have no strong opinion. What do the experts suggest? > > Christoph Langer has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 12 additional > commits since the last revision: > > - Update module-info text > - Merge branch 'master' into JDK-8325579 > - Indentation > - Merge branch 'master' into JDK-8325579 > - Review feedback > - Rename back to LdapSSLHandshakeFailureTest to ease reviewing > - Merge branch 'master' into JDK-8325579 > - Typo > - Merge branch 'master' into JDK-8325579 > - Rename test and refine comment > - ... and 2 more: https://git.openjdk.org/jdk/compare/bc0070cb...10271159 The latest version looks good to me with a couple minor comments and suggestions. src/java.naming/share/classes/module-info.java line 42: > 40: * <br>The value of this environment property specifies the fully > 41: * qualified class name of the socket factory used by the LDAP > provider. > 42: * This class must implement the javax.net.SocketFactory abstract > class You could add a link to the `SocketFactory` class here: Suggestion: * This class must implement the {@link javax.net.SocketFactory} abstract class test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 164: > 162: if > (CustomSocketFactory.customSocket.closeMethodCalledCount() <= 0) { > 163: System.err.println("Custom Socket was not > closed."); > 164: System.exit(-1); Can we update test not to use `System.exit` and replace it with throwing `new RuntimeException`, something like: Suggestion: throw new RuntimeException("Custom Socket was not closed"); test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 177: > 175: ctx.close(); > 176: if (!checkSocketClosed(sock)) { > 177: System.exit(-1); Can be replaced with: Suggestion: throw new RuntimeException("Socket isn't closed"); test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 184: > 182: e.printStackTrace(); > 183: System.exit(-1); > 184: } For simplification and `System.exit` removal purposes this catch block can be removed with addition of `throws Exception` clause to the `main` method. Suggestion: } ------------- PR Review: https://git.openjdk.org/jdk/pull/17797#pullrequestreview-1946713010 PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1530674406 PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1530686689 PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1530696579 PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1530693641
