On Fri, 31 Jan 2025 18:01:28 GMT, Matthew Donovan <mdono...@openjdk.org> wrote:
>> Mikhail Yankelevich has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> cleanup
>
> test/jdk/java/security/cert/CertificateFactory/SlowStream.java line 52:
>
>> 50: while (true) {
>> 51: int len = fin.read(buffer);
>> 52: if (len < 0) break;
>
> it's clearer to do
>
> `if (len < 0) {
> break;
> }`
>
> it also matches the style at line 83
Done in the next commit
> test/jdk/java/security/cert/CertificateFactory/SlowStream.java line 60:
>
>> 58: } catch (final Exception e) {
>> 59: failed.set(true);
>> 60: exception.set(e);
>
> If you're setting this field in both reader and writer threads, is there a
> chance that one overwrites the other?
>
> Thinking about debugging this if the test fails... in the event of a error,
> should each thread print its stack trace and then set failed to true. Then at
> the end of the test, if failed is true, throw a RuntimeException with a
> generic message that something failed?
Done in the next commit
> test/jdk/java/security/cert/CertificateFactory/SlowStream.java line 68:
>
>> 66: final var factory =
>> CertificateFactory.getInstance("X.509");
>> 67: if (factory.generateCertificates(inputStream).size() !=
>> 5) {
>> 68: throw new Exception("Not all certs read");
>
> To aid debugging a failure, it might help to include the number of
> certificates that were read.
Done in the next commit
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23394#discussion_r1939889619
PR Review Comment: https://git.openjdk.org/jdk/pull/23394#discussion_r1939889552
PR Review Comment: https://git.openjdk.org/jdk/pull/23394#discussion_r1939889676
