-
Notifications
You must be signed in to change notification settings - Fork 9.2k
Introduce Call.Decorator #8800
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
base: master
Are you sure you want to change the base?
Introduce Call.Decorator #8800
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
if (index > callDecorators.lastIndex) { | ||
RealCall(this@OkHttpClient, request, forWebSocket = false) | ||
} else { | ||
callDecorators[index++].newCall(this, request) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mutability might be an issue if someone implements a decorator that the chain twice.
|
||
override fun newCall(request: Request): Call = | ||
if (index > callDecorators.lastIndex) { | ||
RealCall(this@OkHttpClient, request, forWebSocket = false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be some wrapper returned, that utilises the result of the first decorator.
then passes it through the interceptor chain.
Demonstrate a possible Call.Decorator API that handles some of the following cases, without forcing clients to deal exclusively with Call.Factory.
Docs
Todo