fix: inconsistent custom okhttp configuration

[OmarAlJarrah]

Situation

In pull-request#857, we proposed a new feature that enables users to inject their own OkHttp client when configuring the SDK client.

A bug was reported to us that custom Dispatcher configurations on OkHttpClient instances are being overridden by OkHttp's default configurations. Look the example below:

val okhttp = OkHttpClient.Builder()
    .dispatcher(
        Dispatcher().apply {
            maxRequests = 5000
            maxRequestsPerHost = 1000
        }
    )
    .build()

val client = RapidClient.builderWithHttpClient()
    .okHttpClient(okHttpClient = okhttp)
    .key("key")
    .secret("secret")
    .build()

In the previous example, we expect maxRequests and maxRequestsPerHost values to be configured properly to their given values. Unfortunately, that's not the case, and configurations are overridden to OkHttp's default values.

We pass custom OkHttpClient instance to Ktor through the HttpClientEngineFactory utility, which enables us to create and configure instances of the OkHttpEngine.

abstract class BaseRapidClient(
    namespace: String,
    clientConfiguration: RapidClientConfiguration,
    httpClientEngine: HttpClientEngine = DEFAULT_HTTP_CLIENT_ENGINE,
) : Client(namespace) {
    // code... code... code... 
    private val engine: HttpClientEngine =
        _configurationProvider.okHttpClient?.let {
            OkHttp.create {
                preconfigured = it
            }
        } ?: httpClientEngine
    // code... code... code... 
}

When we pass an OkHttpClient instance to the Ktor client, Ktor attempts to clone our OkHttpClient instance, then executing our configuration code-block. The issue is that Ktor creates a new dispatcher instance and setting it on the new instance after cloning our client. Looking the Ktor code-base, we can observe that behavior in the following code:

public class OkHttpEngine(override val config: OkHttpConfig) : HttpClientEngineBase("ktor-okhttp") {
    // code... code... code... 
    private val clientCache = createLRUCache(::createOkHttpClient, {}, config.clientCacheSize)

    // code... code... code... 
    private fun createOkHttpClient(timeoutExtension: HttpTimeout.HttpTimeoutCapabilityConfiguration?): OkHttpClient {
        val builder = (config.preconfigured ?: okHttpClientPrototype).newBuilder()

        builder.dispatcher(Dispatcher())
        builder.apply(config.config)
        config.proxy?.let { builder.proxy(it) }
        timeoutExtension?.let {
            builder.setupTimeoutAttributes(it)
        }

        return builder.build()
    }
}

Breaking down the code, we can observe the following facts:

1. Our OkHttpClient instance is cloned and transformed into an instance of OkHttpBuilder.

val builder = (config.preconfigured ?: okHttpClientPrototype).newBuilder()

2. A new Dispatcher instance is created and set on the new builder.

builder.dispatcher(Dispatcher())

3. Our custom-configuration block is executed on the OkHttpBuilder instance.

builder.apply(config.config)

As a result, our custom Dispatcher instance is overridden by a new instance holding the default values set in the Dispatcher class.

Task

In order to resolve the issue and persist custom Dispatcher instances, we need to make sure the the Dispatcher instances is set again properly in the ustom-configuration block. We do not need to care about other values and configurations since they are cloned properly and not overridden later on.

Action

For each client, we made sure Dispatcher instances are set properly on the custom-configuration block level.

OkHttp.create {
     config {
         preconfigured = it
         dispatcher(it.dispatcher)
     }
}

Testing

Verified manually. It would be hard to unit-test setting custom OkHttpClient instances because the instance is set to be inaccessible (private-visibility).

Results

Custom Dispatcher instances are now preserved and respected. Given the following configuration as a sample:

val okhttp = OkHttpClient.Builder()
    .dispatcher(
        Dispatcher().apply {
            maxRequests = 5000
            maxRequestsPerHost = 1000
        }
    )
    .build()

val client = RapidClient.builderWithHttpClient()
    .okHttpClient(okHttpClient = okhttp)
    .key("key")
    .secret("secret")
    .build()

We can observe that our configurations are respected and preserved for each request.

Notes

NaN

[mohnoor94]

You may test the behaviour by setting some config (e,g, timeout) on a custom client.

[OmarAlJarrah]

You may test the behaviour by setting some config (e,g, timeout) on a custom client.

We have agreed to take this conversation into a new scope, potentially in another PR.

Please create tickets for the things you agreed on.