On Sat, 23 Nov 2024 08:36:03 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> Tim Prinzing has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Added more tests for socket connect events.
>>
>> - SocketAdapter connect
>> - SocketAdapter connect with exception
>> - Socket connect with exception
>> - SocketChannel connect with exception
>> - SocketChannel non-blocking connect
>> - SocketChannel non-blocking connect with exception
>
> test/jdk/jdk/jfr/event/io/TestSocketChannelEvents.java line 157:
>
>> 155: while (! sc.finishConnect()) {
>> 156: Thread.sleep(1);
>> 157: }
>
> The connect method returns true if the connection is established, you should
> only need to poll finishConnect if the connection is not established
> immediately. Using a sleep is okay here (although 1ms is very low) and I'm
> guessing you've done this to avoid using a Selector.
It should be OK to sleep for longer if you don't want to use a selector.
> test/jdk/jdk/jfr/event/io/TestSocketChannelEvents.java line 216:
>
>> 214: } catch (IOException ioe) {
>> 215: // we expect this
>> 216: connectException = ioe;
>
> I think you'll need to adjust the try-catch to only set connectException if
> the connect or finishConnect methods fails. If the open or configureBlocking
> fails then connectException should not be set, instead the test should fail.
In addition there's no guarantee that connect will fail - so the test should
guard for the case where connect might succeed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21528#discussion_r1867981816
PR Review Comment: https://git.openjdk.org/jdk/pull/21528#discussion_r1868041972
