On Wed, 6 May 2026 13:49:25 GMT, Alan Bateman <[email protected]> wrote:
>> Robert Toyonaga has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review feedback
>
> test/jdk/jdk/jfr/event/io/TestEventsNotLoadedWithoutJfr.java line 90:
>
>> 88: String s = "short";
>> 89: ByteBuffer data = ByteBuffer.allocate(s.length());
>> 90: data.put(s.getBytes());
>
> `ByteBuffer buffer = StandardCharsets.UTF_8.encode("short");` might be
> clearer.
Thanks! I've adopted your suggestion.
> test/jdk/jdk/jfr/event/io/TestEventsNotLoadedWithoutJfr.java line 108:
>
>> 106: }
>> 107: } finally {
>> 108: Files.deleteIfExists(tmp);
>
> It's often better to not delete the file used by a test as it may be needed
> when diagnosing a test failure. For this test I think better to drop the
> try-finally-delete.
Okay I see. I've removed it now.
> test/jdk/jdk/jfr/event/io/TestEventsNotLoadedWithoutJfr.java line 130:
>
>> 128: }
>> 129: });
>> 130: clientThread.start();
>
> A simpler (and more reliable) test does not need clientThread. The connect
> method will establish the connection, it does not require a thread to be
> blocked in the accept method. This will allow to invoke server.accept after
> the client.connect and it will work fine. You'll find dozens of tests in the
> net and nio/channels directories that do this. On the reliability front it
> means an exception on the "client end" will cause the test method to throw.
Okay I see. I've gotten rid of the pattern of starting a new thread and adopted
your approach instead.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30948#discussion_r3196714422
PR Review Comment: https://git.openjdk.org/jdk/pull/30948#discussion_r3196711763
PR Review Comment: https://git.openjdk.org/jdk/pull/30948#discussion_r3196720215
