On Fri, 22 Nov 2024 20:32:50 GMT, Tim Prinzing <tprinz...@openjdk.org> wrote:
>> Adds a JFR event for socket connect operations.
>>
>> Existing tests TestSocketEvents and TestSocketChannelEvents modified to also
>> check for connect events.
>
> 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
Thanks for the update, it's good to have tests for the 8 possible cases that
might record a SocketConnect event. If you have the energy for it, we could
also add tests to check that a SocketConnect is not recorded for cases where
the connect method fails for other reasons, e.g. already connected, Socket
closed, ... but only if you want.
src/java.base/share/classes/jdk/internal/event/SocketConnectEvent.java line 37:
> 35: * {@code jdk.jfr.events.SocketConnectEvent } where the metadata for the
> event is
> 36: * provided with annotations. Some of the methods are replaced by
> generated
> 37: * methods when jfr is enabled. Note that the order of the arguments of
> the
In passing, there is a mix of "JFR" and "jfr" through-out, I assume you want
"JFR" everywhere.
src/java.base/share/classes/jdk/internal/event/SocketConnectEvent.java line 51:
> 49:
> 50: /**
> 51: * Actually commit an event. The implementation is generated
> automatically.
I assume "Actually" should be removed, this method commits an event.
src/jdk.jfr/share/classes/jdk/jfr/events/SocketConnectEvent.java line 38:
> 36: @Label("Socket Connect")
> 37: @Category("Java Application")
> 38: @Description("Connecting a socket")
I wonder if we can improve on this description. The event is recorded when a
connection is established or cannot be established so it's more like "Socket
Connection".
test/jdk/jdk/jfr/event/io/IOHelper.java line 135:
> 133: }
> 134:
> 135: public static void checkConnectionEventException(RecordedEvent
> event, IOException ioe) {
I assume this should checkConnectEventException.
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.
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.
-------------
PR Review: https://git.openjdk.org/jdk/pull/21528#pullrequestreview-2456365810
PR Review Comment: https://git.openjdk.org/jdk/pull/21528#discussion_r1855145320
PR Review Comment: https://git.openjdk.org/jdk/pull/21528#discussion_r1855145369
PR Review Comment: https://git.openjdk.org/jdk/pull/21528#discussion_r1855145935
PR Review Comment: https://git.openjdk.org/jdk/pull/21528#discussion_r1855146240
PR Review Comment: https://git.openjdk.org/jdk/pull/21528#discussion_r1855150781
PR Review Comment: https://git.openjdk.org/jdk/pull/21528#discussion_r1855151098
