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

[Christoph Langer]

On Tue, 19 Mar 2024 16:09:18 GMT, Aleksei Efimov <aefimov at openjdk.org> wrote:

>> 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/ffdffc5b...10271159
>
> 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");

Done.

> 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");

I simplified this, removing checkSocketClosed() method

> 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:
> 
>         }

I chose to throw a new RuntimeException(e)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1532883964
PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1532885041
PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1532884603