Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lets changes our OkHttp 5 async DNS APIs #8318

Open
swankjesse opened this issue Mar 31, 2024 · 3 comments
Open

Lets changes our OkHttp 5 async DNS APIs #8318

swankjesse opened this issue Mar 31, 2024 · 3 comments
Labels
bug Bug in existing code
Milestone

Comments

@swankjesse
Copy link
Member

Our implementation is great but I’ve got some ideas for changes to the APIs.

We should support multiple rounds of responses from a single AsyncDns instance. This should make it easier to make a compound IPv4 + IPv6 resolver that doesn’t make the caller wait until both are done.

interface AsyncDns {
    /**
     * Invoked on a successful result from a single lookup step.
     * 
     * @param addresses a non-empty list of addresses
     * @param hasMore true if another call to onAddresses or onFailure will be made
     */
    fun onAddresses(
      hasMore: Boolean,
      hostname: String,
      addresses: List<InetAddress>,
    )

    /**
     * Invoked on a failed result from a single lookup step.
     * 
     * @param hasMore true if another call to onAddresses or onFailure will be made
     */
    fun onFailure(
      hasMore: Boolean,
      hostname: String,
      e: IOException,
    )
}

I’m anticipating FastFallbackExchangeFinder as the user of this API. It could act as soon as either address class is returned.

Callback to Blocking

The toDns() API does both callback-to-blocking and also unioning. Let’s split those responsibilities!

/** Returns a [Dns] that blocks until all async results are available. */
fun AsyncDns.asBlocking(): Dns

Union Callbacks

/** 
 * Returns an [AsyncDns] that queries all [sources] in parallel, and calls
 * the callback for each partial result.
 * 
 * The callback will be passed `hasMore = false` only when all sources
 * have no more results.
 * 
 * @param sources one or more AsyncDns sources to query.
 */
fun union(vararg sources: AsyncDns): AsyncDns

Android API v1

By making that complex API above we can union IPv4 + IPv6 for Android users.

class AndroidAsyncDns {
  ...
  companion object {
    ...
    val SYSTEM: AsyncDns
  }
}

Android API v2

Even better than an easier-to-use API is no API at all!

Let’s change OkHttpClient to use Android’s DnsResolver when that’s available? We could probably make it so our AndroidAsyncDns is an implementation detail that nobody needs to see.

@swankjesse swankjesse added the bug Bug in existing code label Mar 31, 2024
@swankjesse swankjesse added this to the 5.0 milestone Mar 31, 2024
@swankjesse
Copy link
Member Author

I’d also really like for OkHttp to do async internally when that’s possible. I expect that to be a difficult refactor.

@yschimke
Copy link
Collaborator

Maybe we should list and prioritise those and work towards them being pragmatically async when it is obviously an improvement.

Some are hard like interceptors. Some might be easier like cache.

@yschimke
Copy link
Collaborator

I'm taking a look at this as part of #8286 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug in existing code
Projects
None yet
Development

No branches or pull requests

2 participants