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

OkHttpClient.Builder network pinning on Android #8376

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

yschimke
Copy link
Collaborator

@yschimke yschimke commented Apr 20, 2024

Two features in 1 PR.

Can split up, but for discussion as it affects the API.

Cleanly pinning network on Android - #8286
Redo the Async DNS API, but internal for now - #8318

@yschimke yschimke added the android Relates to usage specifically on Android label Apr 20, 2024
@ExperimentalOkHttpApi
object NetworkSelection {
@RequiresApi(Build.VERSION_CODES.Q)
fun OkHttpClient.Builder.withNetwork(network: Network?): OkHttpClient.Builder {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this part of the API.

What happens if we attempt this in the main OkHttp module? We’d have a runtime dependency on Network, but you wouldn’t call this API if you don’t have it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am ok with either.

I guess ultimately we should choose either

a) we build android specific bits in main module, but you just don't use them if not needed.
b) we encourage every android app to include okhttp and okhttp-android and use those bits.

I'm not sure.

callback: Callback,
)

/** Returns a [Dns] that blocks until all async results are available. */
open fun asBlocking(): Dns {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This all feels like a round-peg in a square-hole . . . building an async API and only ever using it in a blocking way.

I think we should build a regular Dns implementation for Android’s DNS stuff, and eject AsyncDns from our API until we’re ready to take advantage of it being async

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was using it here, so you can have per-network Dns. I don't think that's possible without the async android resolver API.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ejecting the AsyncDns API for now. it's internal and moved to okhttp-android.

@yschimke yschimke marked this pull request as ready for review April 20, 2024 17:18
@yschimke yschimke changed the title Draft of network pinning on Android OkHttpClient.Builder network pinning on Android Apr 20, 2024
@@ -27,33 +27,6 @@ public final class okhttp3/Address {
public final fun url ()Lokhttp3/HttpUrl;
}

public abstract interface class okhttp3/AsyncDns {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@swankjesse no longer public API anywhere

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.

2 participants