[Not to land] Testing Android HttpEngine with Call Decorator #9013
Conversation
yschimke
changed the title
# Conflicts: # gradle/libs.versions.toml
| * | ||
| * A Decorator that changes the OkHttpClient should typically retain later decorators in the new client. | ||
| */ | ||
| fun interface Decorator { |
There was a problem hiding this comment.
Is adding a brand new public abstraction just for this really worth it? It seems more straightforward to add cancellation support to interceptors (#7164), and treat the HttpEngine interceptor as a "special snowflake" in RealCall. Arguably this wouldn't be as clean as RealCall would need to be aware of HttpEngineInterceptor's existence, but it's not like your approach is squeaky clean either, given you have to refer to OkHttp internals in HttpEngineDecorator (e.g. manually calling BridgeInterceptor which is an internal class). Since neither approach achieves perfect decoupling between the HttpEngine bridge and OkHttp internals, I'd be inclined to go for the most straightforward one.
There was a problem hiding this comment.
It exists for other reasons, and is just the best way to express this problem. Call.Factory could be similar, but isn't well adopted. I fear we lost that argument.
manually calling BridgeInterceptor which is an internal class
Yep, we should fix this, and have a clean set of APIs for bridging interceptors like Cronet
I'd be inclined to go for the most straightforward one.
I don't think we would do that if it meant treating HttpEngine as a special snowflake in RealCall.
|
|
||
| @SuppressSignatureCheck | ||
| class HttpEngineCallDecorator( | ||
| internal val httpEngine: HttpEngine, |
There was a problem hiding this comment.
It is debatable whether this should use HttpEngine directly, or if it should use the Cronet API wrapper (which would then call HttpEngine). The Cronet API wrapper is more flexible and would make it possible to use other variants of Cronet (such as embedded of Play Services) if we ever want to, but the downside is it would involve OkHttp taking a new dependency on the (very small) cronet-api Maven package.
There was a problem hiding this comment.
I think that dependency is the blocker here.
But also, a first version of this should probably be a v2.0 of the google/cronet-transport-for-okhttp
So that can do whatever makes sense?
|
|
||
| @SuppressSignatureCheck | ||
| class HttpEngineCallDecorator( | ||
| internal val httpEngine: HttpEngine, |
There was a problem hiding this comment.
It is debatable whether we want to let the user choose which HttpEngine to use. Arguably this code should instantiate HttpEngine and should configure it by interpreting OkHttpClient settings. Otherwise we may end up backing ourselves into a corner by providing too much flexibility to the user.
There was a problem hiding this comment.
Yep, I'm not aware of what we expect an app developer to configure in typical cases?
Do we want them to allowlist their known hosts for HTTP/3?
But no objection from me. Your call.
2 participants
Uh oh!
There was an error while loading. Please reload this page.