Kong / unirest-java

Unirest in Java: Simplified, lightweight HTTP client library.
http://kong.github.io/unirest-java/
MIT License
2.58k stars 591 forks source link

Can't turn auto retries standard on #506

Closed arturzangiev closed 6 months ago

arturzangiev commented 6 months ago

Describe the bug I turned retries on by setting retryAfter(true)

private UnirestInstance unirest = Unirest.primaryInstance();
this.unirest.config().connectTimeout(60000).retryAfter(true);

But i get an exception

java.lang.NullPointerException: Cannot invoke "kong.unirest.core.HttpResponse.getStatus()" because "response" is null
        at kong.unirest.core.RetryStrategy$Standard.isRetryable(RetryStrategy.java:90)
        at kong.unirest.core.BaseRequest.request(BaseRequest.java:346)
        at kong.unirest.core.BaseRequest.thenConsume(BaseRequest.java:298)

Environmental Data:

ryber commented 6 months ago

The retry thing is for HTTP failures that contain a "Retry-After" header. What we have here is no response at all. I'm not really sure how it managed to do that, It really should have thrown some kind ConnectException if you timed out.

You can override the RetryStrategy in the config to give it custom behavior, without any kind of response at all the current strategy can't know how long it needs to wait, but you can set that behavior. Although if you cannot connect after 60000 millies I doubt you will survive another run.

I'm going to add a null check I guess, which will simply skip the retry if there is no response at all

arturzangiev commented 6 months ago

Yeah it makes sense what you saying about the response, but if I change retryAfter(true) to retryAfter(false) everything works fine. The same API and no other changes. In addition to this this missing response behaviour I can see across multiple different API providers. I am running odds comparison website and call around 20 different bookmakers APIs and the behaviour is the same. So it can't be the case that API is not returning the response.

ryber commented 6 months ago

I cannot recreate this. Unirest has a large suite of behavioral tests that stand up a real Javalin server and then makes requests to it. There is a suite of tests for the retry logic, which works in those conditions. If you can provide an recreateable test then we could work with that, but every attempt I made either worked, or resulted in some other IO error

arturzangiev commented 6 months ago

Just created very basic example with one of the open bookie API:

package com.unirestissue.issue;

import kong.unirest.core.Unirest;
import kong.unirest.core.UnirestInstance;

public class Client {
    private UnirestInstance unirest = Unirest.primaryInstance();

    public Client() {
        this.unirest.config().reset().connectTimeout(60000).retryAfter(true);
    }

    public void callApi() {
        try {
            String url = "http://cache.boylesports.com/feeds/INDEX.json";
            this.unirest.get(url).thenConsume(r -> {
                int statusCode = r.getStatus();
                System.out.println(statusCode + " STATUS CODE: " + url);
            });
        } catch (Exception e) {
            e.printStackTrace();
        }
    }
}

When you invoke callApi() method you get:

java.lang.NullPointerException: Cannot invoke "kong.unirest.core.HttpResponse.getStatus()" because "response" is null
        at kong.unirest.core.RetryStrategy$Standard.isRetryable(RetryStrategy.java:90)
        at kong.unirest.core.BaseRequest.request(BaseRequest.java:346)
        at kong.unirest.core.BaseRequest.thenConsume(BaseRequest.java:298)
        at com.unirestissue.issue.Client.callApi(Client.java:16)
        at com.unirestissue.issue.App.main(App.java:12)

If you change .retryAfter(true) to .retryAfter(false) you get success 200 response.

My Pom has only following dependencies:

  <dependencies>
    <dependency>
      <groupId>junit</groupId>
      <artifactId>junit</artifactId>
      <version>4.11</version>
      <scope>test</scope>
    </dependency>
    <dependency>
      <groupId>com.konghq</groupId>
      <artifactId>unirest-java-core</artifactId>
      <version>4.2.1</version>
    </dependency>
  </dependencies>
ryber commented 6 months ago

Ah I see, you are using the .thenConsume method which fully consumed the raw response and its body and then ends up returning null, that part is the problem, it should not return null. I will get a fix in for it to return a type of response that has the basic information but no body.

ryber commented 6 months ago

4.2.3 should have the fix, might take a bit to get though all the Maven central pipelines

arturzangiev commented 6 months ago

Roughly when should I expect this release?

ryber commented 6 months ago

Maven Central is a mystery, anywhere between 1 minute and 1 day. Nobody knows

ryber commented 6 months ago

I'm going to open a issue with Maven Central.

ryber commented 6 months ago

they showed up now. Closing this