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.
(Moved) com.linecorp.armeria.client.MetricCollectingConnectionPoolListener is moved.
- to com.linecorp.armeria.common.MetricCollectingConnectionPoolListener
(Moved) com.linecorp.armeria.client.ConnectionPoolListenerAdapter is moved.
- to com.linecorp.armeria.common.ConnectionPoolListenerAdapter

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: `f23ca82`)

Job name	Status	Build Scan®
build-windows-latest-jdk-21	❌ (failure)	https://ge.armeria.dev/s/dfqbn5ro7u2f4
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/4ybgstnzmbzjw
build-self-hosted-unsafe-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/76zl7lp5rk6xa
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/mkoamvigfyxeg
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/siq26twezpavc
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/x4pc3f52ut3uo
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/ahtdq4v7zz7dg
build-macos-12-jdk-21	✅	https://ge.armeria.dev/s/dq3gbsej7zzim

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 are missing coverage. Please review.

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

❗ Current head 4be97ef differs from pull request most recent head f23ca82. Consider uploading reports for the commit f23ca82 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.05%   74.03%   -0.03%     
+ Complexity    21253    21197      -56     
============================================
  Files          1850     1844       -6     
  Lines         78600    78438     -162     
  Branches      10032    10010      -22     
============================================
- Hits          58209    58069     -140     
+ Misses        15686    15668      -18     
+ Partials       4705     4701       -4

☔ 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

ikhoon · 2024-05-08T03:08:16Z

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

+/**
+ * A {@link ConnectionPoolListener} that collects metrics for connection pool events.
+ */
+public final class MetricCollectingConnectionPoolListener implements ConnectionPoolListener {


Should we remove MetricCollectingConnectionPoolListener from the public API to minimize a breaking change in the future?

Suggested change

public final class MetricCollectingConnectionPoolListener implements ConnectionPoolListener {

final class MetricCollectingConnectionPoolListener implements ConnectionPoolListener {

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

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

ikhoon · 2024-05-08T03:18:16Z

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

+    private void setConnectionKey(SessionProtocol desiredProtocol,
+                                  PoolKey poolkey,
+                                  ConnectionKey connectionKey) {
+        connectionKeys[desiredProtocol.ordinal()].put(poolkey, connectionKey);


What do you think of adding ConnectionKey to Channel.attr? We may fix ChannelUtil.localAddress() and ChannelUtil.remoteAddress() so that we can see ConnectionKey first.
For example:

class ChannelUtil { public static InetSocketAddress localAddress(@Nullable Channel ch) { final Attribute<ConnectionKey> attr = ch.attr(CONNECTION_KEY); if (attr != null) { final ConnectionKey key = attr.get(); if (key != null) { return key.localAddress(); } } ... } }

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

ikhoon · 2024-05-08T04:39:41Z

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

+/**
+ * Listens to the client connection pool events.
+ */
+public interface ConnectionPoolListener extends com.linecorp.armeria.client.ConnectionPoolListener {


On second thought, ConnectionEventListener would be a better name for the new API than ConnectionPoolListener. Because:

If we use the same name, setter methods can be easily overloaded but it is difficult to name the new API while deprecating the old one.

pool is not a good word for the server side. Servers accept incoming connections but do not manage pooled connections for reuse. Therefore, a more general term, event would make more sense.

Suggested change

public interface ConnectionPoolListener extends com.linecorp.armeria.client.ConnectionPoolListener {

public interface ConnectionEventListener {

Should we deprecate ClientFactoryBuilder.connectionPoolListener() and related methods in favor of ClientFactoryBuilder.connectionEventListener()?
For backward compatibility, we need to convert ConnectionPoolListener to ConnectionEventListener internally.

I like the name. 😊

@ikhoon

Should we deprecate ClientFactoryBuilder.connectionPoolListener() and related methods in favor of ClientFactoryBuilder.connectionEventListener()?

Yes, I think so.

For backward compatibility, we need to convert ConnectionPoolListener to ConnectionEventListener internally.

Okay. I'll convert it.👍

I converted the deprecated ConnectionPoolListener into ClientConnectionEventListener in the ClientFactoryBuilder. if there's already CONNECTION_EVENT_LISTENER, then it just ignores the ConnectionPoolListener.

// ClientConnectionEventListener.class static final ClientConnectionEventListenerAdapter NOOP = new ClientConnectionEventListenerAdapter(); /** * Convert the {@link ConnectionPoolListener} into a {@link ClientConnectionEventListener}. */ static ClientConnectionEventListener of(ConnectionPoolListener connectionPoolListener) { return new ClientConnectionEventListenerAdapter() { @Override public void connectionOpened(@Nullable SessionProtocol desiredProtocol, SessionProtocol protocol, InetSocketAddress remoteAddress, InetSocketAddress localAddress, AttributeMap attrs) throws Exception { connectionPoolListener.connectionOpen(protocol, remoteAddress, localAddress, attrs); } @Override public void connectionClosed(SessionProtocol protocol, InetSocketAddress remoteAddress, InetSocketAddress localAddress, boolean isIdle, AttributeMap attrs) throws Exception { connectionPoolListener.connectionClosed(protocol, remoteAddress, localAddress, attrs); } }; }

// ClientFactoryBuilder.class private ClientFactoryOptions buildOptions() { ... if (options.containsKey(ClientFactoryOptions.CONNECTION_POOL_LISTENER) && !options.containsKey(ClientFactoryOptions.CONNECTION_EVENT_LISTENER)) { final ConnectionPoolListener connectionPoolListener = (ConnectionPoolListener) options.get(ClientFactoryOptions.CONNECTION_POOL_LISTENER).value(); options.put(ClientFactoryOptions.CONNECTION_EVENT_LISTENER, ClientFactoryOptions.CONNECTION_EVENT_LISTENER .newValue(ClientConnectionEventListenerAdapter.of(connectionPoolListener))); }

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

minwoox · 2024-05-14T02:16:49Z

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

+    @Nullable
+    private SessionProtocol desiredProtocol;
+


Suggested change

@Nullable

private SessionProtocol desiredProtocol;

because this isn't used.

I forgot to use it. 😆 Thank you for the review.

This field is going to be used for identifying pending connection events on the client side.

Suppose you count a connection pending event (Like the ConnectionPoolMetrics class) and want to decrease the count after the connection is opened. In that case, you need a key to identify the previous connection pending event. Since the final protocol is undetermined in the connection pending phase, (desiredProtocol, remoteAddress, localAddress) can be used as the connection event key.

void connectionPending(SessionProtocol desiredProtocol, InetSocketAddress remoteAddress, InetSocketAddress localAddress, AttributeMap attrs) throws Exception; void connectionOpened(@Nullable SessionProtocol desiredProtocol, SessionProtocol protocol, InetSocketAddress remoteAddress, InetSocketAddress localAddress, AttributeMap attrs) throws Exception;

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

minwoox · 2024-05-16T01:17:07Z

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

+                    final ConnectionEventKey connectionEventKey = connectionEventKey(ctx.channel());
+
+                    try {
+                        connectionEventListener.connectionIdle(protocol,


Idle connections refer to connections that do not currently have any requests in progress. We've encountered a limitation with AbstractKeepAliveHandler: it resets the connection's idle time whenever it receives a ping.

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

Lines 269 to 271 in e99fa5d

if (connectionIdleTimeNanos > 0 && keepAliveOnPing) {

lastConnectionIdleTime = System.nanoTime();

}

Given this behavior, we're considering an alternative approach: utilizing the unfinishedResponses property in AbstractHttpResponseDecoder. By monitoring changes in unfinishedResponses, such as transitions from 0 to 1 indicating activity and from 1 to 0 suggesting idle status, we can effectively manage connection states without interfering with ping handling.
Side note: For HTTP/1.1 connections, the connectionActive and connectionIdle methods will be invoked frequently.

Also, what do you think of splitting this feature into separate PRs for client and server-side?

Thank you for the detailed explanation! 🙏 I'll try to utilize unfinishedResponses.

Side note: For HTTP/1.1 connections, the connectionActive and connectionIdle methods will be invoked frequently.

Agreed. Also, I'm a little bit worried about performance when a user adds heavy logic to the handler 🤔

Also, what do you think of splitting this feature into separate PRs for client and server-side?

Okay. I'll focus on the client side in this PR and apply it to the server side later.

Also, I'm a little bit worried about performance when a user adds heavy logic to the handler 🤔

It shouldn't be a problem because the default is no-op. 😉

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

Apply ConnectionEventListener

33f16ec

festinalente91 force-pushed the feature/enrich-connection-pool-listener-event-interface branch from 89e0257 to 33f16ec Compare May 14, 2024 02:39

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

Apply review

f23ca82

festinalente91 force-pushed the feature/enrich-connection-pool-listener-event-interface branch from 15bb579 to f23ca82 Compare May 14, 2024 13:37

minwoox reviewed May 16, 2024

View reviewed changes

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

CLAassistant commented Apr 8, 2024 •

edited

github-actions bot commented Apr 8, 2024 •

edited

ikhoon Apr 17, 2024

codecov bot commented Apr 21, 2024 •

edited

trustin left a comment

ikhoon May 8, 2024

ikhoon May 8, 2024

ikhoon May 8, 2024

minwoox May 8, 2024

festinalente91 May 8, 2024

festinalente91 May 14, 2024 •

edited

minwoox left a comment

minwoox May 14, 2024

festinalente91 May 14, 2024 •

edited

minwoox May 16, 2024

minwoox May 14, 2024

festinalente91 May 14, 2024

minwoox May 16, 2024

festinalente91 May 16, 2024

minwoox May 20, 2024

	public final class MetricCollectingConnectionPoolListener implements ConnectionPoolListener {
	final class MetricCollectingConnectionPoolListener implements ConnectionPoolListener {

	public interface ConnectionPoolListener extends com.linecorp.armeria.client.ConnectionPoolListener {
	public interface ConnectionEventListener {

	if (connectionIdleTimeNanos > 0 && keepAliveOnPing) {
	lastConnectionIdleTime = System.nanoTime();
	}

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

CLAassistant commented Apr 8, 2024 • edited

github-actions bot commented Apr 8, 2024 • edited

🔍 Build Scan® (commit: f23ca82)

Choose a reason for hiding this comment

codecov bot commented Apr 21, 2024 • edited

Codecov Report

trustin 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 May 14, 2024 • edited

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

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

Choose a reason for hiding this comment

festinalente91 commented Apr 8, 2024 •

edited

CLAassistant commented Apr 8, 2024 •

edited

github-actions bot commented Apr 8, 2024 •

edited

🔍 Build Scan® (commit: `f23ca82`)

codecov bot commented Apr 21, 2024 •

edited

festinalente91 May 14, 2024 •

edited

festinalente91 May 14, 2024 •

edited