feign
feign copied to clipboard
OpenFeign
Retry based on status code list given as parameter
As written in #731 , the current logic treats all IOExceptions from a Client as something that is network related and that's why it try to do retry but not all of the IOExceptions are related to connectivity. There are IOExceptions that I don't want to retry after I got them. (for example, when trying to serialize/deserialize or dealing with files).
In Addition, maybe we can add a optional parameter to do a retry based on list of status codes we get as parameter.
IOExceptions that occur during the encoding and decoding phases do not trigger a request retry, they are outside of the scope of the client execution. Also, as defined in the Client interface, Client implementations should only throw IOExceptions for communication and transient errors.
https://github.com/OpenFeign/feign/blob/bbdf7320bbacf0dfcef043e3e57a2fd4bbf46265/core/src/main/java/feign/SynchronousMethodHandler.java#L105-L114
At this point encoding has already been performed. Decoding occurs farther down at line 140. Any exception that occurs there is propagated to the caller and no retry is triggered.
You can control the retry based on status by using the ErrorDecoder. You can implement your own where you can inspect the Response and explicitly throw a RetryableException to trigger a retry based on the response. This is outlined in the README - Error Handling
I kinda think RetryableException was a mistake. Why not just have the retryer take the exception ("retryable" or otherwise) and let it determine whether or not the exception truly is "retryable".
That said, I don't use any retryer stuff myself (just whatever you get by default -- if anything) and won't until I find a case of need (if there is a default retryer, it must be working great for me so far). So I'm only speaking theoretically here, not from experience using it.
I'm kind of a fan of the RetryableException pattern. Not sure about IOException being retryable by default. But - if your ErrorDecoder gives back a RetryableException (or one that inherits from it) - then why not retry?
to add: I've used a retryer before. I basically had a situation where we wanted to authenticate once, then use the authentication cookie for all subsequent requests (without authenticating every time) - so it was a bit of a combination of an interceptor - which would authenticate if unauthenticated, then inject the token into the appropriate header into the request every time after that.
However, after a certain time (TTL of the token basically) the authentication would stop working (token expired) and we had to re-authenticate. To do so, we used a Retryer that would look at the RetryableException and whenever we got a 401 (Unauthorized), it would basically mark the Authentication Interceptor as "unauthenticated" and then retry - basically making it so the next request would re-authenticate, update the token and inject it as a header.
Though a little complex, differentiating from 500's (which generally mean "back off, I'm having trouble"), 401's (which, in our case, you can retry) and the rest made it so that if we had issues we didn't force ourselves to 'restart' the app or so - while being smart about how often we hit the authentication service/etc.
Just food for thought!
I've looked into this and created an alternative implementation in our experimental project:
https://github.com/OpenFeign/feignx/pull/45
The approach relies on Conditions, which evaluate the results of the request. This allows for retrying not only on exception but any piece of information provided in the Response. It's a more robust approach that provides users with more control over the retry process than what we provide now. The drawback is that it is completely different and will break existing users.
Take a look. I appreciate any feedback.
Take a look. I appreciate any feedback
From a quick glance I think this model for retry would fit my use case (same as @saintf ). In addition I share this pattern with other company teams so I like the retry pattern as presented in feignx because it is easier to combine conditions.
I did not check if it is executed before or after ErrorDecoder but I think both information could be useful to build conditions (depending on the use case); I would only rely on the bare response but teams may want to use the decoded error if the remote service provides information in a complex format.
BTW thanks for the work and good idea regarding feignx as an incubator
Maybe, what we could do is expand the Retryer scope a little bit https://github.com/OpenFeign/feign/blob/master/core/src/main/java/feign/Retryer.java
Would include a new method to retry based on the Response, so any logic could be added to retry based on ANY response.
Would that make sense?
I think it would help for these cases. I have no specific suggestion on implementation.