Don't re-use OkHttpClient outside Kotlin notebooks (#469)

Addresses #451 by isolating the OkHttpClient-sharing behavior to Kolin
notebooks.

Tests are added to prevent regressions for both notebooks (should share
client resources by default) and elsewhere (should not share client
resources by default).

# #451 copy

Raised by @mcumings. First worked around in
[eBay/metrics-for-develocity-plugin
code](https://github.com/eBay/metrics-for-develocity-plugin/blob/6748cd762ff8c1d6bf7b1f2e4001a46f956f940e/src/main/kotlin/com/ebay/plugins/metrics/develocity/service/DevelocityBuildService.kt#L50).
The workaround was since replicated in one of the library examples,
`example-gradle-task`:
[`DevelocityApiService`](https://github.com/gabrielfeo/develocity-api-kotlin/blob/main/examples/example-gradle-task/buildSrc/src/main/kotlin/build/logic/DevelocityApiService.kt#L19-L24).

OkHttpClient internal resources (via `OkHttpClient.newBuilder`) are
re-used by default to fit the Kotlin notebook use case, in which the
notebook cell that creates an API instance (`DevelocityApi.newInstance`)
may be ran multiple times, with or without changes, in the same Kotlin
kernel. If OkHttpClient resources were not re-used, this common scenario
would result in high resource usage due to several sets of connection
pools and executors being kept. At least in the case of variable renames
(e.g. one cell executes with `val api = [...]` then value renamed to
`val develocityApi = [...]`), the old `api` variable would not get
GC'ed, since the Kotlin kernel holds on to past cell execution
variables.

Once client resources are shut down or `DevelocityApi.shutdown` is
called, however, the client can no longer be used. In the case of usage
in a Gradle plugin, this leads to the following:

```kotlin
abstract class DevelocityApiService
    : DevelocityApi by DevelocityApi.newInstance(),
      BuildService<BuildServiceParameters.None>,
      AutoCloseable {

    override fun close() {
        shutdown()
    }
}
```

0. Build starts
1. Gradle constructs the `BuildService`. `DevelocityApi.newInstance`
without a specified `clientBuilder` will create a new builder out of a
[static and global client
instance](https://github.com/gabrielfeo/develocity-api-kotlin/blob/cbc135a9c0d125dbeb4abbbd6ed8b9c9ff4360e3/library/src/main/kotlin/com/gabrielfeo/develocity/api/internal/OkHttpClient.kt#L21)
that is meant to support the re-using in Kotlin notebooks
3. Build ends
4. Gradle calls `close`, leading to the static and global client's
resources to be closed

Any new builds with that Gradle daemon will lead to the service being
re-created without the "static and global client instance" being
re-initialized. Since a client cannot be used after its resources
(including the executor are closed). An "executor rejected" error is
thrown by OkHttp. Can be reproduced by checking out parent of this [fix
commit](a704a53).

### Workarounds:

- Define your own `clientBuilder` as in the fixed version of
[`DevelocityApiService`](https://github.com/gabrielfeo/develocity-api-kotlin/blob/main/examples/example-gradle-task/buildSrc/src/main/kotlin/build/logic/DevelocityApiService.kt#L19-L24)
- [Untested] Don't shutdown the client (thread pool and connections
would live on in the GradleDaemon process to be re-used by the next
build, which avoids the problem and makes subsequent API requests faster
at the cost of slightly increased memory usage by the idle daemon)

Author: gabrielfeo

examples/example-gradle-task/buildSrc/src/main/kotlin/build/logic/DevelocityApiService.kt

@@ -7,18 +7,11 @@ import org.gradle.api.services.BuildServiceParameters
 import okhttp3.OkHttpClient
 
 abstract class DevelocityApiService
-    : DevelocityApi by DevelocityApi.newInstance(config()),
+    : DevelocityApi by DevelocityApi.newInstance(),
       BuildService<BuildServiceParameters.None>,
       AutoCloseable {
 
     override fun close() {
         shutdown()
     }
 }
-
-private fun config() = Config(
-    // Necessary to accomodate Gradle's build service lifecycle because library
-    // uses a singleton OkHttpClient.Builder unless one is provided.
-    // See https://github.com/gabrielfeo/develocity-api-kotlin/issues/451
-    clientBuilder = OkHttpClient.Builder(),
-)

library/api/library.api

@@ -203,6 +203,26 @@ public final class com/gabrielfeo/develocity/api/extension/BuildsApiExtensionsKt
 	public static synthetic fun getGradleAttributesFlow$default (Lcom/gabrielfeo/develocity/api/BuildsApi;JLjava/lang/String;Ljava/lang/Long;Ljava/lang/String;Ljava/lang/String;Ljava/lang/Boolean;Ljava/lang/Integer;Lkotlinx/coroutines/CoroutineScope;Ljava/util/List;ILjava/lang/Object;)Lkotlinx/coroutines/flow/Flow;
 }
 
+public final class com/gabrielfeo/develocity/api/internal/FreshOkHttpClientBuilderFactory : com/gabrielfeo/develocity/api/internal/OkHttpClientBuilderFactory {
+	public fun <init> ()V
+	public fun create ()Lokhttp3/OkHttpClient$Builder;
+}
+
+public abstract interface class com/gabrielfeo/develocity/api/internal/OkHttpClientBuilderFactory {
+	public static final field Companion Lcom/gabrielfeo/develocity/api/internal/OkHttpClientBuilderFactory$Companion;
+	public abstract fun create ()Lokhttp3/OkHttpClient$Builder;
+}
+
+public final class com/gabrielfeo/develocity/api/internal/OkHttpClientBuilderFactory$Companion {
+	public final fun getDefault ()Lcom/gabrielfeo/develocity/api/internal/OkHttpClientBuilderFactory;
+	public final fun setDefault (Lcom/gabrielfeo/develocity/api/internal/OkHttpClientBuilderFactory;)V
+}
+
+public final class com/gabrielfeo/develocity/api/internal/SharedOkHttpClientBuilderFactory : com/gabrielfeo/develocity/api/internal/OkHttpClientBuilderFactory {
+	public fun <init> ()V
+	public fun create ()Lokhttp3/OkHttpClient$Builder;
+}
+
 public final class com/gabrielfeo/develocity/api/internal/auth/HttpBearerAuth : okhttp3/Interceptor {
 	public fun <init> ()V
 	public fun <init> (Ljava/lang/String;Ljava/lang/String;)V

library/src/integrationTest/kotlin/com/gabrielfeo/develocity/api/internal/jupyter/DevelocityApiJupyterIntegrationTest.kt

@@ -7,6 +7,8 @@ import org.jetbrains.kotlinx.jupyter.api.Code
 import org.jetbrains.kotlinx.jupyter.testkit.JupyterReplTestCase
 import kotlin.reflect.KVisibility
 import kotlin.test.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertNotEquals
 
 @ExperimentalStdlibApi
 class DevelocityApiJupyterIntegrationTest : JupyterReplTestCase() {
@@ -21,6 +23,24 @@ class DevelocityApiJupyterIntegrationTest : JupyterReplTestCase() {
         attrs["custom value name"]
     """)
 
+    @Test
+    fun `Given default clientBuilder, re-uses OkHttpClient resources`() {
+        execSuccess("val api = DevelocityApi.newInstance()")
+        execSuccess("val api2 = DevelocityApi.newInstance()")
+        val connectionPool1 = execRendered("api.config.clientBuilder.build().connectionPool.hashCode()")
+        val connectionPool2 = execRendered("api2.config.clientBuilder.build().connectionPool.hashCode()")
+        assertEquals(connectionPool1, connectionPool2)
+    }
+
+    @Test
+    fun `Given custom clientBuilder set, does not re-use OkHttpClient resources`() {
+        execSuccess("val api = DevelocityApi.newInstance(Config(clientBuilder = okhttp3.OkHttpClient.Builder()))")
+        execSuccess("val api2 = DevelocityApi.newInstance(Config(clientBuilder = okhttp3.OkHttpClient.Builder()))")
+        val connectionPool1 = execRendered("api.config.clientBuilder.build().connectionPool.hashCode()")
+        val connectionPool2 = execRendered("api2.config.clientBuilder.build().connectionPool.hashCode()")
+        assertNotEquals(connectionPool1, connectionPool2)
+    }
+
     @Test
     fun `imports all public classes`() {
         val classes = allPublicClassesRecursive("com.gabrielfeo.develocity.api")

library/src/main/kotlin/com/gabrielfeo/develocity/api/Config.kt

@@ -88,7 +88,7 @@ data class Config(
      * The default is to share resources only within the library, i.e. multiple `Config()` with
      * the default [clientBuilder] will already share resources.
      */
-    val clientBuilder: OkHttpClient.Builder = basicOkHttpClient.newBuilder(),
+    val clientBuilder: OkHttpClient.Builder = OkHttpClientBuilderFactory.default.create(),
 
     /**
      * Maximum amount of concurrent requests allowed. Further requests will be queued. By default,

library/src/main/kotlin/com/gabrielfeo/develocity/api/internal/OkHttpClient.kt

@@ -1,27 +1,17 @@
 package com.gabrielfeo.develocity.api.internal
 
-import com.gabrielfeo.develocity.api.Config
-import com.gabrielfeo.develocity.api.internal.auth.HttpBearerAuth
-import com.gabrielfeo.develocity.api.internal.caching.CacheEnforcingInterceptor
-import com.gabrielfeo.develocity.api.internal.caching.CacheHitLoggingInterceptor
+import com.gabrielfeo.develocity.api.*
+import com.gabrielfeo.develocity.api.internal.auth.*
+import com.gabrielfeo.develocity.api.internal.caching.*
 import okhttp3.Cache
-import okhttp3.Interceptor
 import okhttp3.OkHttpClient
 import okhttp3.logging.HttpLoggingInterceptor
 import okhttp3.logging.HttpLoggingInterceptor.Level.BASIC
 import okhttp3.logging.HttpLoggingInterceptor.Level.BODY
 import java.time.Duration
-import org.slf4j.Logger
 
 private const val HTTP_LOGGER_NAME = "com.gabrielfeo.develocity.api.OkHttpClient"
 
-/**
- * Base instance just so that multiple created [Config]s will share resources by default.
- */
-internal val basicOkHttpClient by lazy {
-    OkHttpClient.Builder().build()
-}
-
 /**
  * Builds the final `OkHttpClient` with a `Config`.
  */

library/src/main/kotlin/com/gabrielfeo/develocity/api/internal/OkHttpClientBuilderFactory.kt

@@ -0,0 +1,28 @@
+package com.gabrielfeo.develocity.api.internal
+
+import okhttp3.OkHttpClient
+
+interface OkHttpClientBuilderFactory {
+
+    companion object {
+        var default: OkHttpClientBuilderFactory = FreshOkHttpClientBuilderFactory()
+    }
+
+    fun create(): OkHttpClient.Builder
+}
+
+/**
+ * Creates a new [OkHttpClient.Builder] instance for every call to [create].
+ */
+class FreshOkHttpClientBuilderFactory : OkHttpClientBuilderFactory {
+    override fun create() = OkHttpClient.Builder()
+}
+
+/**
+ * Re-uses the same [OkHttpClient.Builder] instance for every call to [create], allowing for
+ * internal resources such as connection pools and threads to be shared between clients.
+ */
+class SharedOkHttpClientBuilderFactory : OkHttpClientBuilderFactory {
+    private val sharedClient: OkHttpClient by lazy { OkHttpClient.Builder().build() }
+    override fun create() = sharedClient.newBuilder()
+}

library/src/main/kotlin/com/gabrielfeo/develocity/api/internal/jupyter/DevelocityApiJupyterIntegration.kt

@@ -1,5 +1,6 @@
 package com.gabrielfeo.develocity.api.internal.jupyter
 
+import org.jetbrains.kotlinx.jupyter.api.ExecutionCallback
 import org.jetbrains.kotlinx.jupyter.api.KotlinKernelVersion
 import org.jetbrains.kotlinx.jupyter.api.libraries.LibraryDefinition
 
@@ -13,4 +14,13 @@ class DevelocityApiJupyterIntegration : LibraryDefinition {
         "com.gabrielfeo.develocity.api.model.*",
         "com.gabrielfeo.develocity.api.extension.*",
     )
+
+    override val init: List<ExecutionCallback<*>> = listOf(
+        {
+            execute("""
+                com.gabrielfeo.develocity.api.internal.OkHttpClientBuilderFactory.default =
+                    com.gabrielfeo.develocity.api.internal.SharedOkHttpClientBuilderFactory()
+            """.trimIndent())
+        }
+    )
 }

library/src/test/kotlin/com/gabrielfeo/develocity/api/OkHttpClientTest.kt

@@ -80,6 +80,14 @@ class OkHttpClientTest {
         }
     }
 
+    /**
+     * Tests against regressions of issue gabrielfeo/develocity-api-kotlin#451
+     */
+    @Test
+    fun `Given no clientBuilder specified, OkHttpClient resources not re-used`() {
+        assertNotEquals(buildClient().connectionPool, buildClient().connectionPool)
+    }
+
     private fun buildClient(
         vararg envVars: Pair<String, String?>,
         clientBuilder: OkHttpClient.Builder? = null,