RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket

[Aleksei Efimov]

On Fri, 9 Feb 2024 21:29:28 GMT, Christoph Langer <clanger at 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?

Hi Christoph,

I think the proposed change is good, and it solves the problem we've also seen before with custom socket factories specified in the `"java.naming.ldap.factory.socket"` JNDI environment property not implementing  `javax.net.SocketFactory::createSocket()` method - custom implementations are not required to implement this method, hence `SocketException` can be thrown by the default implementation.
The change proposed by you should help to address such scenarios. 

It would also be great to update the `com.sun.jndi.ldap.connect.timeout` env property documentation in the `java.naming` module-info with the code comment mentioned above.
To fully clarify the `"unconnected sockets are not supported"` statement the `"java.naming.ldap.factory.socket"` environment property might need to have documentation added.

I've launched JNDI/LDAP regression tests with your patch and no failures were observed.
As a good addition to the proposed fix, it would be great to have a test for scenarios when a custom socket factory does/doesn't override the `createSocket` method. There are a few test examples that can be used as a bootstrap - for example, `test/jdk/com/sun/jndi/ldap/DeadSSLLdapTimeoutTest.java`.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/17797#issuecomment-1939169699