fix: Use OkHttp `EventLister` instead of `ConnectionListener` for idle connection monitoring #1312

lauzadis · 2025-07-02T18:15:58Z

We implemented a ConnectionIdleMonitor in #1171, but it used okhttp3.ConnectionListener which was experimental. Now that API is internal in the latest release of OkHttp, we need to migrate to a stable API: okhttp3.EventListener

Issue #

Addresses #1311 in v1.5.x

Description of changes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

lauzadis · 2025-07-02T18:30:06Z

smithy-client-jvm.jar increased by 7KB likely due to OkHttp version bump

ianbotsf · 2025-07-02T19:35:35Z

.changes/db001c20-3788-4cfe-9ec2-284fd86a80bd.json

+{
+    "id": "db001c20-3788-4cfe-9ec2-284fd86a80bd",
+    "type": "bugfix",
+    "description": "Reimplement idle connection monitoring using okhttp3.EventListener instead of now-internal okhttp3.ConnectionListener",


Nit: monospace class names and packages

ianbotsf · 2025-07-02T19:39:04Z

...tp-client-engine-okhttp/jvm/src/aws/smithy/kotlin/runtime/http/engine/okhttp/OkHttpEngine.kt

+        // If connection idle polling is enabled, use ConnectionMonitoringEventListener
+        eventListenerFactory { call ->
+            if (config.connectionIdlePollingInterval != null) {
+                ConnectionMonitoringEventListener(
+                    pool,
+                    config.hostResolver,
+                    dispatcher,
+                    metrics,
+                    config.connectionIdlePollingInterval!!,
+                    call,
+                )
+            } else {
+                HttpEngineEventListener(pool, config.hostResolver, dispatcher, metrics, call)
+            }
+        }


Correctness: Won't this install a new connection monitor for every HTTP call? We need our monitoring to be cross-call since the problems occur when one call has released a connection, it's closed remotely, and then a new call acquires the connection. I think we need to instantiate our monitor once (probably along with the underlying OkHttp client instance) and reuse it.

ianbotsf · 2025-07-02T19:44:51Z

...tp/jvm/src/aws/smithy/kotlin/runtime/http/engine/okhttp/ConnectionMonitoringEventListener.kt

+internal class ConnectionMonitoringEventListener(
+    pool: ConnectionPool,
+    hr: HostResolver,
+    dispatcher: Dispatcher,
+    metrics: HttpClientMetrics,
+    private val pollInterval: Duration,
+    call: Call,
+) : HttpEngineEventListener(pool, hr, dispatcher, metrics, call) {


Correctness: I don't believe inheritance is the right decoupling strategy here. There's nothing really tying together the responsibilities of connection idle polling and emitting call-specific events/metrics.

I recommend an EventListener chain:

class EventListenerChain(val listeners: List<EventListener>) : EventListener() { val reverseListeners = listeners.asReverse() override fun callStart(call: Call) = listeners.forEach { it.callStart(call) } override fun callEnd(call: Call) = reverseListeners.forEach { it.calEnd(call) } }

And then combine the event emitter and idle poller via a chain.

ianbotsf · 2025-07-03T14:35:19Z

...tp/jvm/src/aws/smithy/kotlin/runtime/http/engine/okhttp/ConnectionMonitoringEventListener.kt

+/**
+ * An [okhttp3.EventListener] implementation that monitors connections for remote closure.
+ * This replaces the functionality previously provided by the now-internal [okhttp3.ConnectionListener].
+ */
+@InternalApi
+public class ConnectionMonitoringEventListener(private val pollInterval: Duration) : EventListener() {


Question: Why does this need to be @InternalApi public? Isn't it only used in this module?

It can't be internal because it's exposed by a public API OkHttpEngineConfig.buildClient

Does buildClient need to take a ConnectionMonitoringEventListener? Couldn't it just take a vararg clientScopedEventListeners: EventListener and make it more reusable?

Yeah that'll work better, updated!

ianbotsf · 2025-07-03T14:36:24Z

...tp/jvm/src/aws/smithy/kotlin/runtime/http/engine/okhttp/ConnectionMonitoringEventListener.kt

-    private val monitors = ConcurrentHashMap<Connection, Job>()
+    private val monitors = ConcurrentHashMap<Int, Job>()


Question: Why did this change?

🤷‍♂️ no particular reason, we don't need the Connection key and we were mixing use of connection and connection ID in the last implementation, I standardized everything on connection ID

Reverted back to Connection since it works better when logging Connection instead of connId

ianbotsf · 2025-07-03T14:52:05Z

...tp/jvm/src/aws/smithy/kotlin/runtime/http/engine/okhttp/ConnectionMonitoringEventListener.kt

-            val logger = context.logger<ConnectionIdleMonitor>()
-            logger.trace { "Cancel monitoring for $connection" }
+            val logger = context.logger<ConnectionMonitoringEventListener>()
+            logger.trace { "Cancel monitoring for $connId" }


Question: Connection has a nice toString representation that makes logs pretty useful. Example:

Connection{s3.us-west-2.amazonaws.com:443, proxy=DIRECT hostAddress=s3.us-west-2.amazonaws.com/52.92.164.216:443 cipherSuite=TLS_AES_128_GCM_SHA256 protocol=http/1.1}

Why did we switch to logging a hashcode instead?

Yeah that'll be better, I'll update that

ianbotsf · 2025-07-03T15:00:06Z

...ent-engine-okhttp/jvm/src/aws/smithy/kotlin/runtime/http/engine/okhttp/EventListenerChain.kt

+/**
+ * An [okhttp3.EventListener] that delegates to a chain of EventListeners.
+ * Forward events are sent in order, reverse events are sent in reverse order.
+ */


Nit: I might say that "Start event are sent in forward order, terminal events are sent in reverse order."

ianbotsf · 2025-07-03T15:00:35Z

...ent-engine-okhttp/jvm/src/aws/smithy/kotlin/runtime/http/engine/okhttp/EventListenerChain.kt

+@InternalApi
+public class EventListenerChain(


Question: Why does this need to be @InternalApi public?

It can be made internal

ianbotsf · 2025-07-03T15:03:26Z

...ent-engine-okhttp/jvm/src/aws/smithy/kotlin/runtime/http/engine/okhttp/EventListenerChain.kt

+    override fun cacheConditionalHit(call: Call, cachedResponse: Response): Unit =
+        reverseListeners.forEach { it.cacheConditionalHit(call, cachedResponse) }
+
+    override fun cacheHit(call: Call, response: Response): Unit =
+        reverseListeners.forEach { it.cacheHit(call, response) }
+
+    override fun cacheMiss(call: Call): Unit =
+        reverseListeners.forEach { it.cacheMiss(call) }


Nit: The cache events are non-terminal and should be handled front-to-back.

ianbotsf · 2025-07-03T15:05:58Z

...tp-client-engine-okhttp/jvm/src/aws/smithy/kotlin/runtime/http/engine/okhttp/OkHttpEngine.kt

+/**
+ * Convert SDK version of HTTP configuration to OkHttp specific configuration and return the configured client
+ */
+@InternalApi
+public fun OkHttpEngineConfig.buildClient(
    metrics: HttpClientMetrics,
    poolOverride: ConnectionPool? = null,
+    connectionMonitoringListener: ConnectionMonitoringEventListener? = null,
 ): OkHttpClient {


Question: Do we still set a poolOverride anywhere? Can that parameter be removed?

We don't but our users might be? Since this is going in v1.5 it's probably fine to remove

The only method that allowed passing a pool override was previously private so users couldn't be using it. Let's remove the parameter if we don't need it anymore.

This reverts commit 57fda03.

This reverts commit 8c81ac9.

0marperez · 2025-07-03T21:22:20Z

...ent-engine-okhttp/jvm/src/aws/smithy/kotlin/runtime/http/engine/okhttp/EventListenerChain.kt

+
+/**
+ * An [okhttp3.EventListener] that delegates to a chain of EventListeners.
+ * Start event are sent in forward order, terminal events are sent in reverse order


Nit: "Start event" -> "Start events"

0marperez · 2025-07-03T21:27:07Z

...tp-client-engine-okhttp/jvm/src/aws/smithy/kotlin/runtime/http/engine/okhttp/OkHttpEngine.kt

@@ -102,7 +112,7 @@ private fun OkHttpEngineConfig.buildClientFromConfig(
        writeTimeout(config.socketWriteTimeout.toJavaDuration())

        // use our own pool configured with the timeout settings taken from config


Nit: This comment isn't accurate anymore

Yes it is. We are creating our own pool using timeout settings taken from config

lauzadis · 2025-07-07T15:38:45Z

Downstream CI is failing until Kotlin 2.2.0 upgrade is merged in aws-sdk-kotlin

… fix-connection-listener

lauzadis changed the base branch from main to v1.5-main July 2, 2025 18:18

Reimplement idle connection monitoring using okhttp3.EventListener

52d84fd

This comment has been minimized.

Sign in to view

lauzadis force-pushed the fix-connection-listener branch from eb1006f to 52d84fd Compare July 2, 2025 18:20

lauzadis marked this pull request as ready for review July 2, 2025 18:20

lauzadis requested a review from a team as a code owner July 2, 2025 18:20

lauzadis added the acknowledge-api-break Acknowledge that a change is API breaking and may be backwards-incompatible. Review carefully! label Jul 2, 2025

lauzadis added 2 commits July 2, 2025 14:22

Add changelog

e0e9680

Upgrade to latest version of OkHttp

57fda03

This comment has been minimized.

Sign in to view

Changelog

8c81ac9

This comment has been minimized.

Sign in to view

lauzadis added the acknowledge-artifact-size-increase label Jul 2, 2025

This comment has been minimized.

Sign in to view

ianbotsf requested changes Jul 2, 2025

View reviewed changes

lauzadis added 7 commits July 3, 2025 09:29

EventListenerChain

e75ab3b

Create a single connection monitor, reuse it

5355ea3

update comment

d5655eb

monospace

0480652

clenaup

20d96f5

fix build

44aa0ff

ktlint

c35e62d

This comment has been minimized.

Sign in to view

Simplify connection monitoring listener registration

b575879

This comment has been minimized.

Sign in to view

lauzadis added 2 commits July 3, 2025 10:15

add EventListenerChain tests

6dcb6af

move test class to the bottom

5ca0d1c

This comment has been minimized.

Sign in to view

ianbotsf reviewed Jul 3, 2025

View reviewed changes

lauzadis added 9 commits July 3, 2025 11:12

make EventListenerChain internal

d5a8ce7

update cache events

4aa6b51

remove poolOverride

02a051d

remove poolOverride apiDump

d5a4b28

Use connection instead of connId in user-facing logging

1e0dc1f

Make ConnectionMonitoringEventListener internal

9c42e30

Revert "Upgrade to latest version of OkHttp"

1f9b907

This reverts commit 57fda03.

Revert "Changelog"

a23bef9

This reverts commit 8c81ac9.

Revert how soTimeout is updated

ff9e771

This comment has been minimized.

Sign in to view

lauzadis requested a review from ianbotsf July 3, 2025 18:13

0marperez approved these changes Jul 3, 2025

View reviewed changes

Add missing letter

d79cc34

This comment has been minimized.

Sign in to view

ianbotsf approved these changes Jul 7, 2025

View reviewed changes

lauzadis added 2 commits July 8, 2025 10:17

Upgrade to OkHttp 5.1.0 and Okio 3.15.0

d695ac5

Merge branch 'v1.5-main' of github.com:smithy-lang/smithy-kotlin into…

0457ba7

… fix-connection-listener

This comment has been minimized.

Sign in to view

lauzadis merged commit cec880a into v1.5-main Jul 8, 2025
24 of 25 checks passed

lauzadis deleted the fix-connection-listener branch July 8, 2025 16:48

lauzadis mentioned this pull request Jul 8, 2025

NoClassDefFoundError with OkHttpEngine since okhttp 5.0.0-alpha.17 #1311

Closed

1 task

		private val monitors = ConcurrentHashMap<Connection, Job>()
		private val monitors = ConcurrentHashMap<Int, Job>()

		@@ -102,7 +112,7 @@ private fun OkHttpEngineConfig.buildClientFromConfig(
		writeTimeout(config.socketWriteTimeout.toJavaDuration())

		// use our own pool configured with the timeout settings taken from config

fix: Use OkHttp EventLister instead of ConnectionListener for idle connection monitoring #1312

fix: Use OkHttp EventLister instead of ConnectionListener for idle connection monitoring #1312

Uh oh!

Conversation

lauzadis commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue #

Description of changes

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

lauzadis commented Jul 2, 2025

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ianbotsf Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

lauzadis commented Jul 7, 2025

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

fix: Use OkHttp `EventLister` instead of `ConnectionListener` for idle connection monitoring #1312

fix: Use OkHttp `EventLister` instead of `ConnectionListener` for idle connection monitoring #1312

lauzadis commented Jul 2, 2025 •

edited

Loading

ianbotsf Jul 2, 2025 •

edited

Loading