From a2f95157df9362ca43017b7520a5391b0282dbe2 Mon Sep 17 00:00:00 2001 From: Jacob Halsey Date: Wed, 16 Jul 2025 16:27:01 +0100 Subject: [PATCH 1/2] doExtensiveHealthChecks based on isOneShot instead of method --- .../internal/connection/CallConnectionUser.kt | 2 +- .../jvmTest/kotlin/okhttp3/CallKotlinTest.kt | 43 ++++++++++++++++--- 2 files changed, 37 insertions(+), 8 deletions(-) diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/connection/CallConnectionUser.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/connection/CallConnectionUser.kt index 5f2c7a3158b8..c70b2bbefe74 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/connection/CallConnectionUser.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/connection/CallConnectionUser.kt @@ -116,7 +116,7 @@ internal class CallConnectionUser( connection.connectionListener.noNewExchanges(connection) } - override fun doExtensiveHealthChecks(): Boolean = chain.request.method != "GET" + override fun doExtensiveHealthChecks(): Boolean = chain.request.body?.isOneShot() ?: false override fun isCanceled(): Boolean = call.isCanceled() diff --git a/okhttp/src/jvmTest/kotlin/okhttp3/CallKotlinTest.kt b/okhttp/src/jvmTest/kotlin/okhttp3/CallKotlinTest.kt index 887a209f8f77..cf1c27443f92 100644 --- a/okhttp/src/jvmTest/kotlin/okhttp3/CallKotlinTest.kt +++ b/okhttp/src/jvmTest/kotlin/okhttp3/CallKotlinTest.kt @@ -16,6 +16,7 @@ package okhttp3 import assertk.assertThat +import assertk.assertions.contains import assertk.assertions.isEqualTo import assertk.assertions.isInstanceOf import assertk.assertions.isNotSameAs @@ -23,6 +24,7 @@ import java.io.IOException import java.net.Proxy import java.security.cert.X509Certificate import java.time.Duration +import kotlin.test.assertFails import kotlin.test.assertFailsWith import mockwebserver3.MockResponse import mockwebserver3.MockWebServer @@ -30,7 +32,6 @@ import mockwebserver3.SocketEffect.CloseSocket import mockwebserver3.junit5.StartStop import okhttp3.Headers.Companion.headersOf import okhttp3.MediaType.Companion.toMediaType -import okhttp3.RequestBody.Companion.toRequestBody import okhttp3.TestUtil.assertSuppressed import okhttp3.internal.DoubleInetAddressDns import okhttp3.internal.connection.RealConnection @@ -43,6 +44,8 @@ import org.junit.jupiter.api.Assertions.assertEquals import org.junit.jupiter.api.Test import org.junit.jupiter.api.Timeout import org.junit.jupiter.api.extension.RegisterExtension +import org.junit.jupiter.params.ParameterizedTest +import org.junit.jupiter.params.provider.ValueSource import org.junitpioneer.jupiter.RetryingTest @Timeout(30) @@ -183,13 +186,16 @@ class CallKotlinTest { } } - @Test - fun staleConnectionNotReusedForNonIdempotentRequest() { + /** OneShot requests cannot be retried, so it is important to healthcheck the connection first */ + @ParameterizedTest + @ValueSource(booleans = [true, false]) + fun staleConnectionNotReusedForOneShotRequests(isOneShot: Boolean) { // Capture the connection so that we can later make it stale. var connection: RealConnection? = null client = client .newBuilder() + .retryOnConnectionFailure(false) .addNetworkInterceptor( Interceptor { chain -> connection = chain.connection() as RealConnection @@ -223,11 +229,34 @@ class CallKotlinTest { val requestB = Request( url = server.url("/"), - body = "b".toRequestBody("text/plain".toMediaType()), + body = object : RequestBody() { + override fun contentType(): MediaType? { + return "text/plain".toMediaType() + } + + override fun writeTo(sink: BufferedSink) { + sink.write("b".toByteArray()) + } + + override fun isOneShot(): Boolean { + return isOneShot + } + }, ) - val responseB = client.newCall(requestB).execute() - assertThat(responseB.body.string()).isEqualTo("b") - assertThat(server.takeRequest().exchangeIndex).isEqualTo(0) + if (isOneShot) { + // extensiveHealthcheck on the stale connection will create a new connection + val responseB = client.newCall(requestB).execute() + assertThat(responseB.body.string()).isEqualTo("b") + assertThat(server.takeRequest().exchangeIndex).isEqualTo(0) + } else { + // extensiveHealthcheck is skipped because this request could be retried + // However the exception is thrown because retryOnConnectionFailure=false + val throwable = assertFails { + client.newCall(requestB).execute() + } + assertThat(throwable.message!!).contains("unexpected end of stream") + } + } /** Confirm suppressed exceptions that occur while connecting are returned. */ From dc734dd8aa08aad237b4fc12752463c032482285 Mon Sep 17 00:00:00 2001 From: Jacob Halsey Date: Mon, 21 Jul 2025 14:28:32 +0100 Subject: [PATCH 2/2] Fix style --- .../jvmTest/kotlin/okhttp3/CallKotlinTest.kt | 31 +++++++++---------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/okhttp/src/jvmTest/kotlin/okhttp3/CallKotlinTest.kt b/okhttp/src/jvmTest/kotlin/okhttp3/CallKotlinTest.kt index cf1c27443f92..82228bb07605 100644 --- a/okhttp/src/jvmTest/kotlin/okhttp3/CallKotlinTest.kt +++ b/okhttp/src/jvmTest/kotlin/okhttp3/CallKotlinTest.kt @@ -229,19 +229,16 @@ class CallKotlinTest { val requestB = Request( url = server.url("/"), - body = object : RequestBody() { - override fun contentType(): MediaType? { - return "text/plain".toMediaType() - } - - override fun writeTo(sink: BufferedSink) { - sink.write("b".toByteArray()) - } - - override fun isOneShot(): Boolean { - return isOneShot - } - }, + body = + object : RequestBody() { + override fun contentType(): MediaType? = "text/plain".toMediaType() + + override fun writeTo(sink: BufferedSink) { + sink.write("b".toByteArray()) + } + + override fun isOneShot(): Boolean = isOneShot + }, ) if (isOneShot) { // extensiveHealthcheck on the stale connection will create a new connection @@ -251,12 +248,12 @@ class CallKotlinTest { } else { // extensiveHealthcheck is skipped because this request could be retried // However the exception is thrown because retryOnConnectionFailure=false - val throwable = assertFails { - client.newCall(requestB).execute() - } + val throwable = + assertFails { + client.newCall(requestB).execute() + } assertThat(throwable.message!!).contains("unexpected end of stream") } - } /** Confirm suppressed exceptions that occur while connecting are returned. */