On Thu, 15 Feb 2024 15:11:15 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 pull request now contains two commits: > > - Enhance test > - JDK-8325579 So, I've updated the PR. Sorry for force-pushing, hope you didn't review the code in detail yet. The new version updates module-info as requested. I also enhanced the test `LdapSSLHandshakeFailureTest.java` and added variants that exercise a Socket Factory which does not support unconnected sockets. Two of the test variants would fail with the current JDK. ------------- PR Comment: https://git.openjdk.org/jdk/pull/17797#issuecomment-1946315957
