Closed rsenden closed 9 months ago
The Interceptor is more than capable of acting as an observer to the process, if there are 4 retries it will be invoked 4 times. The problem is that if Unirest sleeps the thread waiting for its chance to run again, its also blocking you from observing the behavior. This could be be sidestepped by running it in the future like this:
Unirest.config().retryAfter(true).interceptor(new Interceptor() {
@Override
public void onResponse(HttpResponse<?> response, HttpRequestSummary request, Config config) {
if(response.getStatus() == 429){
System.out.println("Hey Its paused!");
}
}
});
CompletableFuture<HttpResponse> future = CompletableFuture.supplyAsync(() ->
Unirest.get("http://localhost").asEmpty()
);
This is of course silly, Unirest already has a async mode. You should be able to use that right? Well, no, but that has to do with some legacy reasons, Apache was quite hostile to the whole double async thing going on. I think with Unirest 5 it should be OK and we can look at that again
Thanks for the suggestion. Unfortunately, after some more experimenting, I found out that retryAfter
doesn't work for us at all because our remote system doesn't return a Retry-After
header; instead it returns an X-Rate-Limit-Reset
header. Unirest (at least our version) doesn't seem to allow for customizing the expected header name.
Even if we could customize the expected header name, we'd still have an issue with having the interceptor throw an exception when the request fails. All of our application code is using calls like unirestInstance.get(...).asObject(...).getBody()
, and we'd like unirestInstance
to be configured to throw an exception if the request fails. So, we have the following in our interceptor:
public void onResponse(HttpResponse<?> response, HttpRequestSummary requestSummary, Config config) {
if ( !response.isSuccess() ) {
throw new UnexpectedHttpResponseException(response, requestSummary);
}
}
However, when throwing an exception on a 429 response, the Unirest retry mechanism seems to be skipped altogether. If we do not throw an exception on 429 responses, then the call to unirestInstance.get(...).asObject(...).getBody()
will succeed even if the server returned a 429 response on the last retry attempt, causing incorrect application behavior.
Potentially, we could have our interceptor only throw an exception after seeing a 429 response for x number of times, but this would be quite dirty and difficult to handle in multithreaded applications (as the interceptor would need to keep track of simultaneous requests and responses).
Ideally, Unirest should have a fully customizable retry mechanism to allow for use cases like this. In the meantime, any suggestions on how to best implement this in a generic way with current Unirest versions, with the least amount of changes to our application code? Any Unirest (configuration) feature that might be useful to implement this in a generic way that I may have missed?
In case it helps anyone, I'm currently working around this Unirest limitation by configuring a ServiceUnavailableRetryStrategy
on the underlying Apache HttpClient:
protected final void configure(UnirestInstance unirest) {
unirest.config().httpClient(this::createClient);
...
}
private ApacheClient createClient(Config config) {
return new ApacheClient(config, this::configureClient);
}
private void configureClient(HttpClientBuilder cb) {
cb.setServiceUnavailableRetryStrategy(new RateLimitRetryStrategy());
}
However, should we ever wish to upgrade to Unirest 4.x, we'll likely need to find a different approach as Apache HttpClient is no longer being used in that version.
unirestInstance.get(...).asObject(...).getBody() will succeed even if the server returned a 429 response on the last retry attempt, causing incorrect application behavior.
this is not incorrect behavior, Unirest as a rule does not throw exceptions, except in "exceptional" situations. but getting a response that is technically not a 2xx response is not one of those. It is always expected that it will return a response regardless of status. This is also why having your own code throw an exception in the interceptor stops the retries, becuase exceptions mean we need to stop.
If you want to observe 429's you can do that by injecting a observer object in the interceptor.
I 100% agree that Unirest by default shouldn't throw an exception on non-2xx responses, but on the other hand, throwing an exception does allow for simpler application code as you don't need to worry about non-2xx responses on every individual REST call. It's easy to forget adding an ifFailure
call or explicitly checking the response status and thereby causing unexpected application behavior on non-2xx responses.
Anyway, the primary issue is that the Unirest retryAfter
mechanism doesn't work for us, and Unirest doesn't allow for implementing custom retry mechanisms. Even if we were to handle retries and error handling in explicit calls to ifFailure
on every response, it would be difficult/impossible to retry individual requests in a PagedList
as we wouldn't have access to the individual HttpRequest
instance that needs to be retried.
Ideally, Unirest should allow for configuring customizable retry mechanisms, with the current retry mechanism being just one potential implementation. I haven't thought this true in detail, but potentially we could have interfaces / methods like the following (based on Unirest 3.x):
RequestExecutor
:
public <E> HttpResponse<E> request(Config config, Function<RawResponse, HttpResponse<E>> transformer, Class<?> resultType)
BaseRequest
to a separate class; exposing a lot of internal details, but allows for fully customizable request executionRetryHandler
(or possibly integrate into existing Interceptor
interface):
public <E> HttpResponse<E> onResponse(HttpResponse<E> currentResponse, Supplier<HttpResponse<E>> retry)
currentResponse
if no retry is necessary, or retry.get()
(one or more times) if the request needs to be retried public <E> HttpResponse<E> request(Supplier<HttpResponse<E>> requestExecutor)
requestExecutor.get()
at least once but potentially multiple times if the request need to be retriedAs noted in an earlier comment, we currently have a work-around for Unirest 3.x by utilizing Apache HttpClient ServiceUnavailableStrategy
, but we won't be able to use the same work-around for Unirest 4.x as it's no longer based on Apache HttpClient.
So in order for unirest to support what you want we need the following:
As for the observing, or the logic around making the requests, I don't see that changing. Its perfectly observable as is, without throwing any exceptions or changing how things are executed. If we support retry in async then its even better.
In the end, some consumers are going to want even more customization, and the answer for that is to write your own wrapper for Unirest. Unirest itself should support most people most of the time, but it cannot be infinitely customizable.
I understand that Unirest cannot be infinitely customizable, but I think supporting custom retry logic would be of major benefit for many use cases, not too difficult to implement based on my earlier suggestions, and overall result in a better Unirest design compared to having fixed retry logic with hard-coded statuses and header names.
Also, custom retry logic is not something that can be easily accomplished through a wrapper that's built on top of Unirest, as this would require re-implementing other functionality like paging as well (as explained above).
Based on this discussion, any chance that support for custom retry logic can be added in an upcoming Unirest version?
I'm going to support overriding the why (status, headers, etc), and async.
Great, thanks!
4.0.9 exposes a RetryStrategy in the config which allows a consumer to override much of the behavior.
You can either write your own entire strategy or override the parts of RetryStrategy.Standard you want.
If you were to override the waitFor method you could call an observer to notify the user that the request is pausing:
@Override
void waitFor(long millies) {
observer.tellUserRequestIsPaused();
super.waitFor(millies);
observer.tellUserRequestIsContinuting();
}
Great, thanks! This will definitely be helpful to get rid of our Apache HttpClient-based work-around once we're ready to upgrade to Unirest 4.x.
On Sun, Sep 24, 2023, 15:24 Ryan Bergman @.***> wrote:
4.0.9 exposes a RetryStrategy in the config which allows a consumer to override much of the behavior.
You can either write your own entire strategy or override the parts of RetryStrategy.Standard you want.
If you were to override the waitFor method you could call an observer to notify the user that the request is pausing:
@Overridevoid waitFor(long millies) { observer.tellUserRequestIsPaused(); super.waitFor(millies); observer.tellUserRequestIIsContinuting(); }
— Reply to this email directly, view it on GitHub https://github.com/Kong/unirest-java/issues/491#issuecomment-1732569944, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACB4GAQO4TZE6KOLQMQRY2TX4AX73ANCNFSM6AAAAAA4TFELHE . You are receiving this because you authored the thread.Message ID: @.***>
retry-after now works with async requests as of 4.0.11. This is probably all the more I'm going to do on this issue.
Currently using Unirest 3.14.5, planning to upgrade to 4.x later. Is there any way for configuring custom retry behavior on a
UnirestInstance
in either Unirest 3.x or 4.x?We'd like to retry rate-limited requests, however
Config::retryAfter
doesn't work for us for two reasons:onFailure
all around, we configure an interceptor that throws an exception on unsuccessful responses. If we throw an exception on status 429, then the Unirest retry-after logic is skipped. If we don't throw an exception on status 429, then an exception will never be thrown even after exceeding the maximum retry limit.I've been looking at some potential work-arounds, like:
Interceptor
, for example:onRequest
method, retry on 429, however this method doesn't allow for returning a responseonResponse
method if the original response was 429, however:HttpRequest
instance (could potentially be stored inInterceptor::onRequest
, although might be difficult in multi-threaded apps)HttpResponse<?>
, i.e., should we returnasString()
,asObject()
, ...onFail
method does allow for returning a new response, however this method seems to be invoked only on low-level errors like time-outs, not on the customUnirestException
that we throw in theonResponse
methodServiceUnavailableRetryStrategy
directly on the Apache HttpClientAny other ideas for implementing this?