On Wed, 20 Mar 2024 21:19:38 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 14 additional > commits since the last revision: > > - Review suggestions Aleksei > - Merge branch 'master' into JDK-8325579 > - 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 > - ... and 4 more: https://git.openjdk.org/jdk/compare/02859d70...8fdc039c The latest changes look good to me. Thank you for addressing all the comments. I've tested the proposed change with JNDI regression tests, including the modified one, and there are no new issues discovered. ------------- Marked as reviewed by aefimov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17797#pullrequestreview-1952901178
