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

API that allows to remove all assigned handlers from the HTTP client #5801

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Diagnostics.CodeAnalysis;
using System.Net.Http;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Diagnostics.ExceptionSummarization;
using Microsoft.Extensions.Http.Resilience;
using Microsoft.Extensions.Http.Resilience.Internal;
using Microsoft.Shared.DiagnosticIds;
using Microsoft.Shared.Diagnostics;
using Polly;
using Polly.Registry;
Expand Down Expand Up @@ -75,6 +77,28 @@ public static IHttpResiliencePipelineBuilder AddResilienceHandler(
return pipelineBuilder;
}

/// <summary>
/// Removes all resilience handlers registered earlier.
/// </summary>
/// <param name="builder">The builder instance.</param>
/// <returns>The value of <paramref name="builder"/>.</returns>
[Experimental(diagnosticId: DiagnosticIds.Experiments.Resilience, UrlFormat = DiagnosticIds.UrlFormat)]
public static IHttpClientBuilder RemoveAllResilienceHandlers(this IHttpClientBuilder builder)
{
_ = Throw.IfNull(builder);
_ = builder.ConfigureAdditionalHttpMessageHandlers(static (handlers, _) =>
{
for (int i = handlers.Count - 1; i >= 0; i--)
{
if (handlers[i] is ResilienceHandler)
{
handlers.RemoveAt(i);
}
}
evgenyfedorov2 marked this conversation as resolved.
Show resolved Hide resolved
});
return builder;
}

private static Func<HttpRequestMessage, ResiliencePipeline<HttpResponseMessage>> CreatePipelineSelector(IServiceProvider serviceProvider, string pipelineName)
{
var resilienceProvider = serviceProvider.GetRequiredService<ResiliencePipelineProvider<HttpKey>>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Collections.Generic;
using System.Linq;
using System.Net;
using System.Net.Http;
using System.Threading.Tasks;
Expand Down Expand Up @@ -286,6 +287,78 @@ public void AddResilienceHandler_AuthorityByCustomSelector_NotValidated()
Assert.NotNull(factory.CreateClient("my-client"));
}

[Fact]
evgenyfedorov2 marked this conversation as resolved.
Show resolved Hide resolved
public void RemoveAllResilienceHandlers_ArgumentValidation()
{
var services = new ServiceCollection();
IHttpClientBuilder? builder = null;
Assert.Throws<ArgumentNullException>(() => builder!.AddResilienceHandler("pipeline-name", _ => { }));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we should invoke RemoveAllResilienceHandlers here instead of AddResilienceHanlder.

Assert.Throws<ArgumentNullException>(() => builder!.AddResilienceHandler("pipeline-name", (_, _) => { }));
}

[Fact]
public void RemoveAllResilienceHandlers_EnsureHandlersRemoved()
{
var services = new ServiceCollection();

IHttpClientBuilder? builder = services.AddHttpClient("custom");

builder.AddStandardResilienceHandler();

builder.ConfigureAdditionalHttpMessageHandlers((handlers, _) =>
{
Assert.Single(handlers);
});

builder.RemoveAllResilienceHandlers();

builder.ConfigureAdditionalHttpMessageHandlers((handlers, _) =>
{
Assert.Empty(handlers);
});

using ServiceProvider serviceProvider = services.BuildServiceProvider();
services.BuildServiceProvider().GetRequiredService<IHttpClientFactory>().CreateClient("custom");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
services.BuildServiceProvider().GetRequiredService<IHttpClientFactory>().CreateClient("custom");
serviceProvider.BuildServiceProvider().GetRequiredService<IHttpClientFactory>().CreateClient("custom");

We already have the serviceProvider so we can use it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, fix similar issue in the tests below too.

}

[Fact]
public void RemoveAllResilienceHandlers_AddHandlersAfterRemoval()
{
var services = new ServiceCollection();

IHttpClientBuilder? builder = services.AddHttpClient("custom");
builder.RemoveAllResilienceHandlers().AddStandardResilienceHandler();
builder.ConfigureAdditionalHttpMessageHandlers((handlers, _) =>
{
Assert.Single(handlers);
});

using ServiceProvider serviceProvider = services.BuildServiceProvider();
services.BuildServiceProvider().GetRequiredService<IHttpClientFactory>().CreateClient("custom");
}

[Fact]
public void RemoveAllResilienceHandlers_EnsureOnlyResilienceHandlersRemoved()
{
var services = new ServiceCollection();

IHttpClientBuilder? builder = services.AddHttpClient("custom");

builder.AddHttpMessageHandler(() => new TestHandlerStub(HttpStatusCode.OK));
builder.AddStandardResilienceHandler();

builder.RemoveAllResilienceHandlers();

builder.ConfigureAdditionalHttpMessageHandlers((handlers, _) =>
{
Assert.Single(handlers);
Assert.Equal("TestHandlerStub", handlers.First().GetType().Name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we compare types here instead of type names? That would be more reliable in case somebody decides to rename the type.

});

using ServiceProvider serviceProvider = services.BuildServiceProvider();
services.BuildServiceProvider().GetRequiredService<IHttpClientFactory>().CreateClient("custom");
}

private void ConfigureBuilder(ResiliencePipelineBuilder<HttpResponseMessage> builder) => builder.AddTimeout(TimeSpan.FromSeconds(1));

private class TestMetricsEnricher : MeteringEnricher
Expand Down