Polly
Polly copied to clipboard
App-vNext
[Bug]: AddResiliencePipelineBuilder options.LoggerFactory assignment issue
Describe the bug
This bug is created by a very unusual edge case. I'm using Polly in a custom logger provider implementation. This causes an cycle since my implementation is resolving ILoggerFactory and this code is doing the same. As such I need to nullify the assignment of options.LoggerFactory. It appears that I can assign NullLoggerFactory.Instance to this property during my configuration stage. However, that assignment is overwritten by the assignment below. Maybe you can check for whether the property is not null before making the assignment.
Problematic source code:
\src\Polly.Extensions\DependencyInjection\PollyServiceCollectionExtensions.cs
private static void AddResiliencePipelineBuilder(this IServiceCollection services)
{
services
.AddOptions<TelemetryOptions>()
.Configure<IServiceProvider>((options, serviceProvider) =>
{
options.LoggerFactory = serviceProvider.GetService<ILoggerFactory>() ?? NullLoggerFactory.Instance;
});
services.TryAddTransient(serviceProvider =>
{
var builder = new ResiliencePipelineBuilder();
builder.ConfigureTelemetry(serviceProvider.GetRequiredService<IOptions<TelemetryOptions>>().Value);
return builder;
});
}
My configuration:
builder.Services.AddResiliencePipeline(CircularFileLoggerProvider.FileMoveUnauthorizedAccessExceptionName, builder => {
builder.AddRetry(new RetryStrategyOptions()
{
ShouldHandle = new PredicateBuilder().Handle<UnauthorizedAccessException>(),
MaxRetryAttempts = 3,
}).ConfigureTelemetry(new TelemetryOptions() { LoggerFactory = NullLoggerFactory.Instance });
});
Recommended change:
.Configure<IServiceProvider>((options, serviceProvider) =>
{
options.LoggerFactory ??= serviceProvider.GetService<ILoggerFactory>() ?? NullLoggerFactory.Instance;
});
Expected behavior
No response
Actual behavior
No response
Steps to reproduce
No response
Exception(s) (if any)
No response
Polly version
No response
.NET Version
No response
Anything else?
No response
Sounds reasonable to me that we shouldn't overwrite it, assuming it's always null otherwise on that code path.
Would you like to submit a PR to make the change?
It is too much work to create a PR. Can you just add it to your next PR? I'm sorry if I sound lazy.
If you don't want to do it that's fine, it'll just be longer until it gets fixed.
Given you've already identified what needs changing and have a snippet of code that could be used to test the change, I wouldn't have thought it would take more than 15 minutes if someone else wants to pick it up in the meantime.
@Ephron-WL
I have tried to replicate this issue in #1787 and it seems to work fine. Can you take a look at it and see what scenario is it not covering?
