From a494d8a232e1ccffea580dc265625a71c2e4d97e Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Fri, 26 Jun 2026 00:18:32 +0300 Subject: [PATCH] chore: simplify okhttp and jdkhttp transport adapters Mechanical, behaviour-preserving cleanups across the two transport adapters. None of these change anything on the wire. okhttp: - Hoist a single shared ClientLogger into the OkHttpTransport companion and have both the transport instance and the Builder read it, instead of each constructing its own logger for the identical category. - Drop the redundant default arguments on the shared empty RequestBody (toRequestBody(null, 0, 0) -> toRequestBody()); on a zero-length array these restate the extension's own defaults. - Correct the NonProxyHostSelector KDoc: it is a private nested class, not a "top-level private inner class". jdkhttp: - Fold the two create() factories into one @JvmOverloads method with a default responseTimeout; the one-arg form was the two-arg form with null. Both call arities stay for Java and Kotlin; the synthetic create$default is reflected in the committed .api snapshot. - Have KillSwitchInputStream extend FilterInputStream so the read / available delegation comes for free, keeping only the custom close() that cancels the writer and tears down both pipe ends. - Inline the single-use bufferToByteArray into eagerPublisher. - Apply the same NonProxyHostSelector KDoc correction. --- .../api/sdk-transport-jdkhttp.api | 1 + .../sdk/transport/jdkhttp/JdkHttpTransport.kt | 22 +++++-------- .../jdkhttp/internal/BodyPublishers.kt | 32 ++++++------------- .../sdk/transport/okhttp/OkHttpTransport.kt | 13 ++++++-- .../okhttp/internal/RequestAdapter.kt | 2 +- 5 files changed, 29 insertions(+), 41 deletions(-) diff --git a/sdk-transport-jdkhttp/api/sdk-transport-jdkhttp.api b/sdk-transport-jdkhttp/api/sdk-transport-jdkhttp.api index a2f85b01..5f38060c 100644 --- a/sdk-transport-jdkhttp/api/sdk-transport-jdkhttp.api +++ b/sdk-transport-jdkhttp/api/sdk-transport-jdkhttp.api @@ -23,6 +23,7 @@ public final class org/dexpace/sdk/transport/jdkhttp/JdkHttpTransport$Companion public final fun builder ()Lorg/dexpace/sdk/transport/jdkhttp/JdkHttpTransport$Builder; public final fun create (Ljava/net/http/HttpClient;)Lorg/dexpace/sdk/transport/jdkhttp/JdkHttpTransport; public final fun create (Ljava/net/http/HttpClient;Ljava/time/Duration;)Lorg/dexpace/sdk/transport/jdkhttp/JdkHttpTransport; + public static synthetic fun create$default (Lorg/dexpace/sdk/transport/jdkhttp/JdkHttpTransport$Companion;Ljava/net/http/HttpClient;Ljava/time/Duration;ILjava/lang/Object;)Lorg/dexpace/sdk/transport/jdkhttp/JdkHttpTransport; } public final class org/dexpace/sdk/transport/jdkhttp/JdkHttpTransport$HttpVersion : java/lang/Enum { diff --git a/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransport.kt b/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransport.kt index 3f24774f..85c60c04 100644 --- a/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransport.kt +++ b/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransport.kt @@ -236,23 +236,17 @@ public class JdkHttpTransport private constructor( * BYO factory: wrap a fully-configured [java.net.http.HttpClient]. The supplied * client is used verbatim — the SDK does not override connect timeout, redirect * policy, version, or executor, and [close] will NOT shut down this client (the - * caller owns its lifecycle). No per-request response timeout is applied. - */ - @JvmStatic - public fun create(client: java.net.http.HttpClient): JdkHttpTransport = - JdkHttpTransport(client, null, owned = false, ownedExecutor = null) - - /** - * BYO factory with a per-request response timeout. The timeout is applied to every - * outgoing request via [java.net.http.HttpRequest.Builder.timeout] — the JDK client - * does not expose a global response-timeout knob. Pass `null` for no per-request - * timeout; if a timeout is desirable but no specific value is needed, use the - * single-argument [create] overload (which captures `null`). + * caller owns its lifecycle). + * + * [responseTimeout], when non-`null`, is applied to every outgoing request via + * [java.net.http.HttpRequest.Builder.timeout] — the JDK client does not expose a + * global response-timeout knob. Defaults to `null` (no per-request timeout). */ @JvmStatic + @JvmOverloads public fun create( client: java.net.http.HttpClient, - responseTimeout: Duration?, + responseTimeout: Duration? = null, ): JdkHttpTransport = JdkHttpTransport(client, responseTimeout, owned = false, ownedExecutor = null) /** Returns a fresh [Builder] for SDK-managed [java.net.http.HttpClient] construction. */ @@ -493,7 +487,7 @@ public class JdkHttpTransport private constructor( * from [ProxyOptions] are honoured: hosts the [ProxyOptions] would bypass return * `Proxy.NO_PROXY`, everything else returns the configured proxy. * - * Defined as a top-level private inner class so the builder closure stays clean. + * Defined as a private nested class so the builder closure stays clean. */ private class NonProxyHostSelector( private val options: ProxyOptions, diff --git a/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/BodyPublishers.kt b/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/BodyPublishers.kt index 38b8f151..b69f733c 100644 --- a/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/BodyPublishers.kt +++ b/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/BodyPublishers.kt @@ -10,6 +10,7 @@ package org.dexpace.sdk.transport.jdkhttp.internal import org.dexpace.sdk.core.instrumentation.ClientLogger import org.dexpace.sdk.core.io.Io import java.io.Closeable +import java.io.FilterInputStream import java.io.IOException import java.io.InputStream import java.io.InterruptedIOException @@ -130,8 +131,9 @@ internal object BodyPublishers { * memory is released when the array is handed off. */ private fun eagerPublisher(body: SdkRequestBody): HttpRequest.BodyPublisher { - val bytes = bufferToByteArray(body) - return HttpRequest.BodyPublishers.ofByteArray(bytes) + val buffer = Io.provider.buffer() + body.writeTo(buffer) + return HttpRequest.BodyPublishers.ofByteArray(buffer.snapshot()) } /** @@ -245,16 +247,6 @@ internal object BodyPublishers { .log("piped body writer failed; reader side will see IOException") } - /** - * Drains [body] into a fresh in-memory [org.dexpace.sdk.core.io.Buffer] from the active - * [Io.provider] and snapshots it to a byte array. - */ - private fun bufferToByteArray(body: SdkRequestBody): ByteArray { - val buffer = Io.provider.buffer() - body.writeTo(buffer) - return buffer.snapshot() - } - /** * Wraps a subscription's [PipedInputStream] so that closing the read end also cancels the * writer [Future] and closes the [PipedOutputStream] write end. Without this, a JDK @@ -268,21 +260,15 @@ internal object BodyPublishers { private val pipeIn: PipedInputStream, private val pipeOut: PipedOutputStream, private val writer: Future<*>, - ) : InputStream() { - override fun read(): Int = pipeIn.read() - - override fun read( - b: ByteArray, - off: Int, - len: Int, - ): Int = pipeIn.read(b, off, len) - - override fun available(): Int = pipeIn.available() - + ) : FilterInputStream(pipeIn) { override fun close() { // Interrupt/cancel the writer first so a thread parked in PipedOutputStream.write // wakes up, then close both pipe ends. Order matters: closing pipeOut alone does // not unblock a writer mid-write, so the cancel(true) is what frees the thread. + // + // Deliberately does NOT call super.close(): FilterInputStream.close() would close + // only the wrapped read end, but teardown here must also cancel the writer and + // close the write end. closeQuietly(pipeIn) below covers the read end. writer.cancel(true) closeQuietly(pipeOut) closeQuietly(pipeIn) diff --git a/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransport.kt b/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransport.kt index 1e298c29..2cea0f82 100644 --- a/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransport.kt +++ b/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransport.kt @@ -68,7 +68,6 @@ public class OkHttpTransport private constructor( */ private val owned: Boolean, ) : HttpClient, AsyncHttpClient { - private val log: ClientLogger = ClientLogger("org.dexpace.sdk.transport.okhttp.OkHttpTransport") private val requestAdapter: RequestAdapter = RequestAdapter(log) private val responseAdapter: ResponseAdapter = ResponseAdapter(log) @@ -248,6 +247,15 @@ public class OkHttpTransport private constructor( } public companion object { + /** + * Logger shared by the transport and its [Builder]. A single instance is intentional: + * both call sites log under the same fully-qualified category, so one [ClientLogger] + * is the single source of truth for that category. `ClientLogger` wraps a thread-safe + * SLF4J `Logger` and carries no caller-specific state, so sharing it across the + * transport instance and every `Builder` is safe. + */ + private val log: ClientLogger = ClientLogger("org.dexpace.sdk.transport.okhttp.OkHttpTransport") + /** * BYO factory: wrap a fully-configured [OkHttpClient]. The supplied client is used * verbatim — the SDK does not override `followRedirects`, timeouts, or interceptors, @@ -271,7 +279,6 @@ public class OkHttpTransport private constructor( * — letting OkHttp follow redirects underneath would double-handle them. */ public class Builder internal constructor() : SdkBuilder { - private val log: ClientLogger = ClientLogger("org.dexpace.sdk.transport.okhttp.OkHttpTransport") private var connectTimeout: Duration? = null private var readTimeout: Duration? = null private var writeTimeout: Duration? = null @@ -419,7 +426,7 @@ public class OkHttpTransport private constructor( * [ProxyOptions] are honoured: hosts the [ProxyOptions] would bypass return * `Proxy.NO_PROXY`, everything else returns the configured proxy. * - * Defined as a top-level private inner class so the builder closure stays clean. + * Defined as a private nested class so the builder closure stays clean. */ private class NonProxyHostSelector( private val options: ProxyOptions, diff --git a/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt b/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt index 9769c264..8727a987 100644 --- a/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt +++ b/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt @@ -122,6 +122,6 @@ internal class RequestAdapter( * Shared zero-length body substituted for a body-less POST/PUT/PATCH. Immutable and * stateless, so a single instance is safe to reuse across requests and threads. */ - private val EMPTY_BODY: RequestBody = ByteArray(0).toRequestBody(null, 0, 0) + private val EMPTY_BODY: RequestBody = ByteArray(0).toRequestBody() } }