Enrich ConnectionPoolListener interface to listen more connection events #5580

festinalente91 · 2024-04-08T08:01:07Z

Motivation:

ConnectionPoolListener provides a limited way to listen to connection events. (open, close)
We could provide a more sophisticated event interface like the one below.

Connection pending
Connection failed
Connection active
Connection idle

                                                                                           
                                    +------------------+                                   
        +--------------+            | +--------------+ |            +--------------+       
        |              |            | |              | |            |              |       
        |    Pending   |----------->| |    Opened    | |----------> |    Closed    |       
        |              |            | |              | |            |              |       
        +--------------+            | +--------------+ |            +--------------+       
                |                   |                  |                                   
                |                   |      Active      |                                   
                |                   |         ^        |                                   
                |                   |         |        |                                   
                |                   |         |        |                                   
                v                   |         |        |                                   
        +--------------+            |         |        |                                   
        |              |            |         v        |                                   
        |    Failed    |            |       Idle       |                                   
        |              |            |                  |                                   
        +--------------+            +------------------+

Furthermore, as @ikhoon mentioned here
we can also expand this connection event interface to be used on the server side.

Modifications:

(Deprecated) com.linecorp.armeria.client.ConnectionPoolListener class is deprecated.
- Use com.linecorp.armeria.client.ClientConnectionEventListener instead.

Result:

Now you can handle more detailed connection pool events and metrics.
Closes Enrich ConnectionPoolListener interface to listen to more connection events #4649 .

CLAassistant · 2024-04-08T08:01:13Z

All committers have signed the CLA.

github-actions · 2024-04-08T08:46:44Z

🔍 Build Scan® (commit: `e25895e`)

Job name	Status	Build Scan®
build-windows-latest-jdk-21	✅	https://ge.armeria.dev/s/ic4alezbk7wny
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/ofs5im2f34bsi
build-self-hosted-unsafe-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/25e3alghykzd4
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/n2vhor3x6lbuu
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/2t6phqty33n6w
build-self-hosted-unsafe-jdk-17-leak	❌ (failure)	https://ge.armeria.dev/s/r7y4tsj5g7udk
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/b5pb43s42ybcm
build-macos-12-jdk-21	✅	https://ge.armeria.dev/s/usjacqt6wwcmk

ikhoon · 2024-04-17T09:12:23Z

core/src/main/java/com/linecorp/armeria/internal/common/AbstractKeepAliveHandler.java

@@ -181,6 +189,26 @@ public final void onReadOrWrite() {
            lastConnectionIdleTime = System.nanoTime();
        }

+        if (!isServer && !isActive) {


It should be useful if we also support ConnectionPoolListener for servers. Although we are focusing on the client-side implementation, the public API design should take extensibility into consideration.

I propose to add a new ConnectionPoolListener to c.l.a.common package. The new methods will be added to the new ConnectionPoolListener which extends the old ConnectionPoolListener for compatibility. For example:

armeria/core/src/main/java/com/linecorp/armeria/common/encoding/StreamDecoder.java

Line 27 in 1d268aa

public interface StreamDecoder extends com.linecorp.armeria.client.encoding.StreamDecoder {

Related issue: #3722

core/src/main/java/com/linecorp/armeria/internal/common/AbstractKeepAliveHandler.java

codecov · 2024-04-21T11:13:41Z

Codecov Report

Attention: Patch coverage is 78.20513% with 17 lines in your changes missing coverage. Please review.

Project coverage is 74.03%. Comparing base (e08527d) to head (4be97ef).
Report is 6 commits behind head on main.

❗ Current head 4be97ef differs from pull request most recent head b8873e1

Please upload reports for the commit b8873e1 to get more accurate results.

Files	Patch %	Lines
...eria/internal/common/AbstractKeepAliveHandler.java	66.66%	8 Missing and 2 partials ⚠️
.../armeria/client/ConnectionPoolListenerWrapper.java	0.00%	4 Missing ⚠️
...a/com/linecorp/armeria/client/HttpChannelPool.java	50.00%	1 Missing ⚠️
...inecorp/armeria/common/ConnectionPoolListener.java	0.00%	1 Missing ⚠️
...linecorp/armeria/common/ConnectionPoolMetrics.java	96.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5580      +/-   ##
============================================
- Coverage     74.75%   74.03%   -0.73%     
+ Complexity    21409    21197     -212     
============================================
  Files          1877     1844      -33     
  Lines         79126    78438     -688     
  Branches      10201    10010     -191     
============================================
- Hits          59152    58069    -1083     
- Misses        15179    15668     +489     
+ Partials       4795     4701      -94

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

core/src/main/java/com/linecorp/armeria/client/ConnectionPoolListener.java

trustin

This is really good. Thanks for your high quality contribution, @festinalente91!

core/src/main/java/com/linecorp/armeria/client/ConnectionPoolListener.java

core/src/main/java/com/linecorp/armeria/common/ConnectionPoolListenerAdapter.java

core/src/main/java/com/linecorp/armeria/client/ConnectionPoolListener.java

core/src/main/java/com/linecorp/armeria/common/ConnectionPoolMetrics.java

core/src/main/java/com/linecorp/armeria/common/MetricCollectingConnectionPoolListener.java

core/src/main/java/com/linecorp/armeria/client/ConnectionPoolListener.java

core/src/main/java/com/linecorp/armeria/client/HttpChannelPool.java

core/src/main/java/com/linecorp/armeria/common/ConnectionPoolListener.java

minwoox

Basically, looks good. 👍

core/src/main/java/com/linecorp/armeria/client/ClientConnectionEventListener.java

core/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.java

core/src/main/java/com/linecorp/armeria/client/Http1ClientKeepAliveHandler.java

core/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.java

core/src/main/java/com/linecorp/armeria/client/HttpClientFactory.java

minwoox · 2024-05-14T02:15:32Z

core/src/main/java/com/linecorp/armeria/internal/common/AbstractKeepAliveHandler.java

+        final ConnectionEventKey connectionEventKey = connectionEventKey(channel);
+
+        try {
+            connectionEventListener.connectionOpened(connectionEventKey, channel);


If NoopKeepAliveHandler is used, this event isn't called anymore.

armeria/core/src/main/java/com/linecorp/armeria/client/Http1ResponseDecoder.java

Line 98 in 89e0257

keepAliveHandler = new NoopKeepAliveHandler();

Is there any reason that you've moved this logic from HttpChannelPool?

I tried to move open and close event trigger logic into the AbstractKeepAliveHandler to reuse this on the server side. However, as you said, I had to make extra changes to do that. Any feedback for this? @minwoox @ikhoon

Added connection opened and closed event trigger logic into the NoopKeepAliveHandler

To share the open and close event trigger logic on server side.

Handled HTTP Upgrade cases with context-checking(HttpProtocolUpgradeHandler)

When the protocol is upgraded, AbstractKeepAliveHandler is called twice.

So, I had to ignore the first one which was related to the protocol upgrade process.

(Still struggling to handle HTTP2 Max connection setting test case.

void exceededMaxStreamsPropagatesFailureCorrectly() throws Exception {)

I tried to move open and close event trigger logic into the AbstractKeepAliveHandler to reuse this on the server side

Since the connection mechanisms differ between the server-side and client-side, I don't think we have to put the logic into the same class.
Instead, we could consider incorporating the logic for the server-side into ConnectionLimitingHandler, which is already used for managing connections. You can find the ConnectionLimitingHandler implementation here: Link

core/src/main/java/com/linecorp/armeria/common/ConnectionEventKey.java

…t-interface

When the sessionPromise fails, sessionPromise.getNow() returns a FailedFuture, which returns the channel as null. To keep the connectionEventState, I added connectionEventStates to the HttpChannelPool.

…t-interface

jrhee17

Left some more comments 👍

jrhee17 · 2024-06-26T13:59:53Z

core/src/main/java/com/linecorp/armeria/client/HttpChannelPool.java

+                    // When the transport type(-Dcom.linecorp.armeria.transportType) is NIO,
+                    // sometimes local address is not available yet.


I believe this is because the channel isn't bound yet at this point. Is there a reason we can't just remove L434-L445?

core/src/main/java/com/linecorp/armeria/client/HttpChannelPool.java

core/src/main/java/com/linecorp/armeria/internal/common/ConnectionEventState.java

core/src/main/java/com/linecorp/armeria/internal/common/NoopKeepAliveHandler.java

core/src/main/java/com/linecorp/armeria/client/ClientConnectionEventListenerAdapter.java

ikhoon · 2024-06-26T13:03:40Z

core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java

    public ClientFactoryBuilder connectionPoolListener(
            ConnectionPoolListener connectionPoolListener) {
        option(ClientFactoryOptions.CONNECTION_POOL_LISTENER,
               requireNonNull(connectionPoolListener, "connectionPoolListener"));
        return this;
    }

+    /**
+     * Sets the listener which is notified on a connection event.


I slightly prefer raising an IllegalStateException when both ConnectionPoolListener and ClientConnectionEventListener are set.
We may store a specified ClientConnectionEventListener in a member field and set to options in buildOptions().

public ClientFactoryBuilder connectionPoolListener( ConnectionPoolListener connectionPoolListener) { checkState(connectionEventListener == null, "connectionPoolListener() and connectionEventListener() are mutually exclusive") this.connectionPoolListener = connectionPoolListener; } ... private ClientFactoryOptions buildOptions() { if (connectionPoolListener != null) { // Convert connectionPoolListener to connectionEventListener. } }

core/src/main/java/com/linecorp/armeria/client/HttpChannelPool.java

ikhoon · 2024-06-27T11:54:22Z

This became a big PR with more than 2000 lines. As a first-time contributor, you are not familiar with Armeria's code internals and conventions. It is also not easy for us to review over 2000 lines.

Let me add a clean-up commit and build together. 😉

…tion-pool-listener-event-interface

jrhee17 · 2024-07-10T10:06:37Z

Is this PR re-ready for review? I prefer at least the CI be passing before reviewing since it is time/effort consuming on my end as well.

festinalente91 · 2024-07-10T10:38:12Z

Is this PR re-ready for review? I prefer at least the CI be passing before reviewing since it is time/effort consuming on my end as well.

@jrhee17 Not yet. I apologize for the delay. I will update this PR over the weekend and let you know.

jrhee17 · 2024-07-11T01:15:15Z

I will update this PR over the weekend and let you know.

I saw this comment late: #5580 (comment)
Let's wait to see if @ikhoon is done updating before proceeding with this PR further.

ikhoon · 2024-07-16T11:22:42Z

Let's wait to see if @ikhoon is done updating before proceeding with this PR further.

I refactored the code but I asked @festinalente91 to finish this PR. I may add some minor fixes but @festinalente91 will handle the remaining part.

jrhee17

Left some high level comments 🙇

core/src/main/java/com/linecorp/armeria/client/ClientConnectionEventListener.java

jrhee17 · 2024-07-19T08:17:49Z

core/src/main/java/com/linecorp/armeria/internal/common/AbstractKeepAliveHandler.java

@@ -120,14 +127,22 @@ protected AbstractKeepAliveHandler(Channel channel, String name, Timer keepAlive
    public final void initialize(ChannelHandlerContext ctx) {


I believe this method is called when the HttpSessionHandler is added and not necessarily when the connection is ready for sending/receiving requests.

jrhee17 · 2024-07-19T08:18:32Z

core/src/main/java/com/linecorp/armeria/internal/common/AbstractKeepAliveHandler.java

+
+        keepAliveState = KeepAliveState.INITIALIZED;
+        if (connectionEventListener != null) {
+            connectionEventListener.connectionOpened();


It seems like if initialize is called due to channelActive or handlerAdded, the HttpSession#protocol may not exist. Can you check this point?

You are right. connectionEventListener.connectionOpened() should be called after sessionPromise is completed.
Furthermore, (to support maxUnfinishedRequest filter) this should called before checking unfinished request.
And I was struggling to solve this problem. 🫠

final class HttpChannelPool implements AsyncCloseable { .... private void notifyConnect(SessionProtocol desiredProtocol, PoolKey key, Future<Channel> future, ChannelAcquisitionFuture promise, ClientConnectionTimingsBuilder timingsBuilder) { .... // In the current implementation(master branch), connectionOpen is called at here if (session.incrementNumUnfinishedResponses()) { if (protocol.isMultiplex()) { final Http2PooledChannel pooledChannel = new Http2PooledChannel(channel, protocol); addToPool(protocol, key, pooledChannel); promise.complete(pooledChannel); } else { promise.complete(new Http1PooledChannel(channel, protocol, key)); } } else { // Server set MAX_CONCURRENT_STREAMS to 0, which means we can't send anything. channel.close(); promise.completeExceptionally( UnprocessedRequestException.of(RefusedStreamException.get())); } ... }

jrhee17 · 2024-07-19T08:19:55Z

core/src/main/java/com/linecorp/armeria/internal/common/DelegatingConnectionEventListener.java

+        return actualProtocol;
+    }
+
+    public void connectionPending() {


connectionPending sounds a little odd since ClientConnectionTimings#pendingAcquisition seems to be measuring a different timing although it has a similar name.

Would sessionPending make more sense? @line/dx

I prefer a method name starting with connection to be consistent with other methods.
How about connectionNegotiating()?

…tion-pool-listener-event-interface

github-actions bot requested review from ikhoon, jrhee17, minwoox and trustin April 8, 2024 08:01

festinalente91 force-pushed the feature/enrich-connection-pool-listener-event-interface branch from b52f5a3 to 2851ee6 Compare April 8, 2024 08:41

ikhoon added new feature sprint Issues for OSS Sprint participants labels Apr 8, 2024

festinalente91 force-pushed the feature/enrich-connection-pool-listener-event-interface branch 2 times, most recently from 9f7edb8 to 1da16f2 Compare April 17, 2024 01:12

ikhoon reviewed Apr 17, 2024

View reviewed changes

core/src/main/java/com/linecorp/armeria/internal/common/AbstractKeepAliveHandler.java Outdated Show resolved Hide resolved

festinalente91 force-pushed the feature/enrich-connection-pool-listener-event-interface branch from 416f42b to 4be97ef Compare April 21, 2024 10:38

festinalente91 force-pushed the feature/enrich-connection-pool-listener-event-interface branch from 918d3ac to 6250594 Compare April 28, 2024 03:42

trustin reviewed May 2, 2024

View reviewed changes

core/src/main/java/com/linecorp/armeria/client/ConnectionPoolListener.java Outdated Show resolved Hide resolved

festinalente91 force-pushed the feature/enrich-connection-pool-listener-event-interface branch 5 times, most recently from edcff6b to 5a4530a Compare May 7, 2024 01:52

festinalente91 requested a review from ikhoon May 7, 2024 04:46

festinalente91 marked this pull request as ready for review May 7, 2024 04:50

trustin approved these changes May 7, 2024

View reviewed changes

ikhoon reviewed May 8, 2024

View reviewed changes

festinalente91 force-pushed the feature/enrich-connection-pool-listener-event-interface branch from 5a4530a to 89e0257 Compare May 12, 2024 03:34

minwoox reviewed May 14, 2024

View reviewed changes

festinalente91 force-pushed the feature/enrich-connection-pool-listener-event-interface branch 2 times, most recently from e99fa5d to 6e4e7e5 Compare May 14, 2024 13:25

festinalente91 and others added 4 commits June 21, 2024 19:08

Add MetricCollectingClientConnectionEventListener

73fd998

Merge branch 'main' into feature/enrich-connection-pool-listener-even…

05ee717

…t-interface

Enhance test

f624867

Handle connection failed case

f7231d4

When the sessionPromise fails, sessionPromise.getNow() returns a FailedFuture, which returns the channel as null. To keep the connectionEventState, I added connectionEventStates to the HttpChannelPool.

festinalente91 requested review from ikhoon, jrhee17 and minwoox June 25, 2024 11:21

festinalente91 and others added 4 commits June 25, 2024 21:08

Clean up code with KeepAliveState enum

a6b59e2

Clean up comment in the test

795f394

Fix to hide KeepAliveState enum

7a90c51

Merge branch 'main' into feature/enrich-connection-pool-listener-even…

feeb0f4

…t-interface

jrhee17 reviewed Jun 26, 2024

View reviewed changes

ikhoon reviewed Jun 26, 2024

View reviewed changes

ikhoon added 2 commits June 28, 2024 15:39

Merge branch 'main' into pr-5580-festinalente91-feature/enrich-connec…

f10039d

…tion-pool-listener-event-interface

Introduce DelegatingConnectionEventListener

b8873e1

Notify connection events at NoopKeepAliveHandler

0d45c62

jrhee17 reviewed Jul 19, 2024

View reviewed changes

ikhoon added 5 commits August 1, 2024 19:06

clean up

d10019d

Remove calling of keepAliveHandler.destroy() as possible

91e814b

Merge branch 'main' into pr-5580-festinalente91-feature/enrich-connec…

da15869

…tion-pool-listener-event-interface

nullaway

fe3895d

lint

e25895e

ikhoon modified the milestones: 1.30.0, 1.31.0 Aug 1, 2024

github-actions bot added the Stale label Sep 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enrich ConnectionPoolListener interface to listen more connection events #5580

Enrich ConnectionPoolListener interface to listen more connection events #5580

festinalente91 commented Apr 8, 2024 •

edited

Loading

CLAassistant commented Apr 8, 2024 •

edited

Loading

github-actions bot commented Apr 8, 2024 •

edited

Loading

ikhoon Apr 17, 2024

codecov bot commented Apr 21, 2024 •

edited

Loading

trustin left a comment

minwoox left a comment

minwoox May 14, 2024

festinalente91 May 14, 2024 •

edited

Loading

minwoox May 16, 2024

jrhee17 left a comment

jrhee17 Jun 26, 2024

ikhoon Jun 26, 2024

ikhoon commented Jun 27, 2024

jrhee17 commented Jul 10, 2024

festinalente91 commented Jul 10, 2024

jrhee17 commented Jul 11, 2024

ikhoon commented Jul 16, 2024

jrhee17 left a comment

jrhee17 Jul 19, 2024

jrhee17 Jul 19, 2024

festinalente91 Jul 19, 2024

jrhee17 Jul 19, 2024

ikhoon Jul 31, 2024

		// When the transport type(-Dcom.linecorp.armeria.transportType) is NIO,
		// sometimes local address is not available yet.

		@@ -120,14 +127,22 @@ protected AbstractKeepAliveHandler(Channel channel, String name, Timer keepAlive
		public final void initialize(ChannelHandlerContext ctx) {

Enrich ConnectionPoolListener interface to listen more connection events #5580

Are you sure you want to change the base?

Enrich ConnectionPoolListener interface to listen more connection events #5580

Conversation

festinalente91 commented Apr 8, 2024 • edited Loading

CLAassistant commented Apr 8, 2024 • edited Loading

github-actions bot commented Apr 8, 2024 • edited Loading

🔍 Build Scan® (commit: e25895e)

Choose a reason for hiding this comment

codecov bot commented Apr 21, 2024 • edited Loading

Codecov Report

trustin left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

festinalente91 May 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon commented Jun 27, 2024

jrhee17 commented Jul 10, 2024

festinalente91 commented Jul 10, 2024

jrhee17 commented Jul 11, 2024

ikhoon commented Jul 16, 2024

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

festinalente91 commented Apr 8, 2024 •

edited

Loading

CLAassistant commented Apr 8, 2024 •

edited

Loading

github-actions bot commented Apr 8, 2024 •

edited

Loading

🔍 Build Scan® (commit: `e25895e`)

codecov bot commented Apr 21, 2024 •

edited

Loading

festinalente91 May 14, 2024 •

edited

Loading