Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Discussion] Add more connection failure tracking and tests. #7588

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

[Discussion] Add more connection failure tracking and tests. #7588

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d68d2c8
[Discussion] Add more connection failure tracking and tests.
yschimke Dec 30, 2022
c649a7c
Merge branch 'master' into route_database
yschimke Jan 5, 2023
95be570
Merge fix
yschimke Jan 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions okhttp/src/jvmMain/kotlin/okhttp3/internal/connection/ConnectPlan.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ class ConnectPlan(
success = true
return ConnectResult(plan = this)
} catch (e: IOException) {
client.routeDatabase.failed(route)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will also capture cases where the call was canceled. Hmm.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And looking at the other call to routeDatabase.failed(), I think we should notify the proxy selector.

This is from RealConnection.kt:

    // Tell the proxy selector when we fail to connect on a fresh connection.
    if (failedRoute.proxy.type() != Proxy.Type.DIRECT) {
      val address = failedRoute.address
      address.proxySelector.connectFailed(
        address.url.toUri(), failedRoute.proxy.address(), failure
      )
    }

Perhaps the nicest structure is to move that snippet out of RealConnection and into RouteDatabase, so the two are always in sync.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I wanted this to prompt discussion. I'm not clear that the current exception filtering is obviously right.

I'll split it out and use the opportunity to make a test.

eventListener.connectFailed(call, route.socketAddress, route.proxy, null, e)
return ConnectResult(plan = this, throwable = e)
} finally {
Expand Down
2 changes: 2 additions & 0 deletions okhttp/src/jvmTest/java/okhttp3/CallTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1022,6 +1022,8 @@ open class CallTest {
executeSynchronously(request)
.assertCode(200)
.assertBody("success!")

assertThat(client.routeDatabase.failedRoutes.single().proxy.address()).isEqualTo(server.toProxyAddress().address())
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only failing test when routeDatabase.failed is not called. Seems like we are missing coverage.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or perhaps, the coverage we do have (in RouteFailureTest) is overly pessimistic??

}

/** https://github.com/square/okhttp/issues/1801 */
Expand Down
117 changes: 37 additions & 80 deletions okhttp/src/jvmTest/java/okhttp3/RouteFailureTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ package okhttp3

import java.io.IOException
import java.net.InetAddress
import java.net.InetSocketAddress
import mockwebserver3.MockResponse
import mockwebserver3.MockWebServer
import mockwebserver3.SocketPolicy
Expand All @@ -27,11 +26,11 @@ import okhttp3.testing.PlatformRule
import okhttp3.tls.internal.TlsUtil.localhost
import org.assertj.core.api.Assertions.assertThat
import org.junit.jupiter.api.BeforeEach
import org.junit.jupiter.api.Test
import org.junit.jupiter.api.extension.RegisterExtension
import org.junit.jupiter.params.ParameterizedTest
import org.junit.jupiter.params.provider.ValueSource

class RouteFailureTest {
private lateinit var socketFactory: SpecificHostSocketFactory
private lateinit var client: OkHttpClient

@RegisterExtension
Expand All @@ -45,12 +44,16 @@ class RouteFailureTest {

private var listener = RecordingEventListener()

private var socketFactory: SpecificHostSocketFactory = SpecificHostSocketFactory(defaultAddress = null)

private val handshakeCertificates = localhost()

val dns = FakeDns()

val ipv4 = InetAddress.getByName("203.0.113.1")
val ipv6 = InetAddress.getByName("2001:db8:ffff:ffff:ffff:ffff:ffff:1")
val unresolvableIpv4 = InetAddress.getByName("198.51.100.1")
val unresolvableIpv6 = InetAddress.getByName("2001:db8:ffff:ffff:ffff:ffff:ffff:ffff")

val refusedStream = MockResponse(
socketPolicy = SocketPolicy.RESET_STREAM_AT_START,
Expand All @@ -66,7 +69,6 @@ class RouteFailureTest {
this.server1 = server
this.server2 = server2

socketFactory = SpecificHostSocketFactory(InetSocketAddress(server.hostName, server.port))

client = clientTestRule.newClientBuilder()
.dns(dns)
Expand All @@ -75,66 +77,11 @@ class RouteFailureTest {
.build()
}

@Test
fun http2OneBadHostOneGoodNoRetryOnConnectionFailure() {
enableProtocol(Protocol.HTTP_2)

val request = Request(server1.url("/"))

server1.enqueue(refusedStream)
server2.enqueue(bodyResponse)

dns[server1.hostName] = listOf(ipv6, ipv4)
socketFactory[ipv6] = server1.inetSocketAddress
socketFactory[ipv4] = server2.inetSocketAddress

client = client.newBuilder()
.fastFallback(false)
.apply {
retryOnConnectionFailure = false
}
.build()

executeSynchronously(request)
.assertFailureMatches("stream was reset: REFUSED_STREAM")

assertThat(client.routeDatabase.failedRoutes).isEmpty()
assertThat(server1.requestCount).isEqualTo(1)
assertThat(server2.requestCount).isEqualTo(0)
}

@Test
fun http2OneBadHostOneGoodRetryOnConnectionFailure() {
enableProtocol(Protocol.HTTP_2)

val request = Request(server1.url("/"))

server1.enqueue(refusedStream)
server1.enqueue(refusedStream)
server2.enqueue(bodyResponse)

dns[server1.hostName] = listOf(ipv6, ipv4)
socketFactory[ipv6] = server1.inetSocketAddress
socketFactory[ipv4] = server2.inetSocketAddress

client = client.newBuilder()
.fastFallback(false)
.apply {
retryOnConnectionFailure = true
}
.build()

executeSynchronously(request)
.assertBody("body")

assertThat(client.routeDatabase.failedRoutes).isEmpty()
// TODO check if we expect a second request to server1, before attempting server2
assertThat(server1.requestCount).isEqualTo(2)
assertThat(server2.requestCount).isEqualTo(1)
}

@Test
fun http2OneBadHostOneGoodNoRetryOnConnectionFailureFastFallback() {
@ParameterizedTest
@ValueSource(booleans = [true, false])
fun http2OneBadHostOneGoodNoRetryOnConnectionFailure(
fastFallback: Boolean
) {
enableProtocol(Protocol.HTTP_2)

val request = Request(server1.url("/"))
Expand All @@ -147,7 +94,7 @@ class RouteFailureTest {
socketFactory[ipv4] = server2.inetSocketAddress

client = client.newBuilder()
.fastFallback(true)
.fastFallback(fastFallback)
.apply {
retryOnConnectionFailure = false
}
Expand All @@ -161,8 +108,11 @@ class RouteFailureTest {
assertThat(server2.requestCount).isEqualTo(0)
}

@Test
fun http2OneBadHostOneGoodRetryOnConnectionFailureFastFallback() {
@ParameterizedTest
@ValueSource(booleans = [true, false])
fun http2OneBadHostOneGoodRetryOnConnectionFailure(
fastFallback: Boolean
) {
enableProtocol(Protocol.HTTP_2)

val request = Request(server1.url("/"))
Expand All @@ -176,7 +126,7 @@ class RouteFailureTest {
socketFactory[ipv4] = server2.inetSocketAddress

client = client.newBuilder()
.fastFallback(true)
.fastFallback(fastFallback)
.apply {
retryOnConnectionFailure = true
}
Expand All @@ -191,8 +141,11 @@ class RouteFailureTest {
assertThat(server2.requestCount).isEqualTo(1)
}

@Test
fun http2OneBadHostRetryOnConnectionFailure() {
@ParameterizedTest
@ValueSource(booleans = [true, false])
fun http2OneBadHostRetryOnConnectionFailure(
fastFallback: Boolean
) {
enableProtocol(Protocol.HTTP_2)

val request = Request(server1.url("/"))
Expand All @@ -204,7 +157,7 @@ class RouteFailureTest {
socketFactory[ipv6] = server1.inetSocketAddress

client = client.newBuilder()
.fastFallback(false)
.fastFallback(fastFallback)
.apply {
retryOnConnectionFailure = true
}
Expand All @@ -217,30 +170,34 @@ class RouteFailureTest {
assertThat(server1.requestCount).isEqualTo(1)
}

@Test
fun http2OneBadHostRetryOnConnectionFailureFastFallback() {
@ParameterizedTest
@ValueSource(booleans = [true, false])
fun http2OneUnresolvableHost(
fastFallback: Boolean
) {
enableProtocol(Protocol.HTTP_2)

val request = Request(server1.url("/"))

server1.enqueue(refusedStream)
server1.enqueue(refusedStream)
server1.enqueue(bodyResponse)

dns[server1.hostName] = listOf(ipv6)
dns[server1.hostName] = listOf(unresolvableIpv6, ipv6)
// Will remain unresolvable
// socketFactory[unresolvableIpv6] = unresolvableIpv6
socketFactory[ipv6] = server1.inetSocketAddress

client = client.newBuilder()
.fastFallback(true)
.fastFallback(fastFallback)
.apply {
retryOnConnectionFailure = true
}
.build()

executeSynchronously(request)
.assertFailureMatches("stream was reset: REFUSED_STREAM")
.assertCode(200)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooooh does your fix cause this to connect to a good host? That is PRETTY RAD


assertThat(client.routeDatabase.failedRoutes).isEmpty()
assertThat(server1.requestCount).isEqualTo(1)
val failedRoutes = client.routeDatabase.failedRoutes
assertThat(failedRoutes.single().socketAddress.address).isEqualTo(unresolvableIpv6)
}

private fun enableProtocol(protocol: Protocol) {
Expand Down