From 9849ab8e3bff188ff07239a19cae83b203bb6e47 Mon Sep 17 00:00:00 2001 From: Jesse Wilson Date: Thu, 26 Sep 2019 22:04:58 -0400 Subject: [PATCH] Create OkHttpClient instances eagerly in tests We used to do this lazily because OkHttpClient instances were somewhat heavy: each standalone instance held its own ExecutorService for the connection pool. Now that we have TaskRunner each instance is much more lightweight and the drawbacks of creating instances eagerly is negligible. --- .../internal/sse/EventSourceHttpTest.java | 7 +-- .../main/java/okhttp3/OkHttpClientTestRule.kt | 61 ++++++------------- .../okhttp3/internal/cache/DiskLruCache.kt | 2 +- .../src/test/java/okhttp3/CallKotlinTest.kt | 7 +-- okhttp/src/test/java/okhttp3/CallTest.java | 7 +-- .../java/okhttp3/ConnectionReuseTest.java | 7 +-- okhttp/src/test/java/okhttp3/ConscryptTest.kt | 3 +- okhttp/src/test/java/okhttp3/CookiesTest.java | 7 +-- .../src/test/java/okhttp3/DispatcherTest.java | 9 ++- okhttp/src/test/java/okhttp3/DuplexTest.java | 7 +-- .../test/java/okhttp3/EventListenerTest.java | 8 +-- .../test/java/okhttp3/InterceptorTest.java | 7 +-- okhttp/src/test/java/okhttp3/OpenJSSETest.kt | 6 +- .../test/java/okhttp3/URLConnectionTest.java | 3 +- .../okhttp3/WholeOperationTimeoutTest.java | 7 +-- .../internal/ws/WebSocketHttpTest.java | 25 +++----- 16 files changed, 51 insertions(+), 122 deletions(-) diff --git a/okhttp-sse/src/test/java/okhttp3/internal/sse/EventSourceHttpTest.java b/okhttp-sse/src/test/java/okhttp3/internal/sse/EventSourceHttpTest.java index be0e5e69d..d96f38e84 100644 --- a/okhttp-sse/src/test/java/okhttp3/internal/sse/EventSourceHttpTest.java +++ b/okhttp-sse/src/test/java/okhttp3/internal/sse/EventSourceHttpTest.java @@ -24,7 +24,6 @@ import okhttp3.mockwebserver.MockWebServer; import okhttp3.sse.EventSource; import okhttp3.sse.EventSources; import org.junit.After; -import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -35,11 +34,7 @@ public final class EventSourceHttpTest { @Rule public final OkHttpClientTestRule clientTestRule = new OkHttpClientTestRule(); private final EventSourceRecorder listener = new EventSourceRecorder(); - private OkHttpClient client; - - @Before public void setUp() { - client = clientTestRule.newClient(); - } + private OkHttpClient client = clientTestRule.newClient(); @After public void after() { listener.assertExhausted(); diff --git a/okhttp-testing-support/src/main/java/okhttp3/OkHttpClientTestRule.kt b/okhttp-testing-support/src/main/java/okhttp3/OkHttpClientTestRule.kt index 1315914a3..ed0ef26d7 100644 --- a/okhttp-testing-support/src/main/java/okhttp3/OkHttpClientTestRule.kt +++ b/okhttp-testing-support/src/main/java/okhttp3/OkHttpClientTestRule.kt @@ -22,31 +22,36 @@ import org.junit.rules.TestRule import org.junit.runner.Description import org.junit.runners.model.Statement import java.net.InetAddress -import java.util.concurrent.ConcurrentLinkedDeque import java.util.concurrent.TimeUnit /** Apply this rule to tests that need an OkHttpClient instance. */ class OkHttpClientTestRule : TestRule { private val clientEventsList = mutableListOf() - private var prototype: OkHttpClient? = null + private var testClient: OkHttpClient? = null /** - * Returns an OkHttpClient for all tests to use as a starting point. + * Returns an OkHttpClient for tests to use as a starting point. * - * The shared instance allows all tests to share a single connection pool, which prevents idle - * connections from consuming unnecessary resources while connections wait to be evicted. + * The returned client installs a default event listener that gathers debug information. This will + * be logged if the test fails. * * This client is also configured to be slightly more deterministic, returning a single IP * address for all hosts, regardless of the actual number of IP addresses reported by DNS. */ fun newClient(): OkHttpClient { - return newClientBuilder().build() + var client = testClient + if (client == null) { + client = OkHttpClient.Builder() + .dns(SINGLE_INET_ADDRESS_DNS) // Prevent unexpected fallback addresses. + .eventListener(ClientRuleEventListener { addEvent(it) }) + .build() + testClient = client + } + return client } fun newClientBuilder(): OkHttpClient.Builder { - return checkNotNull(prototype) { "don't create clients in test initialization!" } - .newBuilder() - .eventListener(ClientRuleEventListener { addEvent(it) }) + return newClient().newBuilder() } @Synchronized private fun addEvent(it: String) { @@ -54,7 +59,7 @@ class OkHttpClientTestRule : TestRule { } fun ensureAllConnectionsReleased() { - prototype?.let { + testClient?.let { val connectionPool = it.connectionPool connectionPool.evictAll() assertThat(connectionPool.connectionCount()).isEqualTo(0) @@ -72,7 +77,6 @@ class OkHttpClientTestRule : TestRule { override fun apply(base: Statement, description: Description): Statement { return object : Statement() { override fun evaluate() { - acquireClient() try { base.evaluate() logEventsIfFlaky(description) @@ -86,15 +90,8 @@ class OkHttpClientTestRule : TestRule { } } - private fun acquireClient() { - prototype = prototypes.poll() ?: freshClient() - } - private fun releaseClient() { - prototype?.let { - prototypes.push(it) - prototype = null - } + testClient?.dispatcher?.executorService?.shutdown() } } } @@ -119,40 +116,16 @@ class OkHttpClientTestRule : TestRule { } } - /** - * Called if a test is known to be leaky. - */ - fun abandonClient() { - prototype?.let { - prototype = null - it.dispatcher.executorService.shutdownNow() - it.connectionPool.evictAll() - } - } - companion object { - /** - * Quick and dirty pool of OkHttpClient instances. Each has its own independent dispatcher and - * connection pool. This way we can reuse expensive resources while preventing concurrent tests - * from interfering with each other. - */ - internal val prototypes = ConcurrentLinkedDeque() - /** * A network that resolves only one IP address per host. Use this when testing route selection * fallbacks to prevent the host machine's various IP addresses from interfering. */ - internal val SINGLE_INET_ADDRESS_DNS = object : Dns { + private val SINGLE_INET_ADDRESS_DNS = object : Dns { override fun lookup(hostname: String): List { val addresses = Dns.SYSTEM.lookup(hostname) return listOf(addresses[0]) } } - - private fun freshClient(): OkHttpClient { - return OkHttpClient.Builder() - .dns(SINGLE_INET_ADDRESS_DNS) // Prevent unexpected fallback addresses. - .build() - } } } diff --git a/okhttp/src/main/java/okhttp3/internal/cache/DiskLruCache.kt b/okhttp/src/main/java/okhttp3/internal/cache/DiskLruCache.kt index 60086c6fe..b9321de78 100644 --- a/okhttp/src/main/java/okhttp3/internal/cache/DiskLruCache.kt +++ b/okhttp/src/main/java/okhttp3/internal/cache/DiskLruCache.kt @@ -94,7 +94,7 @@ class DiskLruCache internal constructor( maxSize: Long, /** Used for asynchronous journal rebuilds. */ - taskRunner: TaskRunner = TaskRunner.INSTANCE + taskRunner: TaskRunner ) : Closeable, Flushable { /** The maximum number of bytes that this cache should use to store its data. */ @get:Synchronized @set:Synchronized var maxSize: Long = maxSize diff --git a/okhttp/src/test/java/okhttp3/CallKotlinTest.kt b/okhttp/src/test/java/okhttp3/CallKotlinTest.kt index df7e3f2f8..a1c1f970d 100644 --- a/okhttp/src/test/java/okhttp3/CallKotlinTest.kt +++ b/okhttp/src/test/java/okhttp3/CallKotlinTest.kt @@ -19,7 +19,6 @@ import okhttp3.mockwebserver.MockResponse import okhttp3.mockwebserver.MockWebServer import okhttp3.testing.PlatformRule import org.assertj.core.api.Assertions.assertThat -import org.junit.Before import org.junit.Rule import org.junit.Test import org.junit.rules.TestRule @@ -32,11 +31,7 @@ class CallKotlinTest { @JvmField @Rule val server = MockWebServer() @JvmField @Rule val clientTestRule = OkHttpClientTestRule() - private lateinit var client: OkHttpClient - - @Before fun setUp() { - client = clientTestRule.newClient() - } + private var client = clientTestRule.newClient() @Test fun legalToExecuteTwiceCloning() { diff --git a/okhttp/src/test/java/okhttp3/CallTest.java b/okhttp/src/test/java/okhttp3/CallTest.java index 857bea310..2a50b527b 100644 --- a/okhttp/src/test/java/okhttp3/CallTest.java +++ b/okhttp/src/test/java/okhttp3/CallTest.java @@ -110,7 +110,9 @@ public final class CallTest { private RecordingEventListener listener = new RecordingEventListener(); private HandshakeCertificates handshakeCertificates = localhost(); - private OkHttpClient client; + private OkHttpClient client = clientTestRule.newClientBuilder() + .eventListener(listener) + .build(); private RecordingCallback callback = new RecordingCallback(); private TestLogHandler logHandler = new TestLogHandler(); private Cache cache = new Cache(new File("/cache/"), Integer.MAX_VALUE, fileSystem); @@ -120,9 +122,6 @@ public final class CallTest { platform.assumeNotOpenJSSE(); logger.addHandler(logHandler); - client = clientTestRule.newClientBuilder() - .eventListener(listener) - .build(); } @After public void tearDown() throws Exception { diff --git a/okhttp/src/test/java/okhttp3/ConnectionReuseTest.java b/okhttp/src/test/java/okhttp3/ConnectionReuseTest.java index a3445d781..ea1a635f9 100644 --- a/okhttp/src/test/java/okhttp3/ConnectionReuseTest.java +++ b/okhttp/src/test/java/okhttp3/ConnectionReuseTest.java @@ -24,7 +24,6 @@ import okhttp3.mockwebserver.MockWebServer; import okhttp3.mockwebserver.SocketPolicy; import okhttp3.testing.PlatformRule; import okhttp3.tls.HandshakeCertificates; -import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.rules.TestRule; @@ -43,11 +42,7 @@ public final class ConnectionReuseTest { @Rule public final OkHttpClientTestRule clientTestRule = new OkHttpClientTestRule(); private HandshakeCertificates handshakeCertificates = localhost(); - private OkHttpClient client; - - @Before public void setUp() { - client = clientTestRule.newClient(); - } + private OkHttpClient client = clientTestRule.newClient(); @Test public void connectionsAreReused() throws Exception { server.enqueue(new MockResponse().setBody("a")); diff --git a/okhttp/src/test/java/okhttp3/ConscryptTest.kt b/okhttp/src/test/java/okhttp3/ConscryptTest.kt index 47381de1d..b2a53ef0b 100644 --- a/okhttp/src/test/java/okhttp3/ConscryptTest.kt +++ b/okhttp/src/test/java/okhttp3/ConscryptTest.kt @@ -35,11 +35,10 @@ class ConscryptTest { @JvmField @Rule val clientTestRule = OkHttpClientTestRule() - private lateinit var client: OkHttpClient + private val client = clientTestRule.newClient() @Before fun setUp() { platform.assumeConscrypt() - client = clientTestRule.newClient() } @Test diff --git a/okhttp/src/test/java/okhttp3/CookiesTest.java b/okhttp/src/test/java/okhttp3/CookiesTest.java index 758a0936c..44ac99fd1 100644 --- a/okhttp/src/test/java/okhttp3/CookiesTest.java +++ b/okhttp/src/test/java/okhttp3/CookiesTest.java @@ -30,7 +30,6 @@ import java.util.Map; import okhttp3.mockwebserver.MockResponse; import okhttp3.mockwebserver.MockWebServer; import okhttp3.mockwebserver.RecordedRequest; -import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -45,11 +44,7 @@ public class CookiesTest { @Rule public final MockWebServer server = new MockWebServer(); @Rule public final OkHttpClientTestRule clientTestRule = new OkHttpClientTestRule(); - private OkHttpClient client; - - @Before public void setUp() { - client = clientTestRule.newClient(); - } + private OkHttpClient client = clientTestRule.newClient(); @Test public void testNetscapeResponse() throws Exception { diff --git a/okhttp/src/test/java/okhttp3/DispatcherTest.java b/okhttp/src/test/java/okhttp3/DispatcherTest.java index 2a7c9a06b..edcd981c8 100644 --- a/okhttp/src/test/java/okhttp3/DispatcherTest.java +++ b/okhttp/src/test/java/okhttp3/DispatcherTest.java @@ -20,16 +20,15 @@ public final class DispatcherTest { RecordingWebSocketListener webSocketListener = new RecordingWebSocketListener(); Dispatcher dispatcher = new Dispatcher(executor); RecordingEventListener listener = new RecordingEventListener(); - OkHttpClient client; + OkHttpClient client = clientTestRule.newClientBuilder() + .dispatcher(dispatcher) + .eventListener(listener) + .build(); @Before public void setUp() throws Exception { dispatcher.setMaxRequests(20); dispatcher.setMaxRequestsPerHost(10); listener.forbidLock(dispatcher); - client = clientTestRule.newClientBuilder() - .dispatcher(dispatcher) - .eventListener(listener) - .build(); } @Test public void maxRequestsZero() throws Exception { diff --git a/okhttp/src/test/java/okhttp3/DuplexTest.java b/okhttp/src/test/java/okhttp3/DuplexTest.java index 7453035a9..d33b3e8b6 100644 --- a/okhttp/src/test/java/okhttp3/DuplexTest.java +++ b/okhttp/src/test/java/okhttp3/DuplexTest.java @@ -51,14 +51,13 @@ public final class DuplexTest { private RecordingEventListener listener = new RecordingEventListener(); private HandshakeCertificates handshakeCertificates = localhost(); - private OkHttpClient client; + private OkHttpClient client = clientTestRule.newClientBuilder() + .eventListener(listener) + .build(); @Before public void setUp() { platform.assumeNotOpenJSSE(); platform.assumeHttp2Support(); - client = clientTestRule.newClientBuilder() - .eventListener(listener) - .build(); } @Test public void http1DoesntSupportDuplex() throws IOException { diff --git a/okhttp/src/test/java/okhttp3/EventListenerTest.java b/okhttp/src/test/java/okhttp3/EventListenerTest.java index d5f23f603..f5a8fb3ae 100644 --- a/okhttp/src/test/java/okhttp3/EventListenerTest.java +++ b/okhttp/src/test/java/okhttp3/EventListenerTest.java @@ -82,16 +82,14 @@ public final class EventListenerTest { private final RecordingEventListener listener = new RecordingEventListener(); private final HandshakeCertificates handshakeCertificates = localhost(); - private OkHttpClient client; + private OkHttpClient client = clientTestRule.newClientBuilder() + .eventListener(listener) + .build(); private SocksProxy socksProxy; @Before public void setUp() { platform.assumeNotOpenJSSE(); - client = clientTestRule.newClientBuilder() - .eventListener(listener) - .build(); - listener.forbidLock(RealConnectionPool.Companion.get(client.connectionPool())); listener.forbidLock(client.dispatcher()); } diff --git a/okhttp/src/test/java/okhttp3/InterceptorTest.java b/okhttp/src/test/java/okhttp3/InterceptorTest.java index 3f0784d38..b85efb6bd 100644 --- a/okhttp/src/test/java/okhttp3/InterceptorTest.java +++ b/okhttp/src/test/java/okhttp3/InterceptorTest.java @@ -39,7 +39,6 @@ import okio.GzipSink; import okio.Okio; import okio.Sink; import okio.Source; -import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -50,13 +49,9 @@ public final class InterceptorTest { @Rule public MockWebServer server = new MockWebServer(); @Rule public final OkHttpClientTestRule clientTestRule = new OkHttpClientTestRule(); - private OkHttpClient client; + private OkHttpClient client = clientTestRule.newClient(); private RecordingCallback callback = new RecordingCallback(); - @Before public void setUp() { - client = clientTestRule.newClient(); - } - @Test public void applicationInterceptorsCanShortCircuitResponses() throws Exception { server.shutdown(); // Accept no connections. diff --git a/okhttp/src/test/java/okhttp3/OpenJSSETest.kt b/okhttp/src/test/java/okhttp3/OpenJSSETest.kt index a99243576..8b4ac7392 100644 --- a/okhttp/src/test/java/okhttp3/OpenJSSETest.kt +++ b/okhttp/src/test/java/okhttp3/OpenJSSETest.kt @@ -36,13 +36,11 @@ class OpenJSSETest { @JvmField @Rule var platform = PlatformRule() @JvmField @Rule val clientTestRule = OkHttpClientTestRule() @JvmField @Rule val server = MockWebServer() - lateinit var client: OkHttpClient + var client = clientTestRule.newClient() @Before fun setUp() { platform.assumeOpenJSSE() - - client = clientTestRule.newClient() } @Test @@ -109,4 +107,4 @@ class OpenJSSETest { .build() server.useHttps(handshakeCertificates.sslSocketFactory(), false) } -} \ No newline at end of file +} diff --git a/okhttp/src/test/java/okhttp3/URLConnectionTest.java b/okhttp/src/test/java/okhttp3/URLConnectionTest.java index 656122295..230bf4495 100644 --- a/okhttp/src/test/java/okhttp3/URLConnectionTest.java +++ b/okhttp/src/test/java/okhttp3/URLConnectionTest.java @@ -114,12 +114,11 @@ public final class URLConnectionTest { @Rule public final OkHttpClientTestRule clientTestRule = new OkHttpClientTestRule(); private HandshakeCertificates handshakeCertificates = localhost(); - private OkHttpClient client; + private OkHttpClient client = clientTestRule.newClient(); private @Nullable Cache cache; @Before public void setUp() { server.setProtocolNegotiationEnabled(false); - client = clientTestRule.newClient(); } @After public void tearDown() throws Exception { diff --git a/okhttp/src/test/java/okhttp3/WholeOperationTimeoutTest.java b/okhttp/src/test/java/okhttp3/WholeOperationTimeoutTest.java index bf8b54343..376642db1 100644 --- a/okhttp/src/test/java/okhttp3/WholeOperationTimeoutTest.java +++ b/okhttp/src/test/java/okhttp3/WholeOperationTimeoutTest.java @@ -25,7 +25,6 @@ import okhttp3.mockwebserver.MockResponse; import okhttp3.mockwebserver.MockWebServer; import okhttp3.testing.Flaky; import okio.BufferedSink; -import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -39,11 +38,7 @@ public final class WholeOperationTimeoutTest { @Rule public final MockWebServer server = new MockWebServer(); @Rule public final OkHttpClientTestRule clientTestRule = new OkHttpClientTestRule(); - private OkHttpClient client; - - @Before public void setUp() { - client = clientTestRule.newClient(); - } + private final OkHttpClient client = clientTestRule.newClient(); @Test public void defaultConfigIsNoTimeout() throws Exception { Request request = new Request.Builder() diff --git a/okhttp/src/test/java/okhttp3/internal/ws/WebSocketHttpTest.java b/okhttp/src/test/java/okhttp3/internal/ws/WebSocketHttpTest.java index 2006f235e..8d7d81634 100644 --- a/okhttp/src/test/java/okhttp3/internal/ws/WebSocketHttpTest.java +++ b/okhttp/src/test/java/okhttp3/internal/ws/WebSocketHttpTest.java @@ -72,28 +72,23 @@ public final class WebSocketHttpTest { private final WebSocketRecorder clientListener = new WebSocketRecorder("client"); private final WebSocketRecorder serverListener = new WebSocketRecorder("server"); private final Random random = new Random(0); - private OkHttpClient client; + private OkHttpClient client = clientTestRule.newClientBuilder() + .writeTimeout(500, TimeUnit.MILLISECONDS) + .readTimeout(500, TimeUnit.MILLISECONDS) + .addInterceptor(chain -> { + Response response = chain.proceed(chain.request()); + // Ensure application interceptors never see a null body. + assertThat(response.body()).isNotNull(); + return response; + }) + .build(); @Before public void setUp() { platform.assumeNotOpenJSSE(); - - client = clientTestRule.newClientBuilder() - .writeTimeout(500, TimeUnit.MILLISECONDS) - .readTimeout(500, TimeUnit.MILLISECONDS) - .addInterceptor(chain -> { - Response response = chain.proceed(chain.request()); - // Ensure application interceptors never see a null body. - assertThat(response.body()).isNotNull(); - return response; - }) - .build(); } @After public void tearDown() { clientListener.assertExhausted(); - - // TODO: assert all connections are released once leaks are fixed - clientTestRule.abandonClient(); } @Test public void textMessage() {