Conversation
harrytmthy
left a comment
There was a problem hiding this comment.
Hi @yschimke, I noticed the current API is a bit ambiguous about whether decorators are intended to be "wrappers" or can "short-circuit" the call. If one decorator short-circuits (i.e. returns a call without calling chain.newCall()), it effectively skips all decorators after it, and the rest of the chain is unaware.
Short-circuiting might be intentional in cases like:
if (noNetworkAvailable()) {
return FailingCall("No network")
}
return chain.newCall(request)This ambiguity could lead to some issues:
- Ordering becomes critical and fragile: Decorators that must always run (e.g. security, metrics, tracing) need to be registered first, but this isn't obvious or enforced.
- Invisible dependencies: One decorator might assume another has run (e.g. metrics before auth).
- Debugging becomes tricky: "Why isn't tracing working?" → "Oh, the offline checker short-circuited the chain."
Would it be worth documenting this explicitly? Something like:
Decorators can optionally short-circuit the call chain. If so, be aware that any decorators added after them will not be invoked.
Alternatively, if short-circuiting is rare or discouraged, it might make sense to split the interface (e.g. CallWrapper vs CallInterceptor), though that might be overkill for now.
|
@harrytmthy yep, definitely worth documenting. Interceptors document exactly this, so I'll add. In some ways this is the point, while interceptors surround the execution of the Call. App interceptor - 0..n executions This Call.Decorator are configured centrally in order to allow to short circuit, modify the original request or even redirect the call to another client instance. |
android-test/src/androidTest/java/okhttp/android/test/OffMainThread.kt
Outdated
Show resolved
Hide resolved
|
@pyricau I'm thinking I might need access to the OkHttpClient in my decorator to spawn new derived instances. Will the current API meet your needs? |
It does yes, I only need access to the request. Technically you can actually access the client, by forwarding |
|
@pyricau in my case, I need to fork off new instances of OkHttpClient, so can't let the chain proceed normally. |
|
@harrytmthy I think you were right, so I updated the docs to say
example from #8376 |
5482806 to
f39a193
Compare
|
Replacing by next attempt #9148 |
Demonstrate a possible Call.Decorator API that handles some of the following cases, without forcing clients to deal exclusively with Call.Factory.
Docs