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 @@ -38,6 +38,27 @@ public class OkHttpEngine(
override val config: OkHttpEngineConfig,
) : HttpClientEngineBase("OkHttp") {
public constructor() : this(OkHttpEngineConfig.Default)
public constructor(client: OkHttpClient) : this(OkHttpEngineConfig.Default) {
this.manageClient = false

// destroy our client
this.client.apply {
connectionPool.evictAll()
dispatcher.executorService.shutdown()
}

// use the user-provided client, configured with metrics collection
this.client = client.newBuilder().apply {
eventListenerFactory { call ->
EventListenerChain(
listOf(
HttpEngineEventListener(client.connectionPool, config.hostResolver, client.dispatcher, metrics, call),
),
)
}
addInterceptor(MetricsInterceptor)
}.build()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Correctness: We shouldn't create an OkHttp client just to close it and replace it with the user's client. Let's refactor these constructors to avoid unnecessary client creation and to remove the need for var fields.

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 to initialize the client lazily. I think we still need one var since we can't edit the primary constructor


public companion object : EngineFactory<OkHttpEngineConfig.Builder, OkHttpEngine> {
/**
Expand All @@ -57,7 +78,8 @@ public class OkHttpEngine(
}

private val metrics = HttpClientMetrics(TELEMETRY_SCOPE, config.telemetryProvider)
private val client = config.buildClient(metrics, connectionMonitoringListener)
private var client = config.buildClient(metrics, connectionMonitoringListener)
private var manageClient = true // whether the SDK should manage the underlying OkHttpClient client

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

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

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