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

[Aleksei Efimov]

On Thu, 14 Mar 2024 21:59:54 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?
>
> 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