OpenFeign / feign

Feign makes writing java http clients easier
Apache License 2.0
9.36k stars 1.91k forks source link

Call Timeout(OKHTTP) not working for my Open Feign Config. #2342

Open parthiv-groww opened 4 months ago

parthiv-groww commented 4 months ago

I have made okhttp config bean and used that for my feign client config. I need to use call timeout in my service and it is not working the way it should. I have also configured read and connect timeout and they are working fine.

@Bean Feign feignBuilder(@Qualifier("cdpOkHttpClient") OkHttpClient cdpOkHttpClient) { return Feign .builder() .client(new feign.okhttp.OkHttpClient(cdpOkHttpClient)) .encoder(new SpringEncoder(messageConverters)) .decoder(new SpringDecoder(messageConverters)) .retryer(new CDPRetryerConfig()) .build(); }

My Okhttp config

`@Bean(name = "baseOkHttpClient") public OkHttpClient baseOkHttpClient() { return new OkHttpClient.Builder() .readTimeout(500, TimeUnit.MILLISECONDS) .connectTimeout(500, TimeUnit.MILLISECONDS) .callTimeout(10,TimeUnit.MILLISECONDS) .build(); }

@Bean(name = "cdpOkHttpClient")
public OkHttpClient cdpOkHttpClient(@Qualifier("baseOkHttpClient") OkHttpClient okHttpClient) {
    return okHttpClient.newBuilder()
        .readTimeout(rewardsConfig.getDpBroker().getReadTimeout(), TimeUnit.MILLISECONDS)
        .connectTimeout(rewardsConfig.getDpBroker().getConnectTimeout(), TimeUnit.MILLISECONDS)
            .callTimeout(rewardsConfig.getDpBroker().getCallTimeout(),TimeUnit.MILLISECONDS)
        .build();
}`

and I am picking the timeout values from application.yml file

I have also tried using writing unit test to test this configuration. With custom Okhttp bean it works fine but does not work when used with Feign client.

gromspys commented 4 months ago

@parthiv-groww it looks like you need to add options for your feign client to use the same OkHttpClient:

@Bean 
Feign feignBuilder(@Qualifier("cdpOkHttpClient") OkHttpClient cdpOkHttpClient) { 
    return Feign.builder() 
        .client(new feign.okhttp.OkHttpClient(cdpOkHttpClient)) 
        .options(new Request.Options(cdpOkHttpClient.connectTimeoutMillis(), TimeUnit.MILLISECONDS, 
                                     cdpOkHttpClient.readTimeoutMillis(), TimeUnit.MILLISECONDS,
                                     cdpOkHttpClient.followRedirects())
        .encoder(new SpringEncoder(messageConverters)) 
        .decoder(new SpringDecoder(messageConverters)) 
        .retryer(new CDPRetryerConfig())
        .build(); 
}
parthiv-groww commented 4 months ago

Hello @gromspys , thank you for your response. The problem that I am facing here is using call Timeout in my feign client using Okhttp. I have tested my both read and connect timeout using the configs that I have shared before. It is working as expected but the real issue is with using call timeout in the similar way. Okhttp supports call timeout but it is not properly configurable with feign client and hence unable to use it. Using the (.options) configuration, I am able to use both read and connect timeout but I am not sure how to use that for call timeout. My retryer is also working as expected.

gromspys commented 4 months ago

Could you please give your unit test? I tried to reproduce, but it looks like call timeout works as expected:

  public interface OkHttpClientTestApi {

    @RequestLine("GET /{delay}")
    Response get(@Param("delay") String delay);
  }

  @Test
  public void whenCallTimeoutExceeded_thenInterruptedIOException() {
    okhttp3.OkHttpClient client = new okhttp3.OkHttpClient.Builder()
            .callTimeout(1, TimeUnit.SECONDS)
            .build();

    okhttp3.Request request = new okhttp3.Request.Builder()
            .url("https://httpbin.org/delay/2")
            .build();

    Throwable thrownClient = catchThrowable(() -> client.newCall(request).execute());
    assertThat(thrownClient).isInstanceOf(InterruptedIOException.class);

    OkHttpClientTestApi api = Feign.builder()
            .client(new OkHttpClient(client))
            .target(OkHttpClientTestApi.class, "https://httpbin.org/delay/");

    Throwable thrownApi = catchThrowable(() -> api.get("2"));
    assertThat(thrownApi).isInstanceOf(RetryableException.class);
  }
parthiv-groww commented 4 months ago
wireMock.stubFor(post(urlPathEqualTo("/v1/readFeatureValues")).willReturn(aResponse().withFixedDelay(1001).withStatus(200)));
       Throwable throwable = catchThrowable(() -> cdpDao.getFeatures("user1", CdpRequestDto.builder().build()));
       assertThat(throwable).isInstanceOf(InterruptedIOException.class);

This is my unit test . Even I did try with okhttp client in similar manner and it did work but not with feign client.

OkHttpClient client = new OkHttpClient.Builder() .callTimeout(1, TimeUnit.MILLISECONDS) .build(); Request request = new Request.Builder().url("http://localhost:8030").build(); Throwable throwable= catchThrowable(()-> client.newCall(request).execute()); assertThat(throwable).isInstanceOf(InterruptedIOException.class);

And here is how my cdpDao is annotated with Feign client:

@FeignClient(name = "cdp-client", url = "${url}" , configuration = FeignClientConfig.class) public interface CdpDao

One more thing, in the second assert in your code, dont we need to check with "InterruptedIOException.class"

gromspys commented 4 months ago

As I remember feign client has default retryer. In unit test we can change RetryableException to InterruptedIOException:

assertThat(thrownApi.getCause()).isInstanceOf(InterruptedIOException.class);
parthiv-groww commented 4 months ago

So can you please suggest any changes that we need to do so that we are able to use call timeout?

gromspys commented 4 months ago

It's hard to say what is wrong without full configuration. But it looks like your configuration was ignored:

@Bean 
Feign feignBuilder(@Qualifier("cdpOkHttpClient") OkHttpClient cdpOkHttpClient) { 
    return Feign.builder()
        .client(new feign.okhttp.OkHttpClient(cdpOkHttpClient)) 
        .encoder(new SpringEncoder(messageConverters)) 
        .decoder(new SpringDecoder(messageConverters)) 
        .retryer(new CDPRetryerConfig()) 
        .build(); 
}

Try to remove annotation @FeignClient(name = "cdp-client", url = "${url}" , configuration = FeignClientConfig.class) and build your client manually:

@Bean 
CdpDao cdpDao(@Qualifier("cdpOkHttpClient") OkHttpClient cdpOkHttpClient, @Value("url") String url) { 
    return Feign.builder()
        .client(new feign.okhttp.OkHttpClient(cdpOkHttpClient)) 
        .encoder(new SpringEncoder(messageConverters)) 
        .decoder(new SpringDecoder(messageConverters)) 
        .retryer(new CDPRetryerConfig()) 
        .target(CdpDao.class, url); 
}
parthiv-groww commented 4 months ago

Hey @gromspys I did write an exact test case the way you did .

@Test
    public void sampleCallTimeoutTest(){
        long start = System.currentTimeMillis();
        Throwable throwable = catchThrowable(()-> testDao.get("3"));
        long end = System.currentTimeMillis();
        log.info("elasped time {}", end-start);
        assertThat(throwable).isInstanceOf(InterruptedIOException.class);
    }

My sample Interface

public interface TestDao {
    @RequestLine("POST /{delay}")
    Response get(@Param("delay") String delay);

}

My sample configs

import feign.Feign;
import okhttp3.OkHttpClient;
import org.springframework.beans.factory.ObjectFactory;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Qualifier;
import org.springframework.boot.autoconfigure.http.HttpMessageConverters;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;

@Configuration
public class FeignClientConfig {
    @Autowired
    private ObjectFactory<HttpMessageConverters> messageConverters;

    @Bean
    public TestDao feignBuilder(@Qualifier("cdpOkHttpClient") okhttp3.OkHttpClient cdpOkHttpClient) {
        return  Feign
                .builder()
                .client(new feign.okhttp.OkHttpClient(cdpOkHttpClient))
                .target(TestDao.class, "http://localhost:8090");

    }
}

package com.example.demo.Config;

import lombok.RequiredArgsConstructor;
import okhttp3.OkHttpClient;
import org.springframework.beans.factory.annotation.Qualifier;
import org.springframework.boot.context.properties.EnableConfigurationProperties;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;

import java.util.concurrent.TimeUnit;

@Configuration
@RequiredArgsConstructor
public class OkHttpClientConfig {

    @Bean(name = "baseOkHttpClient")
    public okhttp3.OkHttpClient baseOkHttpClient() {
        return new okhttp3.OkHttpClient.Builder()
                .readTimeout(500,TimeUnit.MILLISECONDS)
                .connectTimeout(500, TimeUnit.MILLISECONDS)
                .callTimeout(10,TimeUnit.MILLISECONDS)
                .build();
    }

    @Bean(name = "cdpOkHttpClient")
    public OkHttpClient cdpOkHttpClient(@Qualifier("baseOkHttpClient") okhttp3.OkHttpClient okHttpClient) {
        return okHttpClient.newBuilder()
                .readTimeout(500,TimeUnit.MILLISECONDS)
                .connectTimeout(500, TimeUnit.MILLISECONDS)
                .callTimeout(1000,TimeUnit.MILLISECONDS)
                .build();
    }
}

My pom.xml file

<dependency>
            <groupId>io.github.openfeign</groupId>
            <artifactId>feign-core</artifactId>
            <version>13.2.1</version>
        </dependency>
        <dependency>
            <groupId>io.github.openfeign</groupId>
            <artifactId>feign-okhttp</artifactId>
            <version>13.1</version>
        </dependency>

I set a call timeout of 100ms and started a timer to evaluate the exact time the thread runs for. Even though the call timeout is set , the thread duration for that particular test is roughly around 1300 ms . With call timeout set, it should probably get terminated.

gromspys commented 4 months ago

It looks like there is default retryer bacause I'm getting 6300 ms. Try to set .retryer(Retryer.NEVER_RETRY)

parthiv-groww commented 4 months ago
import feign.Retryer;
import okhttp3.OkHttpClient;
import org.springframework.beans.factory.ObjectFactory;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Qualifier;
import org.springframework.boot.autoconfigure.http.HttpMessageConverters;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;

@Configuration
public class FeignClientConfig {
    @Autowired
    private ObjectFactory<HttpMessageConverters> messageConverters;

    @Bean
    public TestDao feignBuilder(@Qualifier("cdpOkHttpClient") okhttp3.OkHttpClient cdpOkHttpClient) {
        return  Feign
                .builder()
                .client(new feign.okhttp.OkHttpClient(cdpOkHttpClient))
                .retryer(Retryer.NEVER_RETRY)
                .target(TestDao.class, "http://localhost:8090");

    }
}

I have made the suggested changes and my call timeout is 10ms but still my total thread duration is somewhat around 90ms (which suggests that call timeout is not working). And I am not getting the required InterruptedIOException.

gromspys commented 4 months ago

Need some time for initialization. Try to call twice in the same test and compare time. To get InterruptedIOException exception use throwable.getCause()

parthiv-groww commented 4 months ago
    @Test
    public void sampleCallTimeoutTest(){
        long start = System.currentTimeMillis();
        Throwable throwable = catchThrowable(()-> testDao.get("3"));
        long end = System.currentTimeMillis();
        long start1 = System.currentTimeMillis();
        Throwable throwable1 = catchThrowable(() -> testDao.get("4"));
        long end1 = System.currentTimeMillis();
        log.info("elasped time {}", end-start);
        log.info("elasped second time {}", end1-start1);
        assertThat(throwable.getCause()).isInstanceOf(InterruptedIOException.class);
    }

This is the changes that I have made, My call timeout is 10ms

2024-03-15T10:41:58.796+05:30  INFO 30665 --- [           main] c.e.demo.Service.PortfolioServiceTest    : elasped time 88
2024-03-15T10:41:58.796+05:30  INFO 30665 --- [           main] c.e.demo.Service.PortfolioServiceTest    : elasped second time 2

java.lang.AssertionError: 
Expecting actual throwable to be an instance of:
  java.io.InterruptedIOException
but was:
  java.net.ConnectException: Failed to connect to localhost/[0:0:0:0:0:0:0:1]:8090

and this was my output.

gromspys commented 4 months ago

Looks like the url http://localhost:8090 is not working. Try using https://httpbin.org/delay/ instead.

parthiv-groww commented 4 months ago
@Bean(name = "baseOkHttpClient")
    public OkHttpClient baseOkHttpClient() {
        return new OkHttpClient.Builder()
            .readTimeout(500, TimeUnit.MILLISECONDS)
            .connectTimeout(500, TimeUnit.MILLISECONDS)
                .callTimeout(100,TimeUnit.MILLISECONDS)
            .build();
    }

    @Bean(name = "cdpOkHttpClient")
    public OkHttpClient cdpOkHttpClient(@Qualifier("baseOkHttpClient") OkHttpClient okHttpClient) {
        return okHttpClient.newBuilder()
            .readTimeout(1000, TimeUnit.MILLISECONDS)
            .connectTimeout(1000, TimeUnit.MILLISECONDS)
                .callTimeout(10,TimeUnit.MILLISECONDS)
            .build();
    }

Now this is my config for okhttp

@Bean
    CdpDao feignBuilder(@Qualifier("cdpOkHttpClient") OkHttpClient cdpOkHttpClient) {
        return Feign
            .builder()
            .client(new feign.okhttp.OkHttpClient(cdpOkHttpClient))
            .encoder(new SpringEncoder(messageConverters))
            .decoder(new SpringDecoder(messageConverters))
            .retryer(new CDPRetryerConfig())
                .target(CdpDao.class, "https://httpbin.org/delay");
    }

And my config for Feign Client bean

 @Test
    void testCallTimeout(){
        long start = System.currentTimeMillis();
        Throwable throwable = catchThrowable(()-> cdpDao.getFeatures("3"));
        long end = System.currentTimeMillis();
        long start1 = System.currentTimeMillis();
        Throwable throwable1 = catchThrowable(() -> cdpDao.getFeatures("4"));
        long end1 = System.currentTimeMillis();
        log.info("elasped time {}", end-start);
        log.info("elasped second time {}", end1-start1);
        assertThat(throwable1.getCause()).isInstanceOf(InterruptedIOException.class);
    }
2024-03-15T14:47:01.410+05:30  INFO 39192 --- [           main]    : Feign retry attempt 1 due to timeout executing POST https://httpbin.org/delay/
2024-03-15T14:47:01.475+05:30  INFO 39192 --- [           main]     : Feign retry attempt 2 due to timeout executing POST https://httpbin.org/delay/
2024-03-15T14:47:01.590+05:30  INFO 39192 --- [           main]   : Feign retry attempt 3 due to timeout executing POST https://httpbin.org/delay/
2024-03-15T14:47:01.590+05:30  INFO 39192 --- [           main]   : elasped time 317
2024-03-15T14:47:01.590+05:30  INFO 39192 --- [           main]       : elasped second time 194

I have a retryer setup at 1000 ms of read or connect Timeout and my call timeout is 10 ms as per the above config. So my retryer should not have been called and call timeout should have been executed. (But I am still getting the InterruptedIoException as expected and my test case is successful).

gromspys commented 4 months ago

Everything works as expected. Retryer was executed by call timeout exception (like 500 server status code). If you don't need to retry, just use Retryer.NEVER_RETRY.

parthiv-groww commented 4 months ago
 @PostMapping(value = "/v1/readFeatureValues",
            headers = {"X-USER-ID: {userAccountId}", "Content-Type: application/json"})
    CdpResponseDto getFeatures(@RequestHeader("X-USER-ID") String userAccountId,
                               @RequestBody CdpRequestDto cdpRequestDto);

Here postmapping is not working also and I want to replace it with RequestLine which does not support using more parameters with RequestBody. Can you help on this.

gromspys commented 4 months ago

You can find how to do it in documentation https://github.com/OpenFeign/feign