Skip to content

Conversation

@yschimke
Copy link
Collaborator

Primarily an Android specific implementation of AsyncDns.
This is intended to unblock the Happy Eyeballs, fast follows implementation to handle IPv4 and IPv6 separately.

AsyncDns should be considered an experimental public API.

@swankjesse I don't know how to do this in okhttp core using Flow. Bringing in kotlinx.coroutines is probably awkward for JVM clients already frustrated at getting kotlin, and more likely to introduce versioning issues that stdlib (which has been perfect).

Also an optional okhttp-android module, to allow specific implementations that are network aware*. Published as an AAR file which might help with android specific optimisations like baseline profiles.

Android apps may often need to understand network conditions, choose the appropriate network, and when doing so
should use the appropriate socket factory, dns, and even proxy.

@yschimke yschimke requested a review from swankjesse March 12, 2022 11:57
@yschimke yschimke added the android Relates to usage specifically on Android label Mar 12, 2022
@yschimke
Copy link
Collaborator Author

Two remaining topics

  1. When to pull over the changes for DnsOverHttps
  2. When to consider the design for other Dns records like Svcb, Https (draft RFC https://datatracker.ietf.org/doc/draft-ietf-dnsop-svcb-https/)

@yschimke yschimke marked this pull request as ready for review March 15, 2022 09:08
Copy link
Collaborator

@swankjesse swankjesse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It gets complicated using one instance to deal with either 1 or 2 responses.

What if we make it so each request gets a single response, and let the caller pick what’s in that response? If the caller wants one callback we can give them that, and if they want two callbacks we can give them that too?

interface AsyncDns {
  fun lookup(hostname: String, callback: Callback)

  interface Callback {
    fun onFailure(hostname: String, e: IOException)
    fun onResponse(hostname: String, response: List<InetAddress>)
  }

  companion object {
    /** Unions the results from [SYSTEM_IPV4_ONLY] and [SYSTEM_IPV6_ONLY]. */
    val SYSTEM: AsyncDns
    val SYSTEM_IPV4_ONLY: AsyncDns
    val SYSTEM_IPV6_ONLY: AsyncDns
  }
}

}
}

private fun assumeNetwork() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

@swankjesse
Copy link
Collaborator

I’d like to run with this on OkHttp 5, and make it so our default Dns internally is async.

@yschimke
Copy link
Collaborator Author

yschimke commented Mar 17, 2022

What if we make it so each request gets a single response, and let the caller pick what’s in that response? If the caller wants one callback we can give them that, and if they want two callbacks we can give them that too?

Makes sense, I'll simplify to this API.

public fun query (Ljava/lang/String;Lokhttp3/AsyncDns$Callback;)V
}

public final class okhttp3/android/AndroidAsyncDns$Companion {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are not nice from java, but as these are Android specific, I think Kotlin is a good assumption.

/**
* Run with "./gradlew :android-test:connectedCheck" and make sure ANDROID_SDK_ROOT is set.
*/
@ExtendWith(MockWebServerExtension::class)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay!


/**
* Dns implementation based on android.net.DnsResolver, which submits two requests for
* A and AAAA records, and returns the addresses or exception from each before returning.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment out of date?

* @param network network to use, if not selects the default network.
*/
@RequiresApi(Build.VERSION_CODES.Q)
class AndroidAsyncDns(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide public constructor?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Hmmm . . . . actually for Network this is useful . . . hmmmm)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep - I actually liked this bit. There are bugs in stackoverflow because android/wear networking returns differing/empty results across Wifi, LTE, Bluetooth paired connections (on Wear).

The choice is implicit currently with Dns.SYSTEM, and this makes it explicit.

* Class of Dns addresses, such that clients that treat these differently, such
* as attempting IPv6 first, can make such decisions.
*/
enum class DnsClass(val type: Int) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need this be a public API? (I think it has to be??)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep - part of public API.

Copy link
Collaborator

@swankjesse swankjesse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a follow-up I think we could make it so the main OkHttp artifact’s Platform class directs us to use this code when running on the Android platform?

@yschimke yschimke merged commit fc77503 into square:master Mar 20, 2022
@yschimke yschimke deleted the async_dns2 branch May 27, 2023 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

android Relates to usage specifically on Android

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants