Skip to content
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

Can't turn auto retries standard on #506

Closed
arturzangiev opened this issue Dec 16, 2023 · 10 comments
Closed

Can't turn auto retries standard on #506

arturzangiev opened this issue Dec 16, 2023 · 10 comments

Comments

@arturzangiev
Copy link

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:

  • OpenJDK Runtime Environment Temurin-17.0.1+12 (build 17.0.1+12)
  • unirest-java-core 4.2.1
@ryber
Copy link
Collaborator

ryber commented Dec 16, 2023

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
Copy link
Author

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
Copy link
Collaborator

ryber commented Dec 16, 2023

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
Copy link
Author

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
Copy link
Collaborator

ryber commented Dec 16, 2023

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 added a commit that referenced this issue Dec 16, 2023
…that other things like retry logic and interceptors can function properly
@ryber
Copy link
Collaborator

ryber commented Dec 16, 2023

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

@arturzangiev
Copy link
Author

Roughly when should I expect this release?

@ryber
Copy link
Collaborator

ryber commented Dec 16, 2023

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

@ryber
Copy link
Collaborator

ryber commented Dec 18, 2023

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

@ryber
Copy link
Collaborator

ryber commented Dec 18, 2023

they showed up now. Closing this

@ryber ryber closed this as completed Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants