RFR: 8310996: Add JFR event for connect operations [v5]

[Alan Bateman]

On Fri, 22 Nov 2024 20:32:50 GMT, Tim Prinzing <tprinzing at 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