From 3e560bc76bded39f701caf2eee8610b0e736efc7 Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Mon, 22 Apr 2024 21:01:28 +0100 Subject: [PATCH 1/2] Simplify the logic. Rely on OkHttpClient caching, instead of a cache only request. --- .../okhttp3/dnsoverhttps/DnsOverHttps.kt | 66 +++---------------- .../okhttp3/dnsoverhttps/DnsOverHttpsTest.kt | 26 ++++++++ 2 files changed, 35 insertions(+), 57 deletions(-) diff --git a/okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/DnsOverHttps.kt b/okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/DnsOverHttps.kt index cc3872e5e194..d6cd09d0b420 100644 --- a/okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/DnsOverHttps.kt +++ b/okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/DnsOverHttps.kt @@ -16,11 +16,9 @@ package okhttp3.dnsoverhttps import java.io.IOException -import java.net.HttpURLConnection import java.net.InetAddress import java.net.UnknownHostException import java.util.concurrent.CountDownLatch -import okhttp3.CacheControl import okhttp3.Call import okhttp3.Callback import okhttp3.Dns @@ -72,16 +70,16 @@ class DnsOverHttps internal constructor( @Throws(UnknownHostException::class) private fun lookupHttps(hostname: String): List { - val networkRequests = ArrayList(2) - val failures = ArrayList(2) - val results = ArrayList(5) - - buildRequest(hostname, networkRequests, results, failures, DnsRecordCodec.TYPE_A) + val networkRequests = buildList { + add(client.newCall(buildRequest(hostname, DnsRecordCodec.TYPE_A))) - if (includeIPv6) { - buildRequest(hostname, networkRequests, results, failures, DnsRecordCodec.TYPE_AAAA) + if (includeIPv6) { + add(client.newCall(buildRequest(hostname, DnsRecordCodec.TYPE_AAAA))) + } } + val failures = ArrayList(2) + val results = ArrayList(5) executeRequests(hostname, networkRequests, results, failures) return results.ifEmpty { @@ -89,21 +87,6 @@ class DnsOverHttps internal constructor( } } - private fun buildRequest( - hostname: String, - networkRequests: MutableList, - results: MutableList, - failures: MutableList, - type: Int, - ) { - val request = buildRequest(hostname, type) - val response = getCacheOnlyResponse(request) - - response?.let { processResponse(it, hostname, results, failures) } ?: networkRequests.add( - client.newCall(request), - ) - } - private fun executeRequests( hostname: String, networkRequests: List, @@ -186,38 +169,6 @@ class DnsOverHttps internal constructor( throw unknownHostException } - private fun getCacheOnlyResponse(request: Request): Response? { - if (client.cache != null) { - try { - // Use the cache without hitting the network first - // 504 code indicates that the Cache is stale - val onlyIfCached = - CacheControl.Builder() - .onlyIfCached() - .build() - - var cacheUrl = request.url - - val cacheRequest = - request.newBuilder() - .cacheControl(onlyIfCached) - .cacheUrlOverride(cacheUrl) - .build() - - val cacheResponse = client.newCall(cacheRequest).execute() - - if (cacheResponse.code != HttpURLConnection.HTTP_GATEWAY_TIMEOUT) { - return cacheResponse - } - } catch (ioe: IOException) { - // Failures are ignored as we can fallback to the network - // and hopefully repopulate the cache. - } - } - - return null - } - @Throws(Exception::class) private fun readResponse( hostname: String, @@ -325,7 +276,8 @@ class DnsOverHttps internal constructor( this.bootstrapDnsHosts = bootstrapDnsHosts } - fun bootstrapDnsHosts(vararg bootstrapDnsHosts: InetAddress): Builder = bootstrapDnsHosts(bootstrapDnsHosts.toList()) + fun bootstrapDnsHosts(vararg bootstrapDnsHosts: InetAddress): Builder = + bootstrapDnsHosts(bootstrapDnsHosts.toList()) fun systemDns(systemDns: Dns) = apply { diff --git a/okhttp-dnsoverhttps/src/test/java/okhttp3/dnsoverhttps/DnsOverHttpsTest.kt b/okhttp-dnsoverhttps/src/test/java/okhttp3/dnsoverhttps/DnsOverHttpsTest.kt index bfe84cb34800..34d602b36fe9 100644 --- a/okhttp-dnsoverhttps/src/test/java/okhttp3/dnsoverhttps/DnsOverHttpsTest.kt +++ b/okhttp-dnsoverhttps/src/test/java/okhttp3/dnsoverhttps/DnsOverHttpsTest.kt @@ -35,6 +35,7 @@ import okhttp3.Cache import okhttp3.Dns import okhttp3.OkHttpClient import okhttp3.Protocol +import okhttp3.RecordingEventListener import okhttp3.testing.PlatformRule import okio.Buffer import okio.ByteString.Companion.decodeHex @@ -53,9 +54,11 @@ class DnsOverHttpsTest { private lateinit var server: MockWebServer private lateinit var dns: Dns private val cacheFs = FakeFileSystem() + private val eventListener = RecordingEventListener() private val bootstrapClient = OkHttpClient.Builder() .protocols(listOf(Protocol.HTTP_2, Protocol.HTTP_1_1)) + .eventListener(eventListener) .build() @BeforeEach @@ -194,16 +197,22 @@ class DnsOverHttpsTest { assertThat(recordedRequest.path) .isEqualTo("/lookup?ct&dns=AAABAAABAAAAAAAABmdvb2dsZQNjb20AAAEAAQ") + assertThat(cacheEvents()).containsExactly("CacheMiss") + result = cachedDns.lookup("google.com") assertThat(server.takeRequest(1, TimeUnit.MILLISECONDS)).isNull() assertThat(result).isEqualTo(listOf(address("157.240.1.18"))) + assertThat(cacheEvents()).containsExactly("CacheHit") + result = cachedDns.lookup("www.google.com") assertThat(result).containsExactly(address("157.240.1.18")) recordedRequest = server.takeRequest() assertThat(recordedRequest.method).isEqualTo("GET") assertThat(recordedRequest.path) .isEqualTo("/lookup?ct&dns=AAABAAABAAAAAAAAA3d3dwZnb29nbGUDY29tAAABAAE") + + assertThat(cacheEvents()).containsExactly("CacheMiss") } @Test @@ -231,16 +240,22 @@ class DnsOverHttpsTest { assertThat(recordedRequest.path) .isEqualTo("/lookup?ct") + assertThat(cacheEvents()).containsExactly("CacheMiss") + result = cachedDns.lookup("google.com") assertThat(server.takeRequest(0, TimeUnit.MILLISECONDS)).isNull() assertThat(result).isEqualTo(listOf(address("157.240.1.18"))) + assertThat(cacheEvents()).containsExactly("CacheHit") + result = cachedDns.lookup("www.google.com") assertThat(result).containsExactly(address("157.240.1.18")) recordedRequest = server.takeRequest(0, TimeUnit.MILLISECONDS)!! assertThat(recordedRequest.method).isEqualTo("POST") assertThat(recordedRequest.path) .isEqualTo("/lookup?ct") + + assertThat(cacheEvents()).containsExactly("CacheMiss") } @Test @@ -265,6 +280,9 @@ class DnsOverHttpsTest { assertThat(recordedRequest.path).isEqualTo( "/lookup?ct&dns=AAABAAABAAAAAAAABmdvb2dsZQNjb20AAAEAAQ", ) + + assertThat(cacheEvents()).containsExactly("CacheMiss") + Thread.sleep(2000) server.enqueue( dnsResponse( @@ -282,6 +300,14 @@ class DnsOverHttpsTest { assertThat(recordedRequest!!.method).isEqualTo("GET") assertThat(recordedRequest.path) .isEqualTo("/lookup?ct&dns=AAABAAABAAAAAAAABmdvb2dsZQNjb20AAAEAAQ") + + assertThat(cacheEvents()).containsExactly("CacheMiss") + } + + private fun cacheEvents(): List { + return eventListener.recordedEventTypes().filter { it.contains("Cache") }.also { + eventListener.clearAllEvents() + } } private fun dnsResponse(s: String): MockResponse { From 5b3e747a5cefb83fb2ac62f56f43f24c5a348517 Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Mon, 22 Apr 2024 21:07:44 +0100 Subject: [PATCH 2/2] Simplify the logic. Rely on OkHttpClient caching, instead of a cache only request. --- .../kotlin/okhttp3/dnsoverhttps/DnsOverHttps.kt | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/DnsOverHttps.kt b/okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/DnsOverHttps.kt index d6cd09d0b420..726643b872be 100644 --- a/okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/DnsOverHttps.kt +++ b/okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/DnsOverHttps.kt @@ -70,13 +70,14 @@ class DnsOverHttps internal constructor( @Throws(UnknownHostException::class) private fun lookupHttps(hostname: String): List { - val networkRequests = buildList { - add(client.newCall(buildRequest(hostname, DnsRecordCodec.TYPE_A))) + val networkRequests = + buildList { + add(client.newCall(buildRequest(hostname, DnsRecordCodec.TYPE_A))) - if (includeIPv6) { - add(client.newCall(buildRequest(hostname, DnsRecordCodec.TYPE_AAAA))) + if (includeIPv6) { + add(client.newCall(buildRequest(hostname, DnsRecordCodec.TYPE_AAAA))) + } } - } val failures = ArrayList(2) val results = ArrayList(5) @@ -276,8 +277,7 @@ class DnsOverHttps internal constructor( this.bootstrapDnsHosts = bootstrapDnsHosts } - fun bootstrapDnsHosts(vararg bootstrapDnsHosts: InetAddress): Builder = - bootstrapDnsHosts(bootstrapDnsHosts.toList()) + fun bootstrapDnsHosts(vararg bootstrapDnsHosts: InetAddress): Builder = bootstrapDnsHosts(bootstrapDnsHosts.toList()) fun systemDns(systemDns: Dns) = apply {