Skip to content

Commit

Permalink
Fix ConnectionPoolTest bug (#8384)
Browse files Browse the repository at this point in the history
1. Advance taskFaker time forward to the present when the test starts.
2. Stop having multiple factories and taskfakers floating around.
  • Loading branch information
ean5533 committed Apr 24, 2024
1 parent 8d83f92 commit aede7c5
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 23 deletions.
8 changes: 2 additions & 6 deletions okhttp/src/test/java/okhttp3/FakeRoutePlanner.kt
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,9 @@ import okhttp3.internal.connection.RoutePlanner
import okhttp3.internal.connection.RoutePlanner.ConnectResult

class FakeRoutePlanner(
private val taskFaker: TaskFaker,
val factory: TestValueFactory = TestValueFactory(),
val taskFaker: TaskFaker = factory.taskFaker,
) : RoutePlanner, Closeable {
/**
* Note that we don't use the same [TaskFaker] for this factory. That way off-topic tasks like
* connection pool maintenance tasks don't add noise to route planning tests.
*/
val factory = TestValueFactory()
val pool = factory.newConnectionPool(routePlanner = this)
val events = LinkedBlockingDeque<String>()
var canceled = false
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import okhttp3.FakeRoutePlanner
import okhttp3.OkHttpClient
import okhttp3.Request
import okhttp3.TestUtil.awaitGarbageCollection
import okhttp3.TestValueFactory
import okhttp3.internal.concurrent.TaskRunner
import okhttp3.internal.connection.Locks.withLock
import okhttp3.internal.http2.Http2
Expand All @@ -39,7 +38,9 @@ import org.junit.jupiter.api.AfterEach
import org.junit.jupiter.api.Test

class ConnectionPoolTest {
private val factory = TestValueFactory()
private val routePlanner = FakeRoutePlanner()
private val factory = routePlanner.factory
private val taskFaker = routePlanner.taskFaker
private val peer = MockHttp2Peer()

/** The fake task runner prevents the cleanup runnable from being started. */
Expand All @@ -50,8 +51,6 @@ class ConnectionPoolTest {
private val addressC = factory.newAddress("c")
private val routeC1 = factory.newRoute(addressC)

private val routePlanner = FakeRoutePlanner(factory.taskFaker)

@AfterEach fun tearDown() {
factory.close()
peer.close()
Expand Down Expand Up @@ -205,7 +204,8 @@ class ConnectionPoolTest {
}

@Test fun connectionPreWarmingHttp1() {
val expireTime = factory.taskFaker.nanoTime + 1_000_000_000_000
taskFaker.advanceUntil(System.nanoTime())
val expireTime = taskFaker.nanoTime + 1_000_000_000_000

routePlanner.autoGeneratePlans = true
routePlanner.defaultConnectionIdleAtNanos = expireTime
Expand All @@ -219,19 +219,20 @@ class ConnectionPoolTest {
// Connections are replaced if they idle out or are evicted from the pool
evictAllConnections(pool)
assertThat(pool.connectionCount()).isEqualTo(2)
forceConnectionsToExpire(pool, routePlanner, expireTime)
forceConnectionsToExpire(pool, expireTime)
assertThat(pool.connectionCount()).isEqualTo(2)

// Excess connections aren't removed until they idle out, even if no longer needed
setPolicy(pool, address, ConnectionPool.AddressPolicy(1))
assertThat(pool.connectionCount()).isEqualTo(2)
forceConnectionsToExpire(pool, routePlanner, expireTime)
forceConnectionsToExpire(pool, expireTime)
assertThat(pool.connectionCount()).isEqualTo(1)
}

@Test fun connectionPreWarmingHttp2() {
val expireSooner = factory.taskFaker.nanoTime + 1_000_000_000_000
val expireLater = factory.taskFaker.nanoTime + 2_000_000_000_000
taskFaker.advanceUntil(System.nanoTime())
val expireSooner = taskFaker.nanoTime + 1_000_000_000_000
val expireLater = taskFaker.nanoTime + 2_000_000_000_000

routePlanner.autoGeneratePlans = true
val address = routePlanner.address
Expand All @@ -258,7 +259,7 @@ class ConnectionPoolTest {

// Increase the connection's max so that the new connection is no longer needed
updateMaxConcurrentStreams(http2Connection, 5)
forceConnectionsToExpire(pool, routePlanner, expireSooner)
forceConnectionsToExpire(pool, expireSooner)
assertThat(pool.connectionCount()).isEqualTo(1)
}

Expand All @@ -268,23 +269,22 @@ class ConnectionPoolTest {
policy: ConnectionPool.AddressPolicy,
) {
pool.setPolicy(address, policy)
routePlanner.factory.taskFaker.runTasks()
taskFaker.runTasks()
}

private fun evictAllConnections(pool: RealConnectionPool) {
pool.evictAll()
assertThat(pool.connectionCount()).isEqualTo(0)
routePlanner.factory.taskFaker.runTasks()
taskFaker.runTasks()
}

private fun forceConnectionsToExpire(
pool: RealConnectionPool,
routePlanner: FakeRoutePlanner,
expireTime: Long,
) {
val idleTimeNanos = expireTime + pool.keepAliveDurationNs
repeat(pool.connectionCount()) { pool.closeConnections(idleTimeNanos) }
routePlanner.factory.taskFaker.runTasks()
taskFaker.runTasks()
}

private fun connectHttp2(
Expand Down Expand Up @@ -316,7 +316,7 @@ class ConnectionPoolTest {
assertThat(ackFrame.streamId).isEqualTo(0)
assertThat(ackFrame.ack).isTrue()

routePlanner.factory.taskFaker.runTasks()
taskFaker.runTasks()

return connection
}
Expand All @@ -329,7 +329,7 @@ class ConnectionPoolTest {
settings[Settings.MAX_CONCURRENT_STREAMS] = amount
connection.readerRunnable.applyAndAckSettings(true, settings)
assertThat(connection.peerSettings[Settings.MAX_CONCURRENT_STREAMS]).isEqualTo(amount)
routePlanner.factory.taskFaker.runTasks()
taskFaker.runTasks()
}

/** Use a helper method so there's no hidden reference remaining on the stack. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,12 @@ import org.junit.jupiter.api.Test
internal class FastFallbackExchangeFinderTest {
private val taskFaker = TaskFaker()
private val taskRunner = taskFaker.taskRunner
private val routePlanner = FakeRoutePlanner(taskFaker)

/**
* Note that we don't use the same [TaskFaker] for this factory. That way off-topic tasks like
* connection pool maintenance tasks don't add noise to route planning tests.
*/
private val routePlanner = FakeRoutePlanner(taskFaker = taskFaker)
private val finder = FastFallbackExchangeFinder(routePlanner, taskRunner)

@AfterEach
Expand Down

0 comments on commit aede7c5

Please sign in to comment.