Race condition causing reuse of leaked connections

[ifedorenko]

OkHttp 5.3.2 fails with ProtocolException: Unexpected status line when client app leaks Response objects. The problem did not exist in 4.12 and may be related to this "Very small behavior change".

package com.salesforce.test.extensions

import mockwebserver3.Dispatcher
import mockwebserver3.MockResponse
import mockwebserver3.MockWebServer
import mockwebserver3.RecordedRequest
import okhttp3.OkHttpClient
import okhttp3.Request
import org.junit.jupiter.api.Test

internal class CorruptedConnectionTest {
    @Test
    fun test() {
        val server = MockWebServer()
        server.dispatcher = object : Dispatcher() {
            override fun dispatch(request: RecordedRequest): MockResponse {
                return MockResponse.Builder()
                    .code(200)
                    .body("body-"+request.url.pathSegments.single())
                    .build()
            }
        }
        server.start()

        val client = OkHttpClient.Builder().build()

        // leak a few responses
        client.newCall(Request.Builder().url(server.url("/a")).build()).execute()
        client.newCall(Request.Builder().url(server.url("/b")).build()).execute()

        // run gc to trigger connection pool leak detection logic
        System.gc()

        // run more requests
        client.newCall(Request.Builder().url(server.url("/c")).build()).execute().close()
        client.newCall(Request.Builder().url(server.url("/d")).build()).execute().close()
    }
}

The test above fails with the following exception

java.net.ProtocolException: Unexpected status line: body-aHTTP/1.1 200 OK

	at okhttp3.internal.http.StatusLine$Companion.parse(StatusLine.kt:73)
	at okhttp3.internal.http1.Http1ExchangeCodec.readResponseHeaders(Http1ExchangeCodec.kt:196)
	at okhttp3.internal.connection.Exchange.readResponseHeaders(Exchange.kt:117)
	at okhttp3.internal.http.CallServerInterceptor.intercept(CallServerInterceptor.kt:97)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:126)
	at okhttp3.internal.connection.ConnectInterceptor.intercept(ConnectInterceptor.kt:34)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:126)
	at okhttp3.internal.cache.CacheInterceptor.intercept(CacheInterceptor.kt:101)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:126)
	at okhttp3.internal.http.BridgeInterceptor.intercept(BridgeInterceptor.kt:85)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:126)
	at okhttp3.internal.http.RetryAndFollowUpInterceptor.intercept(RetryAndFollowUpInterceptor.kt:74)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:126)
	at okhttp3.internal.connection.RealCall.getResponseWithInterceptorChain$okhttp(RealCall.kt:226)
	at okhttp3.internal.connection.RealCall.execute(RealCall.kt:178)
	at com.salesforce.test.extensions.CorruptedConnectionTest.test(CorruptedConnectionTest.kt:36)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
        ...

[yschimke]

Thanks for the great repro. I'll take a look

[yschimke]

Yep - my change, noted by Jesse

https://github.com/square/okhttp/pull/7456/changes#r1059695271

Very small behavior change here - if we leak a Call that doesn‘t immediately mark the connection with noNewExchanges. That’s fine.

[yschimke]

The change means that after pruneAndGetAllocationCount detects and frees the leaked connection it's a race condition between when cleanupTask or callAcquirePooledConnection runs.

[yschimke]

Ahh, it's specifically an issue with multiple leaked connections. closeConnections updates references for all leaked connections, but closes only one.