diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/RetryAndFollowUpInterceptor.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/RetryAndFollowUpInterceptor.kt index ca03741b6ca4..4f8782041f9c 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/RetryAndFollowUpInterceptor.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/RetryAndFollowUpInterceptor.kt @@ -76,6 +76,7 @@ class RetryAndFollowUpInterceptor : Interceptor { val isRecoverable = recover(e, call, chain, request) call.eventListener.retryDecision(call, e, isRecoverable) if (!isRecoverable) throw e.withSuppressed(recoveredFailures) + if (recoveredFailures.size > MAX_RECOVERED_FAILURES) throw e.withSuppressed(recoveredFailures) recoveredFailures += e newRoutePlanner = false continue @@ -353,5 +354,13 @@ class RetryAndFollowUpInterceptor : Interceptor { * curl, and wget follow 20; Safari follows 16; and HTTP/1.0 recommends 5. */ private const val MAX_FOLLOW_UPS = 20 + + /** + * How many connection failures should we retry before giving up? Without a cap, a route + * selector that keeps yielding usable routes (or keeps cycling through an exhausted set) + * lets this loop retry indefinitely, growing [recoveredFailures] without bound and repeatedly + * contending for locks a stuck attempt is still holding. + */ + private const val MAX_RECOVERED_FAILURES = 20 } } diff --git a/okhttp/src/jvmTest/kotlin/okhttp3/CallTest.kt b/okhttp/src/jvmTest/kotlin/okhttp3/CallTest.kt index e00c97a4e8e1..9029ef65b4b3 100644 --- a/okhttp/src/jvmTest/kotlin/okhttp3/CallTest.kt +++ b/okhttp/src/jvmTest/kotlin/okhttp3/CallTest.kt @@ -1316,6 +1316,39 @@ open class CallTest { recoverWhenRetryOnConnectionFailureIsTrue() } + @Test + fun doesNotRetryConnectionFailuresUnboundedly() { + // Provide more usable routes (25) than the cap allows. The cap check runs before the current + // failure is appended to recoveredFailures (so the thrown exception is never suppressed by + // itself), which means the throw fires one request later than MAX_RECOVERED_FAILURES retries + // plus the original attempt would suggest: 22, not 21. Mirrors DoubleInetAddressDns's + // technique of resolving the same real address repeatedly to guarantee fallback routes. + // Without the cap, recover() would keep finding a usable route and the call would consume + // closer to all 25 queued failures before route exhaustion finally stopped it. Asserting the + // exact request count, not just that some IOException was eventually thrown, is what actually + // proves the cap fired rather than route exhaustion coincidentally doing the same job. + val dispatcher = QueueDispatcher() + repeat(25) { + dispatcher.enqueue(MockResponse.Builder().onResponseStart(CloseSocket()).build()) + } + server.dispatcher = dispatcher + client = + client + .newBuilder() + .dns( + object : Dns { + override fun lookup(hostname: String): List { + val address = Dns.SYSTEM.lookup(hostname)[0] + return List(25) { address } + } + }, + ).build() + assertFailsWith { + client.newCall(Request.Builder().url(server.url("/")).build()).execute() + } + assertThat(server.requestCount).isEqualTo(22) + } + @Test fun noRecoverWhenRetryOnConnectionFailureIsFalse() { server.enqueue(MockResponse(body = "seed connection pool"))