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

Polly infinity retry in HttpClientProxies #2009

Closed
chai93323 opened this issue Oct 25, 2019 · 11 comments · Fixed by #2038 or #2201
Closed

Polly infinity retry in HttpClientProxies #2009

chai93323 opened this issue Oct 25, 2019 · 11 comments · Fixed by #2038 or #2201
Assignees
Labels
Milestone

Comments

@chai93323
Copy link

Currently when the service throw exception, Abp HttpClientProxies will infinity retry with polly and throw the exception below. How can I fix this issue or include new policy to disable polly without modify abp http client library?

TaskCanceledException: A task was canceled.
Polly.Retry.AsyncRetryEngine.ImplementationAsync(Func<Context, CancellationToken, Task> action, Context context, CancellationToken cancellationToken, ExceptionPredicates shouldRetryExceptionPredicates, ResultPredicates shouldRetryResultPredicates, Func<DelegateResult, TimeSpan, int, Context, Task> onRetryAsync, int permittedRetryCount, IEnumerable sleepDurationsEnumerable, Func<int, DelegateResult, Context, TimeSpan> sleepDurationProvider, bool continueOnCapturedContext)

@maliming
Copy link
Member

I will check it out.

@maliming
Copy link
Member

hi @chai93323 Can you explain it in detail? And the steps to reproduce.

@chai93323
Copy link
Author

chai93323 commented Oct 29, 2019

hi @maliming , you can just include throw new Exception("Polly Test"); on any service method. The exception will fail retry on method below then throw the polly retry exception.

 //use IHttpClientFactory and polly
            services.AddHttpClient(remoteServiceConfigurationName)
                .AddTransientHttpErrorPolicy(builder =>
                    // retry 3 times
                    builder.WaitAndRetryAsync(3, i => TimeSpan.FromSeconds(Math.Pow(2, i))));

@maliming
Copy link
Member

maliming commented Oct 29, 2019

@chai93323
When I test, the http request will be retried 3 times as expected.

image

@chai93323
Copy link
Author

@maliming how can we include own polly policy? Current policy cause issue on our environment.

@maliming
Copy link
Member

It is not currently customizable, but we can consider adding an option to configure it. I will try to implement it.

@techmouse84
Copy link

Pls reopen this. I'm facing this issue as well.
I'm able to reproduce this issue in version 1.0.2
It's not infinitely retry but definitely alot more than just 3 retries (default)
Steps to reproduce.
using the default microservice demo

  1. Deliberately throw exception in ProductAppService
public async Task<ListResultDto<ProductDto>> GetListAsync() 
        {
            throw new AbpException("Test");
        }
  1. Update ConsoleClientDemo appSettings to point to the microservice
  2. Update ClientDemoService to run only TestProduceService

Below is my fiddler output.
image

I believe the problem is in ServiceCollectionDynamicHttpClientProxyExtensions
The polly retry is registered multiple times due to the foreach loop within the AddHttpClientProxy method.

    foreach (var serviceType in serviceTypes)
            {
                services.AddHttpClientProxy(
                    serviceType, 
                    remoteServiceConfigurationName,
                    asDefaultServices
                    );
            }

I moved the Polly config out of the for loop and it seems to fix the problem.

 //use IHttpClientFactory and polly
            services.AddHttpClient(remoteServiceConfigurationName)
                .AddTransientHttpErrorPolicy(builder =>
                    // retry 3 times
                    builder.WaitAndRetryAsync(3, i => TimeSpan.FromSeconds(Math.Pow(2, i))));

            foreach (var serviceType in serviceTypes)
            {
                services.AddHttpClientProxy(
                    serviceType, 
                    remoteServiceConfigurationName,
                    asDefaultServices
                    );
            }

(1 call + 3 retries )
image

@maliming
Copy link
Member

@techmouse84 This pr should solve your problem. #2038

@techmouse84
Copy link

@maliming ,
thanks for the speedy reply.
I have tried this PR . However, this does not address my problem. I understand that this PR basically expose the configuration of polly policy. I can remove the configuration of polly in the following manner but this is not what i'm trying to resolve. The following code removes polly entirely.

public class ProductManagementHttpApiClientModule : AbpModule
    {
        public const string RemoteServiceName = "ProductManagement";

        public override void ConfigureServices(ServiceConfigurationContext context)
        {
            context.Services.AddHttpClientProxies(
                typeof(ProductManagementApplicationContractsModule).Assembly,
                RemoteServiceName, configureHttpClientBuilder:
                (httpClientBuilder) => {
                }
            );
        }
    }

However, if i use the default setup, i still run into many retries instead of the expected number 3.

public class ProductManagementHttpApiClientModule : AbpModule
    {
        public const string RemoteServiceName = "ProductManagement";

        public override void ConfigureServices(ServiceConfigurationContext context)
        {
            context.Services.AddHttpClientProxies(
                typeof(ProductManagementApplicationContractsModule).Assembly,
                RemoteServiceName, configureHttpClientBuilder: null
            );
        }
    }

image

Maybe you can take a look at my previous post in details?
I mentioned on how i tested and proved that the problem is due to the registration of the polly and httpClient.

@maliming
Copy link
Member

maliming commented Nov 18, 2019

@techmouse84 I tested it in a project created by cli. Not tried microservices.
I will continue to check it, thanks.

@maliming
Copy link
Member

@techmouse84 see #2201

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment