RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v8]
Christoph Langer
clanger at openjdk.org
Wed Mar 20 21:19:40 UTC 2024
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
More information about the core-libs-dev
mailing list