Allow OkHttpClient overrides in Interceptor #9108
Conversation
okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/RealInterceptorChain.kt
Outdated
Show resolved
Hide resolved
okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/RealInterceptorChain.kt
Outdated
Show resolved
Hide resolved
okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/RealInterceptorChain.kt
Outdated
Show resolved
Hide resolved
# Conflicts: # okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/connection/RealCall.kt # okhttp/src/jvmTest/kotlin/okhttp3/internal/connection/RouteSelectorTest.kt
swankjesse
left a comment
swankjesse
left a comment
There was a problem hiding this comment.
I think this is pretty rad actually. The most difficult part is gonna be making the tests?
okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/connection/RealCall.kt
Outdated
Show resolved
Hide resolved
|
@swankjesse I'll follow up with more specific samples, such as Android Network Pinning. |
swankjesse
left a comment
swankjesse
left a comment
There was a problem hiding this comment.
Love it
I would like more confidence that the override value is actually used, different from whether it’s reflected in the chain’s own properties. That’s necessary but not sufficient.
okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/connection/RealCall.kt
Outdated
Show resolved
Hide resolved
|
@swankjesse still have a few tests to fix, but ready for review |
| if (sink | ||
| .timeout() | ||
| .timeoutNanos() | ||
| .nanoseconds.inWholeMilliseconds == 10L |
There was a problem hiding this comment.
@swankjesse I tried to implement this using
.socketFactory(
DelayingSocketFactory(onWrite = {
Thread.sleep(100L)
}),
But if I break point at Thread.sleep, then socket.sink().timeout().timeoutNanos() is always cleared (0).
I suspect a bug with write timeout, but this replicates a timeout just by checking if we have the bad timeout value.
| interceptors += RetryAndFollowUpInterceptor(client) | ||
| interceptors += BridgeInterceptor(client.cookieJar) | ||
| interceptors += CacheInterceptor(this, client.cache) | ||
| interceptors += RetryAndFollowUpInterceptor() |
There was a problem hiding this comment.
@swankjesse this seems like a nice win.
2 participants
This pull request introduces significant enhancements to the
Interceptor.ChainAPI in OkHttp, allowing much more fine-grained and dynamic control over the HTTP client's configuration on a per-call basis.Let's delve into it;
API Extensions for Interceptor.Chain
Interceptor.Chaininterface for properties such as DNS, proxy, cache, authenticators, SSL factories, certificate pinners, connection pool, cookie jar, hostname verifier, and retry settings. These methods allow interceptors and clients to override or access the OkHttpClient's configuration dynamically within the interceptor chain.Internal Refactoring to Use Chain Properties
BridgeInterceptorandCacheInterceptorto use the new properties fromInterceptor.Chaininstead of being directly constructed with dependencies likeCookieJarorCache. This makes interceptors more decoupled and flexible.Extensive test of overrides
InterceptorOverridesTestexhaustively tests that overrides can be applied by an application interceptor and observed from a network interceptor. That they can't be set in a network interceptor. And finally that bad defaults on OkHttpClient, can be saved by good overrides.