Skip to content

improve OkHttpClient reuse & efficiency #1131

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
meladRaouf merged 1 commit into from
Mar 12, 2025
Merged

improve OkHttpClient reuse & efficiency #1131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions infra/network/src/main/java/com/simprints/infra/network/SimNetworkImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
package com.simprints.infra.network

import com.simprints.infra.network.apiclient.SimApiClientImpl
import com.simprints.infra.network.httpclient.DefaultOkHttpClientBuilder
import com.simprints.infra.network.httpclient.BuildOkHttpClientUseCase
import com.simprints.infra.network.url.BaseUrlProvider
import javax.inject.Inject
import javax.inject.Singleton
import kotlin.reflect.KClass

@Singleton
internal class SimNetworkImpl @Inject constructor(
private val baseUrlProvider: BaseUrlProvider,
private val okHttpClientBuilder: DefaultOkHttpClientBuilder,
private val buildOkHttpClient: BuildOkHttpClientUseCase,
) : SimNetwork {
override fun <T : SimRemoteInterface> getSimApiClient(
remoteInterface: KClass<T>,
Expand All @@ -17,7 +19,7 @@ internal class SimNetworkImpl @Inject constructor(
authToken: String?,
): SimNetwork.SimApiClient<T> = SimApiClientImpl(
remoteInterface,
okHttpClientBuilder,
buildOkHttpClient,
getApiBaseUrl(),
deviceId,
versionName,
Expand Down
13 changes: 4 additions & 9 deletions infra/network/src/main/java/com/simprints/infra/network/apiclient/SimApiClientImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,10 @@ import com.simprints.infra.network.exceptions.NetworkConnectionException
import com.simprints.infra.network.exceptions.RetryableCloudException
import com.simprints.infra.network.exceptions.SyncCloudIntegrationException
import com.simprints.infra.network.exceptions.isCausedFromBadNetworkConnection
import com.simprints.infra.network.httpclient.DefaultOkHttpClientBuilder
import com.simprints.infra.network.httpclient.BuildOkHttpClientUseCase
import com.simprints.infra.network.json.JsonHelper
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.withContext
import okhttp3.OkHttpClient
import retrofit2.HttpException
import retrofit2.Retrofit
import retrofit2.converter.jackson.JacksonConverterFactory
Expand All @@ -27,7 +26,7 @@ import kotlin.reflect.KClass
*/
internal class SimApiClientImpl<T : SimRemoteInterface>(
private val service: KClass<T>,
private val okHttpClientBuilder: DefaultOkHttpClientBuilder,
private val buildOkHttpClient: BuildOkHttpClientUseCase,
private val url: String,
private val deviceId: String,
private val versionName: String,
Expand All @@ -51,14 +50,10 @@ internal class SimApiClientImpl<T : SimRemoteInterface>(
.addConverterFactory(ScalarsConverterFactory.create())
.addConverterFactory(JacksonConverterFactory.create(JsonHelper.jackson))
.baseUrl(url)
.client(okHttpClientConfig.build())
.client(buildOkHttpClient(authToken, deviceId, versionName))
.build()
}

private val okHttpClientConfig: OkHttpClient.Builder by lazy {
okHttpClientBuilder.get(authToken, deviceId, versionName)
}

override suspend fun <V> executeCall(networkBlock: suspend (T) -> V): V = try {
retryIO(
times = attempts,
Expand Down Expand Up @@ -106,7 +101,7 @@ internal class SimApiClientImpl<T : SimRemoteInterface>(

private fun HttpException.parseEstimatedOutage(): Long? = try {
response()?.headers()?.get(HEADER_RETRY_AFTER)?.toLong()
} catch (e: NumberFormatException) {
} catch (_: NumberFormatException) {
null
}
}
48 changes: 41 additions & 7 deletions .../httpclient/DefaultOkHttpClientBuilder.kt → ...rk/httpclient/BuildOkHttpClientUseCase.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@ import okio.buffer
import java.io.IOException
import java.util.concurrent.TimeUnit
import javax.inject.Inject
import javax.inject.Singleton

internal class DefaultOkHttpClientBuilder @Inject constructor(
@Singleton
internal class BuildOkHttpClientUseCase @Inject constructor(
@ApplicationContext private val ctx: Context,
private val networkCache: Cache,
private val persistentLogger: PersistentLogger,
Expand All @@ -29,6 +31,8 @@ internal class DefaultOkHttpClientBuilder @Inject constructor(
const val DEVICE_ID_HEADER = "X-Device-ID"
const val AUTHORIZATION_HEADER = "Authorization"
const val USER_AGENT_HEADER = "User-Agent"
const val READ_TIMEOUT = 60L
const val WRITE_TIMEOUT = 60L

/**
* This header can be used to force a specific version of the API endpoint to be used.
Expand All @@ -40,20 +44,49 @@ internal class DefaultOkHttpClientBuilder @Inject constructor(
const val FORCE_VERSION_HEADER = "X-Force-Version"
}

fun get(
private var okHttpClient: OkHttpClient? = null
private var currentAuthToken: String? = null
Copy link
Copy Markdown
Contributor

@BurningAXE BurningAXE Mar 12, 2025

Choose a reason for hiding this comment

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

Just to double check - once logged in, we always use just one auth token for all calls going through OkHttp!?

Copy link
Copy Markdown
Collaborator Author

@meladRaouf meladRaouf Mar 12, 2025

Choose a reason for hiding this comment

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

Yes, We use the same firebase auth token in every authenticated api request
if the token changed then the BuildOkHttpClientUseCase will create another OK http client


private val lock = Any() // synchronization lock for the okHttpClient lazy property

/**
* Retrieves an instance of [OkHttpClient], ensuring that the same instance is reused unless the authentication token changes.
*
* This method is **thread-safe** using `synchronized(lock)`, ensuring that only one instance of `OkHttpClient` is created at a time.
* If the provided `authToken` differs from the current stored token, a **new OkHttpClient instance** is created.
* Otherwise, the existing client instance is reused to optimize memory usage and prevent OOM errors.
*
* @param authToken The authentication token used for secure API requests. If `null`, a non-authenticated client is used.
* @param deviceId A unique identifier for the device, added to the request headers.
* @param versionName The application version name, added to the request headers.
* @return An instance of [OkHttpClient] configured based on the provided parameters.
*
*/
operator fun invoke(
authToken: String? = null,
deviceId: String,
versionName: String,
): OkHttpClient.Builder = OkHttpClient
): OkHttpClient = synchronized(lock) {
if (okHttpClient == null || currentAuthToken != authToken) {
currentAuthToken = authToken
okHttpClient = buildOkHttpClient(deviceId, versionName)
}
okHttpClient!!
}

private fun buildOkHttpClient(
deviceId: String,
versionName: String,
) = OkHttpClient
.Builder()
.cache(networkCache)
.followRedirects(false)
.followSslRedirects(false)
.readTimeout(60, TimeUnit.SECONDS)
.writeTimeout(60, TimeUnit.SECONDS)
.readTimeout(READ_TIMEOUT, TimeUnit.SECONDS)
.writeTimeout(WRITE_TIMEOUT, TimeUnit.SECONDS)
.apply {
if (!authToken.isNullOrBlank()) {
addInterceptor(buildAuthenticationInterceptor(authToken))
if (!currentAuthToken.isNullOrBlank()) {
addInterceptor(buildAuthenticationInterceptor(currentAuthToken!!))
}
}.addNetworkInterceptor(ChuckerInterceptor.Builder(ctx).build())
.addInterceptor(buildDeviceIdInterceptor(deviceId))
Expand All @@ -64,6 +97,7 @@ internal class DefaultOkHttpClientBuilder @Inject constructor(
addInterceptor(buildSimberLoggingInterceptor())
}
}.addInterceptor(buildPersistentLoggerInterceptor())
.build()

private fun buildAuthenticationInterceptor(authToken: String) = Interceptor { chain ->
val newRequest = chain
Expand Down
4 changes: 2 additions & 2 deletions infra/network/src/test/java/com/simprints/infra/network/SimNetworkImplTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package com.simprints.infra.network

import com.simprints.infra.network.apiclient.SimApiClientImpl
import com.simprints.infra.network.httpclient.DefaultOkHttpClientBuilder
import com.simprints.infra.network.httpclient.BuildOkHttpClientUseCase
import com.simprints.infra.network.url.BaseUrlProvider
import io.mockk.MockKAnnotations
import io.mockk.impl.annotations.MockK
Expand All @@ -15,7 +15,7 @@ class SimNetworkImplTest {
private lateinit var baseUrlProvider: BaseUrlProvider

@MockK
private lateinit var okHttpClientBuilder: DefaultOkHttpClientBuilder
private lateinit var okHttpClientBuilder: BuildOkHttpClientUseCase

@Before
fun setUp() {
Expand Down
6 changes: 3 additions & 3 deletions infra/network/src/test/java/com/simprints/infra/network/apiclient/SimApiClientImplTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import com.simprints.infra.network.exceptions.ApiError
import com.simprints.infra.network.exceptions.BackendMaintenanceException
import com.simprints.infra.network.exceptions.NetworkConnectionException
import com.simprints.infra.network.exceptions.SyncCloudIntegrationException
import com.simprints.infra.network.httpclient.DefaultOkHttpClientBuilder
import com.simprints.infra.network.httpclient.BuildOkHttpClientUseCase
import com.simprints.logging.persistent.PersistentLogger
import com.simprints.testtools.common.syntax.assertThrows
import io.mockk.MockKAnnotations
Expand Down Expand Up @@ -38,15 +38,15 @@ class SimApiClientImplTest {
private lateinit var mockWebServer: MockWebServer
private lateinit var simApiClientImpl: SimApiClientImpl<FakeRetrofitInterface>

private lateinit var httpClientBuilder: DefaultOkHttpClientBuilder
private lateinit var httpClientBuilder: BuildOkHttpClientUseCase

@Before
fun setup() {
MockKAnnotations.init(this, relaxed = true)

mockWebServer = MockWebServer()
mockWebServer.start()
httpClientBuilder = DefaultOkHttpClientBuilder(
httpClientBuilder = BuildOkHttpClientUseCase(
mockk(),
networkCache,
persistentLogger,
Expand Down
95 changes: 47 additions & 48 deletions ...pclient/DefaultOkHttpClientBuilderTest.kt → ...ttpclient/BuildOkHttpClientUseCaseTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
package com.simprints.infra.network.httpclient

import android.content.Context
import com.google.common.truth.Truth.assertThat
import com.google.common.truth.Truth.*
import com.simprints.infra.network.httpclient.BuildOkHttpClientUseCase.Companion.AUTHORIZATION_HEADER
import com.simprints.infra.network.httpclient.BuildOkHttpClientUseCase.Companion.DEVICE_ID_HEADER
import com.simprints.infra.network.httpclient.BuildOkHttpClientUseCase.Companion.USER_AGENT_HEADER
import com.simprints.logging.persistent.PersistentLogger
import io.mockk.MockKAnnotations
import io.mockk.*
import io.mockk.impl.annotations.MockK
import okhttp3.MediaType.Companion.toMediaTypeOrNull
import okhttp3.OkHttpClient
Expand All @@ -15,7 +18,7 @@ import org.junit.After
import org.junit.Before
import org.junit.Test

class DefaultOkHttpClientBuilderTest {
class BuildOkHttpClientUseCaseTest {
// mock server to read http request parameters
private lateinit var mockWebServer: MockWebServer
private lateinit var okHttpClient: OkHttpClient
Expand All @@ -29,15 +32,15 @@ class DefaultOkHttpClientBuilderTest {
@MockK
lateinit var persistentLogger: PersistentLogger

private lateinit var okHttpBuilder: DefaultOkHttpClientBuilder
private lateinit var buildOkHttpClient: BuildOkHttpClientUseCase

@Before
fun setup() {
MockKAnnotations.init(this, relaxed = true)
mockWebServer = MockWebServer()
mockWebServer.start()

okHttpBuilder = DefaultOkHttpClientBuilder(ctx, networkCache, persistentLogger)
buildOkHttpClient = BuildOkHttpClientUseCase(ctx, networkCache, persistentLogger)
}

@After
Expand All @@ -52,21 +55,16 @@ class DefaultOkHttpClientBuilderTest {
val versionName = "cxxlvxzn.12.2049"

// create okHttp client using default builder
okHttpClient = okHttpBuilder
.get("", "", versionName)
.build()
okHttpClient = buildOkHttpClient("", "", versionName)

val mockHttpRequest = Request
.Builder()
.url(mockWebServer.url("/"))
.build()
val mockHttpRequest = Request.Builder().url(mockWebServer.url("/")).build()

// execute mock request
okHttpClient.newCall(mockHttpRequest).execute()
// read recorded request
val recordedRequest = mockWebServer.takeRequest()

assertThat(recordedRequest.getHeader(DefaultOkHttpClientBuilder.USER_AGENT_HEADER))
assertThat(recordedRequest.getHeader(USER_AGENT_HEADER))
.isEqualTo("SimprintsID/$versionName")
}

Expand All @@ -76,43 +74,33 @@ class DefaultOkHttpClientBuilderTest {

val deviceId = "symeAwxomedyvexeid"

okHttpClient = okHttpBuilder
.get("", deviceId, "")
.build()
okHttpClient = buildOkHttpClient("", deviceId, "")

val mockHttpRequest = Request
.Builder()
.url(mockWebServer.url("/"))
.build()
val mockHttpRequest = Request.Builder().url(mockWebServer.url("/")).build()

// execute mock request
okHttpClient.newCall(mockHttpRequest).execute()
// read recorded request
val recordedRequest = mockWebServer.takeRequest()

assertThat(recordedRequest.getHeader(DefaultOkHttpClientBuilder.DEVICE_ID_HEADER))
assertThat(recordedRequest.getHeader(DEVICE_ID_HEADER))
.isEqualTo(deviceId)
}

@Test
fun `should not include auth token in request headers, when auth token is null`() {
mockWebServer.enqueue(MockResponse())

okHttpClient = okHttpBuilder
.get(null, "", "")
.build()
okHttpClient = buildOkHttpClient(null, "", "")

val mockHttpRequest = Request
.Builder()
.url(mockWebServer.url("/"))
.build()
val mockHttpRequest = Request.Builder().url(mockWebServer.url("/")).build()

// execute mock request
okHttpClient.newCall(mockHttpRequest).execute()
// read recorded request
val recordedRequest = mockWebServer.takeRequest()

assertThat(recordedRequest.getHeader(DefaultOkHttpClientBuilder.AUTHORIZATION_HEADER)).isNull()
assertThat(recordedRequest.getHeader(AUTHORIZATION_HEADER)).isNull()
}

@Test
Expand All @@ -121,37 +109,50 @@ class DefaultOkHttpClientBuilderTest {

val authToken = "eyxSomeAwesomeAuth.TokenThatIsUsed.ForUnitTesting"

okHttpClient = okHttpBuilder
.get(authToken, "", "")
.build()
okHttpClient = buildOkHttpClient(authToken, "", "")

val mockHttpRequest = Request
.Builder()
.url(mockWebServer.url("/"))
.build()
val mockHttpRequest = Request.Builder().url(mockWebServer.url("/")).build()

// execute mock request
okHttpClient.newCall(mockHttpRequest).execute()
// read recorded request
val recordedRequest = mockWebServer.takeRequest()

assertThat(recordedRequest.getHeader(DefaultOkHttpClientBuilder.AUTHORIZATION_HEADER))
assertThat(recordedRequest.getHeader(AUTHORIZATION_HEADER))
.isEqualTo("Bearer $authToken")
}

@Test
fun `should return same instance for same token`() {
val client1 = buildOkHttpClient(authToken = "token123", deviceId = "12345", versionName = "1.0")
val client2 = buildOkHttpClient(authToken = "token123", deviceId = "12345", versionName = "1.0")

assertThat(client1).isSameInstanceAs(client2)
}

@Test
fun `should create new client when authToken changes`() {
val client1 = buildOkHttpClient(authToken = null, deviceId = "12345", versionName = "1.0")
val client2 = buildOkHttpClient(authToken = "token2", deviceId = "12345", versionName = "1.0")
val client3 = buildOkHttpClient(authToken = "token3", deviceId = "12345", versionName = "1.0")

assertThat(client1).isNotSameInstanceAs(client2)
assertThat(client2).isNotSameInstanceAs(client3)
assertThat(client1).isNotSameInstanceAs(client3)
}

@Test
fun `should not compress request body if gzip header not provided`() {
mockWebServer.enqueue(MockResponse())

okHttpClient = okHttpBuilder
.get(null, "", "")
.build()
okHttpClient = buildOkHttpClient(null, "", "")

val mockHttpRequest = Request
.Builder()
.url(mockWebServer.url("/"))
.post("some request body".toRequestBody("text/plain".toMediaTypeOrNull()))
.build()
val mockHttpRequest =
Request
.Builder()
.url(mockWebServer.url("/"))
.post("some request body".toRequestBody("text/plain".toMediaTypeOrNull()))
.build()

// execute mock request
okHttpClient.newCall(mockHttpRequest).execute()
Expand All @@ -165,9 +166,7 @@ class DefaultOkHttpClientBuilderTest {
fun `should compress request body if gzip header provided`() {
mockWebServer.enqueue(MockResponse())

okHttpClient = okHttpBuilder
.get(null, "", "")
.build()
okHttpClient = buildOkHttpClient(null, "", "")

val mockHttpRequest = Request
.Builder()
Expand Down