On Fri, 16 Feb 2024 10:27:11 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 five additional > commits since the last revision: > > - Typo > - Merge branch 'master' into JDK-8325579 > - Rename test and refine comment > - Enhance test > - JDK-8325579 src/java.naming/share/classes/module-info.java line 51: > 49: * network times out. > 50: * <br> If a custom socket factory is provided via property > 51: * {@code java.naming.ldap.factory.socket} and unconnected sockets Suggestion: * <br> If a custom socket factory is provided via {@code java.naming.ldap.factory.socket} environment * property and unconnected sockets src/java.naming/share/classes/module-info.java line 51: > 49: * network times out. > 50: * <br> If a custom socket factory is provided via property > 51: * {@code java.naming.ldap.factory.socket} and unconnected sockets Suggestion: * <br> If a custom socket factory is provided via {@code java.naming.ldap.factory.socket} environment * property and unconnected sockets src/java.naming/share/classes/module-info.java line 51: > 49: * network times out. > 50: * <br> If a custom socket factory is provided via property > 51: * {@code java.naming.ldap.factory.socket} and unconnected sockets Since we're referencing socket factory env property, maybe we can also document it here. Something like: <li>{@code com.sun.jndi.ldap.connect.timeout}: <br>The value of this environment property specifies the fully qualified class name of the socket factory used by the LDAP provider. This class must implement the javax.net.SocketFactory abstract class. By default the environment property is not set. </li> ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1494840144 PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1494840513 PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1494861825
