Skip to content
Merged
8 changes: 8 additions & 0 deletions .changes/4d3a104a-3225-4dcb-be05-f5155d320952.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"id": "4d3a104a-3225-4dcb-be05-f5155d320952",
"type": "feature",
"description": "Allow configuring a custom OkHttpClient in OkHttpEngine",
"issues": [
"https://github.com/aws/aws-sdk-kotlin/issues/1707"
]
}
6 changes: 2 additions & 4 deletions runtime/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,7 @@
* SPDX-License-Identifier: Apache-2.0
*/
import aws.sdk.kotlin.gradle.dsl.configurePublishing
import aws.sdk.kotlin.gradle.kmp.configureKmpTargets
import aws.sdk.kotlin.gradle.kmp.kotlin
import aws.sdk.kotlin.gradle.kmp.needsKmpConfigured
import aws.sdk.kotlin.gradle.kmp.*
import org.jetbrains.kotlin.gradle.dsl.JvmTarget
import org.jetbrains.kotlin.gradle.plugin.mpp.KotlinNativeTarget
import org.jetbrains.kotlin.gradle.targets.native.tasks.KotlinNativeSimulatorTest
Expand Down Expand Up @@ -143,7 +141,7 @@ subprojects {
}
}

// configureIosSimulatorTasks()
configureIosSimulatorTasks()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Why did this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why it was commented, but I don't think it should be. This is necessary for TLS tests to pass, it was originally added to aws-crt-kotlin and I applied it here too, but it might not be necessary since our CI has been passing without it. Want me to delete it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's not necessary for CI and we don't need it for any other reason then yes, I think we should remove it.

val excludeFromDocumentation = listOf(
":runtime:testing",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ public final class aws/smithy/kotlin/runtime/http/engine/okhttp/OkHttpEngine : a
public static final field Companion Laws/smithy/kotlin/runtime/http/engine/okhttp/OkHttpEngine$Companion;
public fun <init> ()V
public fun <init> (Laws/smithy/kotlin/runtime/http/engine/okhttp/OkHttpEngineConfig;)V
public fun <init> (Lokhttp3/OkHttpClient;)V
public synthetic fun getConfig ()Laws/smithy/kotlin/runtime/http/engine/HttpClientEngineConfig;
public fun getConfig ()Laws/smithy/kotlin/runtime/http/engine/okhttp/OkHttpEngineConfig;
public fun roundTrip (Laws/smithy/kotlin/runtime/operation/ExecutionContext;Laws/smithy/kotlin/runtime/http/request/HttpRequest;Lkotlin/coroutines/Continuation;)Ljava/lang/Object;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import aws.smithy.kotlin.runtime.InternalApi
import aws.smithy.kotlin.runtime.http.HttpCall
import aws.smithy.kotlin.runtime.http.config.EngineFactory
import aws.smithy.kotlin.runtime.http.engine.AlpnId
import aws.smithy.kotlin.runtime.http.engine.HttpClientEngine
import aws.smithy.kotlin.runtime.http.engine.HttpClientEngineBase
import aws.smithy.kotlin.runtime.http.engine.TlsContext
import aws.smithy.kotlin.runtime.http.engine.callContext
Expand Down Expand Up @@ -39,6 +40,11 @@ public class OkHttpEngine(
) : HttpClientEngineBase("OkHttp") {
public constructor() : this(OkHttpEngineConfig.Default)

private var userProvidedClient: OkHttpClient? = null
public constructor(client: OkHttpClient) : this(OkHttpEngineConfig.Default) {
userProvidedClient = client
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment: The refactor looks much better and addresses my main concern about unnecessary client instantiation. Thanks!

Nit:

we can't edit the primary constructor

I don't believe a constructor's "primaryness" affects the public API. It's a source code term which indicates it's the one that appears on the class declaration line, as opposed to those which appear in the body. The compiler also enforces that if a primary constructor exists then all secondary constructors must invoke it.

I believe this code could be further improved by moving the existing primary constructor to a secondary one and either a) making each secondary constructor fully set the initial state or b) adding a new private primary constructor to set state. As I noted above, my primary concern is addressed so I'll leave it up to you whether to continue iterating.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored a bit more for option b), what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

public companion object : EngineFactory<OkHttpEngineConfig.Builder, OkHttpEngine> {
/**
* Initializes a new [OkHttpEngine] via a DSL builder block
Expand All @@ -57,7 +63,10 @@ public class OkHttpEngine(
}

private val metrics = HttpClientMetrics(TELEMETRY_SCOPE, config.telemetryProvider)
private val client = config.buildClient(metrics, connectionMonitoringListener)
private val client: OkHttpClient by lazy {
userProvidedClient?.withMetrics(metrics, config)
?: config.buildClient(metrics, connectionMonitoringListener)
}

@OptIn(ExperimentalCoroutinesApi::class)
override suspend fun roundTrip(context: ExecutionContext, request: HttpRequest): HttpCall {
Expand Down Expand Up @@ -85,9 +94,11 @@ public class OkHttpEngine(

override fun shutdown() {
connectionMonitoringListener?.closeIfCloseable()
client.connectionPool.evictAll()
client.dispatcher.executorService.shutdown()
metrics.close()
if (userProvidedClient == null) {
client.connectionPool.evictAll()
client.dispatcher.executorService.shutdown()
}
}
}

Expand Down Expand Up @@ -170,6 +181,18 @@ public fun OkHttpEngineConfig.buildClient(
}.build()
}

// Configure a user-provided client to collect SDK metrics
private fun OkHttpClient.withMetrics(metrics: HttpClientMetrics, config: OkHttpEngineConfig) = newBuilder().apply {
eventListenerFactory { call ->
EventListenerChain(
listOf(
HttpEngineEventListener(connectionPool, config.hostResolver, dispatcher, metrics, call),
),
)
}
addInterceptor(MetricsInterceptor)
}.build()

private fun tlsConnectionSpec(tlsContext: TlsContext, cipherSuites: List<String>?): ConnectionSpec {
val minVersion = tlsContext.minVersion ?: TlsVersion.TLS_1_2
val okHttpTlsVersions = SdkTlsVersion
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/
package aws.smithy.kotlin.runtime.http.engine.okhttp

import aws.smithy.kotlin.runtime.content.ByteStream
import aws.smithy.kotlin.runtime.http.Headers
import aws.smithy.kotlin.runtime.http.HttpException
import aws.smithy.kotlin.runtime.http.HttpMethod
import aws.smithy.kotlin.runtime.http.SdkHttpClient
import aws.smithy.kotlin.runtime.http.request.HttpRequest
import aws.smithy.kotlin.runtime.http.toHttpBody
import aws.smithy.kotlin.runtime.net.url.Url
import kotlinx.coroutines.test.runTest
import okhttp3.OkHttpClient
import java.io.IOException
import kotlin.test.Test
import kotlin.test.assertFailsWith
import kotlin.test.assertTrue

class OkHttpEngineConfigTest {
@Test
fun testUserClient() = runTest {
val userClient = OkHttpClient.Builder().apply {
addInterceptor { throw DummyOkHttpClientException() }
}.build()

val engine = OkHttpEngine(userClient)
val sdkClient = SdkHttpClient(engine)

val data = "a".repeat(100)
val url = Url.parse("https://aws.amazon.com")
val request = HttpRequest(HttpMethod.POST, url, Headers.Empty, ByteStream.fromString(data).toHttpBody())

val ex = assertFailsWith<HttpException> {
sdkClient.call(request)
}
assertTrue(ex.cause is DummyOkHttpClientException)
}

private class DummyOkHttpClientException : IOException("Custom OkHttpClient interceptor was called")
}
Loading