Kong/unirest-java

Configure custom retry behavior

rsenden opened this issue · 12 comments

rsenden commented

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:

  • We'd like to inform the user / log a message that we're waiting for the rate limit to be reset
  • To avoid bothering about unsuccessful responses with 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:

  • Implementing a Unirest Interceptor, for example:
    • Already execute the request in the onRequest method, retry on 429, however this method doesn't allow for returning a response
    • Retry the request in the onResponse method if the original response was 429, however:
      • There's no access to the original HttpRequest instance (could potentially be stored in Interceptor::onRequest, although might be difficult in multi-threaded apps)
      • Again no possibility to 'override' / return a new response
    • For both of the above, we don't know the generic type of the HttpResponse<?>, i.e., should we return asString(), asObject(), ...
    • The 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 custom UnirestException that we throw in the onResponse method
  • Configuring a custom ServiceUnavailableRetryStrategy directly on the Apache HttpClient

Any other ideas for implementing this?

ryber commented

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

rsenden commented

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?

rsenden commented

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.

ryber commented

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.

rsenden commented

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):

  • Interface RequestExecutor:
    • public <E> HttpResponse<E> request(Config config, Function<RawResponse, HttpResponse<E>> transformer, Class<?> resultType)
    • Basically moving the existing method from BaseRequest to a separate class; exposing a lot of internal details, but allows for fully customizable request execution
  • Interface RetryHandler (or possibly integrate into existing Interceptor interface):
    • public <E> HttpResponse<E> onResponse(HttpResponse<E> currentResponse, Supplier<HttpResponse<E>> retry)
    • Implementations simply check current response status, and either return currentResponse if no retry is necessary, or retry.get() (one or more times) if the request needs to be retried
  • Variation of the above:
    • public <E> HttpResponse<E> request(Supplier<HttpResponse<E>> requestExecutor)
    • Implementations call requestExecutor.get() at least once but potentially multiple times if the request need to be retried

As 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.

ryber commented

So in order for unirest to support what you want we need the following:

  • Allow custom logic for determining if the request needs to retry. This could include things like
    • Modifying which status to do it on
    • or maybe status is not even the trigger, we should allow the consumer to figure that out.
    • Maybe both of the above
  • Allow custom logic for determining how long to wait
    • The Retry-After header documents two formats, but folk should be able to pick a different header, or a different format for the value, or none (hard-code for x seconds)

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.

rsenden commented

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?

ryber commented

I'm going to support overriding the why (status, headers, etc), and async.

  • 👍1
rsenden commented

Great, thanks!

ryber commented

4.0.9 exposes a RetryStrategy in the config which allows a consumer to override much of the behavior.

see https://github.com/Kong/unirest-java/blob/main/unirest/src/main/java/kong/unirest/core/RetryStrategy.java

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();
}
rsenden commented
ryber commented

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.