From 678ccff98a703bc95aa5df2bcb72bf38f27d9234 Mon Sep 17 00:00:00 2001 From: Jesse Wilson Date: Sat, 25 Jul 2026 10:47:10 -0400 Subject: [PATCH] Move DoH public/private host rules into the DnsQuery I had attempted to avoid making StateMachineDnsCall have its own TaskRunner, instead relying exclusively on the DnsQuery callback for concurrency. This was more complexity than it was worth, and it interacted awkwardly with the cache. Now the DnsOverHttps DnsQuery enforces this invariant, and the adjacent code is simpler because of it. --- .../okhttp3/dnsoverhttps/DnsOverHttps.kt | 43 +++++++---------- .../internal/-DnsOverHttpsQuery.kt | 47 ++++++++++++++++++- okhttp/api/android/okhttp.api | 1 - okhttp/api/jvm/okhttp.api | 1 - .../kotlin/okhttp3/android/AndroidDns.kt | 2 +- .../kotlin/okhttp3/DnsCache.kt | 2 +- .../internal/dns/-StateMachineDnsCall.kt | 33 ++++++++----- .../internal/dns/StateMachineDnsCallTest.kt | 47 ++----------------- .../internal/dns/StateMachineDnsCallTester.kt | 2 +- okhttp/src/jvmMain/java9/module-info.java | 2 +- 10 files changed, 92 insertions(+), 88 deletions(-) diff --git a/okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/DnsOverHttps.kt b/okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/DnsOverHttps.kt index 6fe03cefcbd5..364b2f31b9e8 100644 --- a/okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/DnsOverHttps.kt +++ b/okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/DnsOverHttps.kt @@ -23,6 +23,7 @@ import okhttp3.MediaType import okhttp3.MediaType.Companion.toMediaType import okhttp3.OkHttpClient import okhttp3.dnsoverhttps.internal.DnsOverHttpsQuery +import okhttp3.internal.concurrent.TaskRunner import okhttp3.internal.dns.StateMachineDnsCall import okhttp3.internal.dns.execute import okhttp3.internal.publicsuffix.PublicSuffixDatabase @@ -38,6 +39,7 @@ import okhttp3.internal.publicsuffix.PublicSuffixDatabase * [doh_spec]: https://tools.ietf.org/html/draft-ietf-doh-dns-over-https-13 */ class DnsOverHttps internal constructor( + private val taskRunner: TaskRunner, @get:JvmName("client") val client: OkHttpClient, @get:JvmName("url") val url: HttpUrl, @get:JvmName("includeIPv6") val includeIPv6: Boolean, @@ -48,6 +50,9 @@ class DnsOverHttps internal constructor( ) : Dns { private val queryFactory = DnsOverHttpsQuery.Factory( + taskRunner = taskRunner, + resolvePrivateAddresses = resolvePrivateAddresses, + resolvePublicAddresses = resolvePublicAddresses, client = client, dnsUrl = url, post = post, @@ -55,36 +60,18 @@ class DnsOverHttps internal constructor( override fun newCall(request: Dns.Request): Dns.Call = StateMachineDnsCall( + taskRunner = taskRunner, request = request, queryFactory = queryFactory, - canceledException = validate(request.hostname), includeIPv6 = includeIPv6, includeServiceMetadata = includeServiceMetadata, ) - /** - * Returns an exception if [hostname] should not be resolved. - * - * We **return** this exception rather than throwing it because in the [Dns.Callback] case we want - * `onFailure()` to be called on a dispatcher thread and not synchronously. - */ - private fun validate(hostname: String): UnknownHostException? { - // Don't load the public suffix list unless necessary. - if (resolvePrivateAddresses && resolvePublicAddresses) return null - - val privateHost = isPrivateHost(hostname) - - return when { - privateHost && !resolvePrivateAddresses -> UnknownHostException("private hosts not resolved") - !privateHost && !resolvePublicAddresses -> UnknownHostException("public hosts not resolved") - else -> null - } - } - @Throws(UnknownHostException::class) override fun lookup(hostname: String): List { val withoutServiceMetadata = DnsOverHttps( + taskRunner = taskRunner, client = client, url = url, includeIPv6 = includeIPv6, @@ -101,6 +88,7 @@ class DnsOverHttps internal constructor( } class Builder { + internal val taskRunner = TaskRunner.INSTANCE internal var client: OkHttpClient? = null internal var url: HttpUrl? = null internal var includeIPv6 = true @@ -114,13 +102,14 @@ class DnsOverHttps internal constructor( fun build(): DnsOverHttps { val client = this.client ?: throw NullPointerException("client not set") return DnsOverHttps( - client.newBuilder().dns(buildBootstrapClient(this)).build(), - checkNotNull(url) { "url not set" }, - includeIPv6, - includeServiceMetadata, - post, - resolvePrivateAddresses, - resolvePublicAddresses, + taskRunner = taskRunner, + client = client.newBuilder().dns(buildBootstrapClient(this)).build(), + url = checkNotNull(url) { "url not set" }, + includeIPv6 = includeIPv6, + includeServiceMetadata = includeServiceMetadata, + post = post, + resolvePrivateAddresses = resolvePrivateAddresses, + resolvePublicAddresses = resolvePublicAddresses, ) } diff --git a/okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/internal/-DnsOverHttpsQuery.kt b/okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/internal/-DnsOverHttpsQuery.kt index 4e721eecef25..7417edd945e2 100644 --- a/okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/internal/-DnsOverHttpsQuery.kt +++ b/okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/internal/-DnsOverHttpsQuery.kt @@ -19,6 +19,7 @@ package okhttp3.dnsoverhttps.internal import java.io.IOException import java.net.ProtocolException +import java.net.UnknownHostException import okhttp3.Call import okhttp3.Callback import okhttp3.HttpUrl @@ -29,7 +30,9 @@ import okhttp3.RequestBody import okhttp3.Response import okhttp3.dnsoverhttps.DnsOverHttps.Companion.DNS_MESSAGE import okhttp3.dnsoverhttps.DnsOverHttps.Companion.MAX_RESPONSE_SIZE +import okhttp3.dnsoverhttps.DnsOverHttps.Companion.isPrivateHost import okhttp3.internal.OkHttpInternalApi +import okhttp3.internal.concurrent.TaskRunner import okhttp3.internal.dns.DnsMessage import okhttp3.internal.dns.DnsMessageReader import okhttp3.internal.dns.DnsMessageWriter @@ -41,7 +44,7 @@ import okio.BufferedSink @OkHttpInternalApi internal class DnsOverHttpsQuery( - val call: Call, + private val call: Call, ) : DnsQuery { override fun enqueue(callback: DnsQuery.Callback) { call.enqueue( @@ -75,11 +78,19 @@ internal class DnsOverHttpsQuery( } class Factory( + private val taskRunner: TaskRunner, + private val resolvePrivateAddresses: Boolean, + private val resolvePublicAddresses: Boolean, private val client: OkHttpClient, private val dnsUrl: HttpUrl, private val post: Boolean, ) : DnsQuery.Factory { override fun newQuery(question: Question): DnsQuery { + val overrideQuery = overrideQuery(question) + if (overrideQuery != null) { + return overrideQuery + } + val dnsMessage = DnsMessage.query(question) return DnsOverHttpsQuery( call = @@ -111,6 +122,40 @@ internal class DnsOverHttpsQuery( ), ) } + + /** Returns an alternate query for [question] if the [DnsOverHttpsQuery] should not be used. */ + private fun overrideQuery(question: Question): DnsQuery? { + // Don't load the public suffix list unless necessary. + if (resolvePrivateAddresses && resolvePublicAddresses) return null + + val privateHost = isPrivateHost(question.name) + + val resolveUnsupportedMessage = + when { + privateHost && !resolvePrivateAddresses -> "private hosts not resolved" + !privateHost && !resolvePublicAddresses -> "public hosts not resolved" + else -> return null + } + + return ResolveUnsupportedQuery( + hostname = question.name, + message = resolveUnsupportedMessage, + ) + } + + private inner class ResolveUnsupportedQuery( + private val hostname: String, + private val message: String, + ) : DnsQuery { + override fun enqueue(callback: DnsQuery.Callback) { + taskRunner.newQueue().execute("$hostname dns") { + callback.onFailure(UnknownHostException(message)) + } + } + + override fun cancel() { + } + } } } diff --git a/okhttp/api/android/okhttp.api b/okhttp/api/android/okhttp.api index aa380ae04447..a30c998b415b 100644 --- a/okhttp/api/android/okhttp.api +++ b/okhttp/api/android/okhttp.api @@ -538,7 +538,6 @@ public final class okhttp3/Dns$Request { } public final class okhttp3/DnsCache { - public final field -delegate Lokhttp3/internal/dns/RealDnsCache; public synthetic fun (JJJJIILkotlin/jvm/internal/DefaultConstructorMarker;)V public synthetic fun (JJJJILkotlin/jvm/internal/DefaultConstructorMarker;)V public final fun evictAll ()V diff --git a/okhttp/api/jvm/okhttp.api b/okhttp/api/jvm/okhttp.api index e01fecde1972..aae4cbd894b5 100644 --- a/okhttp/api/jvm/okhttp.api +++ b/okhttp/api/jvm/okhttp.api @@ -538,7 +538,6 @@ public final class okhttp3/Dns$Request { } public final class okhttp3/DnsCache { - public final field -delegate Lokhttp3/internal/dns/RealDnsCache; public synthetic fun (JJJJIILkotlin/jvm/internal/DefaultConstructorMarker;)V public synthetic fun (JJJJILkotlin/jvm/internal/DefaultConstructorMarker;)V public final fun evictAll ()V diff --git a/okhttp/src/androidMain/kotlin/okhttp3/android/AndroidDns.kt b/okhttp/src/androidMain/kotlin/okhttp3/android/AndroidDns.kt index 65ac4b304831..ab7cf89e0a03 100644 --- a/okhttp/src/androidMain/kotlin/okhttp3/android/AndroidDns.kt +++ b/okhttp/src/androidMain/kotlin/okhttp3/android/AndroidDns.kt @@ -87,9 +87,9 @@ class AndroidDns( includeServiceMetadata: Boolean, ): Dns.Call = StateMachineDnsCall( + taskRunner = taskRunner, request = request, queryFactory = queryFactory, - canceledException = null, // A single `A` query stands in for both families: the system resolver returns IPv4 and // IPv6 addresses together, so there's no separate `AAAA` query. includeIPv6 = false, diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/DnsCache.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/DnsCache.kt index 2b09f94f2c87..abe492698854 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/DnsCache.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/DnsCache.kt @@ -76,7 +76,7 @@ import okhttp3.internal.dns.RealDnsCache class DnsCache internal constructor( @OkHttpInternalApi @JvmField - val `-delegate`: RealDnsCache, + internal val `-delegate`: RealDnsCache, ) { @get:JvmName("minimumTimeToLive") val minimumTimeToLive: Duration diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/dns/-StateMachineDnsCall.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/dns/-StateMachineDnsCall.kt index f068f7fa5348..496a1e5b22a5 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/dns/-StateMachineDnsCall.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/dns/-StateMachineDnsCall.kt @@ -24,6 +24,7 @@ import java.util.concurrent.atomic.AtomicReference import okhttp3.Dns import okhttp3.Protocol import okhttp3.internal.OkHttpInternalApi +import okhttp3.internal.concurrent.TaskRunner /** * An application-layer [Dns.Call] that performs multiple transport-layer [DnsQuery]s in parallel. @@ -53,9 +54,9 @@ import okhttp3.internal.OkHttpInternalApi */ @OkHttpInternalApi class StateMachineDnsCall( + private val taskRunner: TaskRunner, override val request: Dns.Request, private val queryFactory: DnsQuery.Factory, - private val canceledException: IOException?, private val includeIPv6: Boolean, private val includeServiceMetadata: Boolean, ) : Dns.Call { @@ -75,19 +76,32 @@ class StateMachineDnsCall( add(Question(request.hostname, TYPE_A)) } - val queries = - questions.map { question -> - queryFactory.newQuery(question) - } - while (true) { val previous = state.get() as? State.Idle ?: error("already enqueued") + // If it's canceled before it is enqueued, jump straight to Complete. + if (previous.canceled) { + val next = State.Complete(canceled = true) + + if (!state.compareAndSet(previous, next)) continue // Lost a race, retry. + + taskRunner.newQueue().execute("${request.hostname} dns") { + callback.onFailure(this, IOException("canceled")) + } + + return + } + + val queries = + questions.map { question -> + queryFactory.newQuery(question) + } + val next = State.Running( - canceled = previous.canceled, + canceled = false, callback = callback, runningQueries = queries, ) @@ -95,10 +109,6 @@ class StateMachineDnsCall( if (!state.compareAndSet(previous, next)) continue // Lost a race, retry. for (query in queries) { - if (previous.canceled || canceledException != null) { - query.cancel() - } - query.enqueue( callback = object : DnsQuery.Callback { @@ -210,7 +220,6 @@ class StateMachineDnsCall( val allExceptions = when { - canceledException != null -> listOf(canceledException) newException != null -> previous.pendingExceptions + newException else -> previous.pendingExceptions } diff --git a/okhttp/src/commonTest/kotlin/okhttp3/internal/dns/StateMachineDnsCallTest.kt b/okhttp/src/commonTest/kotlin/okhttp3/internal/dns/StateMachineDnsCallTest.kt index 6cbcd6a67f46..13d2f94bd31c 100644 --- a/okhttp/src/commonTest/kotlin/okhttp3/internal/dns/StateMachineDnsCallTest.kt +++ b/okhttp/src/commonTest/kotlin/okhttp3/internal/dns/StateMachineDnsCallTest.kt @@ -841,59 +841,22 @@ class StateMachineDnsCallTest { } } - /** - * The implementation still enqueues canceled queries, because that's an easy way to jump to a - * dispatcher thread to post the failures back to the callback. - */ - @Test - fun `cancel before enqueue`() = - testStateMachineDnsCall { - val call = - newCall( - request = Dns.Request(hostname = "lysine.dev"), - caching = false, - ) - call.cancel() - call.enqueue() - - queryFactory.takeCancel("lysine.dev", TYPE_HTTPS) - val query0 = queryFactory.takeQuery("lysine.dev", TYPE_HTTPS) - queryFactory.takeCancel("lysine.dev", TYPE_AAAA) - val query1 = queryFactory.takeQuery("lysine.dev", TYPE_AAAA) - queryFactory.takeCancel("lysine.dev", TYPE_A) - val query2 = queryFactory.takeQuery("lysine.dev", TYPE_A) - - query0.respondFailure("canceled") - query1.respondFailure("canceled") - query2.respondFailure("canceled") - - call.takeOnFailure("canceled") - } - @Test - fun `cancel before enqueue with caching`() = + fun `cancel before enqueue`(caching: Boolean) = testStateMachineDnsCall { val call = newCall( request = Dns.Request(hostname = "lysine.dev"), - caching = true, + caching = caching, ) call.cancel() call.enqueue() - val query0 = queryFactory.takeQuery("lysine.dev", TYPE_HTTPS) - val query1 = queryFactory.takeQuery("lysine.dev", TYPE_AAAA) - val query2 = queryFactory.takeQuery("lysine.dev", TYPE_A) - - query0.respondFailure("canceled") - query1.respondFailure("canceled") - query2.respondFailure("canceled") - call.takeOnFailure("canceled") - assertThat(cache.size).isEqualTo(3) - assertThat(cache.requestCount).isEqualTo(3) - assertThat(cache.networkCount).isEqualTo(3) + assertThat(cache.size).isEqualTo(0) + assertThat(cache.requestCount).isEqualTo(0) + assertThat(cache.networkCount).isEqualTo(0) assertThat(cache.hitCount).isEqualTo(0) } diff --git a/okhttp/src/commonTest/kotlin/okhttp3/internal/dns/StateMachineDnsCallTester.kt b/okhttp/src/commonTest/kotlin/okhttp3/internal/dns/StateMachineDnsCallTester.kt index deee6de212f2..87770346fb41 100644 --- a/okhttp/src/commonTest/kotlin/okhttp3/internal/dns/StateMachineDnsCallTester.kt +++ b/okhttp/src/commonTest/kotlin/okhttp3/internal/dns/StateMachineDnsCallTester.kt @@ -146,13 +146,13 @@ class StateMachineDnsCallTester internal constructor() { val call = StateMachineDnsCall( + taskRunner = taskFaker.taskRunner, request = request, queryFactory = when { caching -> cache.wrap(queryFactory) else -> queryFactory }, - canceledException = null, includeIPv6 = includeIPv6, includeServiceMetadata = includeServiceMetadata, ) diff --git a/okhttp/src/jvmMain/java9/module-info.java b/okhttp/src/jvmMain/java9/module-info.java index bb08c78336ee..cecc47f37722 100644 --- a/okhttp/src/jvmMain/java9/module-info.java +++ b/okhttp/src/jvmMain/java9/module-info.java @@ -5,7 +5,7 @@ requires java.logging; exports okhttp3; exports okhttp3.internal to okhttp3.logging, okhttp3.sse, okhttp3.java.net.cookiejar, okhttp3.dnsoverhttps, mockwebserver3, okhttp3.mockwebserver, mockwebserver3.junit5, okhttp3.coroutines, okhttp3.tls; - exports okhttp3.internal.concurrent to mockwebserver3, okhttp3.mockwebserver; + exports okhttp3.internal.concurrent to mockwebserver3, okhttp3.dnsoverhttps, okhttp3.mockwebserver; exports okhttp3.internal.connection to mockwebserver3, okhttp3.mockwebserver, okhttp3.logging; exports okhttp3.internal.dns to okhttp3.dnsoverhttps; exports okhttp3.internal.http to okhttp3.logging, okhttp3.brotli, mockwebserver3;