Address ECH PR review feedback#24
Merged
Merged
Conversation
- Restore -Xjvm-default=all (reverts out-of-scope TrailersSource API change) - Move robolectric --add-opens workaround into base-conventions, dropping the duplicates in testing-conventions and android-test - Move EchAware into the okhttp3.internal package - Remove leftover debug println from Android17SocketAdapter - Use kotlin.time.Duration for the DnsResolver timeout - Simplify FakeDnsLookup test helper - Replace the bespoke android37 emulator job with the standard android-emulator-runner using api-level 37.0
API 37 only ships 16 KB page-size (_ps16k) system images; the plain google_apis_playstore target does not exist for this level.
EchAware exposes the internal EchConfig type, so the interface must be internal too; it was unintentionally left public when moved to the okhttp3.internal package.
Reintroduces @ExperimentalOkHttpApi and an AsyncDns resolver that returns rich DnsResult values rather than bare addresses: - DnsResult.Address carries A/AAAA addresses; DnsResult.HttpsService carries an HTTPS/SVCB record (RFC 9460) with hints, ALPN, port and the ECHConfigList. Because results flow by value through DnsCallback, a resolver that wraps and forwards another preserves ECH/SVCB data automatically (it survives decoration, unlike the EchAware side-channel). - AsyncDns.asBlocking() bridges to the blocking Dns interface. - AndroidAsyncDns resolves both addresses and ECH from a single DnsResolver HTTPS query on API 36+. - OkHttpClient gains an experimental asyncDns, defaulting to the platform resolver (Platform.platformAsyncDns(); AndroidAsyncDns on API 37+). This is the API foundation. The connect path does not yet consume asyncDns; wiring route selection to read ECH off DnsResult.HttpsService (and removing the EchAware cache) is the next step. Builds: compileKotlinJvm, compileAndroidMain, compileAndroidHostTest, apiDump.
Revert the android37 job to the inline sdkmanager/avdmanager/emulator script (the android-emulator-runner action can't select the only available API 37 image, google_apis_playstore_ps16k, the 16 KB page-size variant).
Add api-level 37.0 (target playstore_ps16k -> google_apis_playstore_ps16k, the 16 KB page-size image) to the android matrix and drop the standalone android37 job.
Lint flags the DnsResolver(Context, Looper) constructor as requiring an SDK extension; suppress at the class level like AndroidDnsResolver, since the resolver is gated at runtime by @RequiresApi(36).
The single HttpsEndpoint query relied on the HTTPS answer for addresses, which is unreliable for hosts without an HTTPS record. Issue independent A, AAAA and HTTPS queries instead, delivering each as its own DnsResult batch; the last to complete reports hasMore = false (atomic counter). Add AsyncDns.newCall(hostname, addressesOnly): when true the resolver may skip the HTTPS/SVCB query. asBlocking() (which feeds Dns.lookup) passes addressesOnly = true since Dns cannot carry ECH.
Promote okhttp3.ech.EchConfig, EchMode and EchModeConfiguration from internal to public @ExperimentalOkHttpApi, consistent with AsyncDns/ DnsResult. Opt the internal consumers and tests in via file-level @OptIn, and regenerate the API dumps.
Revert the @ExperimentalOkHttpApi approach: AsyncDns, DnsResult and the okhttp3.ech types (EchConfig, EchMode, EchModeConfiguration) go back to internal, OkHttpClient.asyncDns becomes an internal field (no public builder), and the reintroduced ExperimentalOkHttpApi marker is removed. This matches the reviewer's guidance to keep unstable ECH/DNS APIs out of the public surface (cf. the EchAware "internal package" comment, and the team having removed @ExperimentalOkHttpApi / stashed AsyncDns rather than ship incomplete public API). API dumps return to baseline.
Replace call.tag(EchMode::class) / call.tag(EchConfig::class) with internal echMode / echConfig fields on RealCall. Tags are an end-user extension point; OkHttp's own per-call state belongs in fields (and setting echMode = Fallback on retry is now an unambiguous overwrite rather than tag compute-if-absent). EchAware is unchanged: it still bridges Dns.lookup (which has no Call) to applyEch; only the tag-based carrying is removed.
Introduce okhttp3.internal.RealCall.resolveAddresses(dns, hostname): the internal, call-aware DNS entry point. When the platform has an AsyncDns it resolves with it and captures the HTTPS/SVCB ECH config onto the call's echConfig field; otherwise it drops the call and uses the public, call-less Dns. RouteSelector now goes through it. With ECH carried on the call field, applyEch becomes apply-only (reconstructing EchConfigList via EchConfigList.fromBytes), and EchAware, its per-host cache, AndroidDnsResolverDns and AndroidEchConfig are removed. Android17Platform shares one AndroidAsyncDns for both platformDns (asBlocking) and platformAsyncDns. Follows the reviewer's guidance: state on a field (not a tag), and the new DNS API lives in the okhttp3.internal package.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to the review on lysine-dev#9383, stacked on
testing_ech.Changes
-Xjvm-default=allin jvm-conventions, reverting the out-of-scopeTrailersSource.peekAPI change (and the.apidumps).--add-opensworkaround intobase-conventions, removing the duplicates intesting-conventionsandandroid-test.EchAwareinterface into theokhttp3.internalpackage.printlnfromAndroid17SocketAdapter.kotlin.time.Durationfor theDnsResolvertimeout instead of a bare secondsLong.FakeDnsLookuptest helper (publicresponses, nosetoperator).android37emulator job with the standardandroid-emulator-runner(api-level: 37.0), matching the existingandroidjob.Test plan
act -lparses the workflow; YAML validates.apiCheckand the AndroidconnectedCheck/ host tests on CI.target: google_apis_playstoreresolves the intended API 37 image (the old manual job used thegoogle_apis_playstore_ps16k16 KB-page variant, which the action'stargetinput can't select).