dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.66k stars 4.57k forks source link

Microsoft.Extensions.Http.Resilience - Cannot access a disposed object. - Integrationtest with WebApplicationFactory #97037

Closed jnormen closed 6 months ago

jnormen commented 6 months ago

Problem with .AddStandardResilienceHandler(); when used in integration test with custom web factory and when there is more than one test.

I have a service that uses HttpClients to 3rd party endpoints. On this one, I use the.AddStandardResilienceHandler(); I set this up in my startup.cs then I have an integration test where I use the WebApplicationFactory

All works fine, but after some lib updates, I start getting random exceptions from my tests. If I run one test at a time with no problem, it seems to be shared resource issues when they run in parallel.

The tests randomly give me:

  Message: 
System.ObjectDisposedException : Cannot access a disposed object.
Object name: 'IServiceProvider'.

And:

System.InvalidCastException: Unable to cast object of type 'System.TimeSpan' to type 'System.String'.
   at __OptionValidationGeneratedAttributes.<Validators_g>F8B1F3D42962A35D8FF1B1489612AEF6C36F3713335EFF79DB68A25973333C495____SourceGen__RangeAttribute.IsValid(Object value)
   at System.ComponentModel.DataAnnotations.ValidationAttribute.IsValid(Object value, ValidationContext validationContext)
   at System.ComponentModel.DataAnnotations.ValidationAttribute.GetValidationResult(Object value, ValidationContext validationContext)
   at System.ComponentModel.DataAnnotations.Validator.TryValidate(Object value, ValidationContext validationContext, ValidationAttribute attribute, ValidationError& validationError)
   at System.ComponentModel.DataAnnotations.Validator.GetValidationErrors(Object value, ValidationContext validationContext, IEnumerable`1 attributes, Boolean breakOnFirstError)
   at System.ComponentModel.DataAnnotations.Validator.TryValidateValue(Object value, ValidationContext validationContext, ICollection`1 validationResults, IEnumerable`1 validationAttributes)
   at Microsoft.Extensions.Http.Resilience.__HttpTimeoutStrategyOptionsValidator__.

It does not happen all the time but mostly every time.

I have tried fixture, using the same client that's created on all test methods but it seems like a disposal is happening. But next time there is some expectation that the resilience services shall exist somehow. Not all services are not resettled. Some singleton was added once I guess.

 public HttpClient CreateClient(ITestOutputHelper testOutputHelper = null)
 {
     _waf = new WebApplicationFactory<TProgram>()
        .WithWebHostBuilder(builder =>
        {

If I remove the .AddStandardResilienceHandler(); from my setup of the HTTP client all works fine.

I also tried to remove the added services and override them in

 builder.ConfigureTestServices(services =>
 {

But that will not work because it's still have added the resilience stuff before I reach this area.

So it seams like: First it creates my client with resilience, then I ran the fist FACT test. Then a second one. I have 2 tests one for success and one for failure. If I remove one of the test all works fine. But as soon I share the client in many test this happens.

Not sure if there is som clean-up, or dispose issues with the resilience setup? I have followed the basic guidelines to setup integration tests in .net

Have I missed something? Or is this a known bug? I can see there is a bug reported where it seams to be a shared setting for all services atm so we can't override settings on different once. Maybe that's related here?

geeknoid commented 6 months ago

Thanks for your report. I think we might be dealing with multiple different issues, given the different symptoms.

Regarding the stack trace that ends with an InvalidCastException, that one is very puzzling. I just ran a test that exercises this code, and it never gets to a point where it is casting a TimeSpan into a string. I tried both a valid timespan and an invalid timespan to make sure I try out both success and failure validation paths. This code path should not be subject to non-deterministic behavior, either it works or it doesn't work. So I don't know what's going on here.

Are you running the .NET 8 runtime with .NET 8 versions of all extensions? The only explanation I have is that you're somehow using an older version of the RangeAttribute type which didn't support the semantics this code is depending on.

jnormen commented 6 months ago

I set it up with Refit so maybe related to that? This starts to happen after 3 months without an issue. I have updated some .net libs so not sure exactly when it started.

I use .net 8 final version. The tests run with xUnit latest version as well.

The strange thing is that it feels like something is not disposing but some parts is. Then it try to get something from the IServiceCollection that is disposed, like the standard settings the recilienece library seams to set up. The Options of default values? And in that case if IServcieCollection somehow gets disposed but some other stuffs are not it try to inject the default settings and get wrong values that are not valid?

Its so hard to debug because it happens when two test or more run in within the same class. So I can never get this problem when run each test one by one manually.

But I do not get why IServiceCollection is disposed any way, the exception start when it try to create and use the client.

If I remove the .AddStandardResilienceHandler(); from the code the error gets away. It would be easier If I could get this to happen all the time :)

I'm using System.Net.Http" Version="4.3.4" "FluentAssertions" Version="6.12.0" "FluentValidation" Version="11.9.0" "Microsoft.NET.Test.Sdk" Version="17.8.0" "Microsoft.VisualStudio.Threading.Analyzers" Version="17.8.14" "SonarAnalyzer.CSharp" Version="9.16.0.82469" "xunit" Version="2.6.5" "xunit.extensibility.core" Version="2.6.5" xunit.runner.visualstudio" Version="2.5.6 "Refit" Version="7.0.0" "Refit.HttpClientFactory" Version="7.0.0"

.net 8 With languange set to 12 in main project and my test projects

I have not test it without Refit and had no time test it more so I just added a if statement so it does not add that line for my tests :( I shall see if I have more time to do even more tests. After 1 day testing with different disposing, order of registrations, change my test to use fixture, no fixture etc without any progress I searched here and saw the issue about shared settings and thought maybe this can be related and a know issue.

geeknoid commented 6 months ago

It doesn't feel as though Refit is to blame here. There could definitely be a race here due to some global state being attached somewhere. I can see where the multiple disposal issue could surface in that context.

But the InvalidCastException appears highly mysterious. If there's any way you could catch it in the debugger when this happens and look around to see how we got there, maybe we can figure out what's happening in that particular case. And who knows, maybe that will shed some light on the multiple disposal issue as well.

jnormen commented 6 months ago

Sadly I have limited skills here in deep debugging when it comes to catching it from a random situation.

All I can get is more info about the whole error thought, not sure if it gives more info.

Message: 
System.ObjectDisposedException : Cannot access a disposed object.
Object name: 'IServiceProvider'.

  Stack Trace: 
ThrowHelper.ThrowObjectDisposedException()
ServiceProvider.GetService(ServiceIdentifier serviceIdentifier, ServiceProviderEngineScope serviceProviderEngineScope)
ServiceProvider.GetService(Type serviceType)
ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider provider, Type serviceType)
ServiceProviderServiceExtensions.GetRequiredService[T](IServiceProvider provider)
WebApplicationFactory`1.ConfigureHostBuilder(IHostBuilder hostBuilder)
WebApplicationFactory`1.EnsureServer()
WebApplicationFactory`1.CreateDefaultClient(DelegatingHandler[] handlers)
WebApplicationFactory`1.CreateDefaultClient(Uri baseAddress, DelegatingHandler[] handlers)
WebApplicationFactory`1.CreateClient(WebApplicationFactoryClientOptions options)
WebApplicationFactory`1.CreateClient()
CustomWebApplicationFactory`1.CreateClient(ITestOutputHelper testOutputHelper) line 66
AddUpdateStoreTest.StoreImportRequest_Should_Be_Valid() line 54
AddUpdateStoreTest.StoreImportRequest_Should_Be_Valid() line 61
--- End of stack trace from previous location ---

  Standard Output: 
Hosting failed to start
System.InvalidCastException: Unable to cast object of type 'System.TimeSpan' to type 'System.String'.
   at __OptionValidationGeneratedAttributes.<Validators_g>F8B1F3D42962A35D8FF1B1489612AEF6C36F3713335EFF79DB68A25973333C495____SourceGen__RangeAttribute.IsValid(Object value)
   at System.ComponentModel.DataAnnotations.ValidationAttribute.IsValid(Object value, ValidationContext validationContext)
   at System.ComponentModel.DataAnnotations.ValidationAttribute.GetValidationResult(Object value, ValidationContext validationContext)
   at System.ComponentModel.DataAnnotations.Validator.TryValidate(Object value, ValidationContext validationContext, ValidationAttribute attribute, ValidationError& validationError)
   at System.ComponentModel.DataAnnotations.Validator.GetValidationErrors(Object value, ValidationContext validationContext, IEnumerable`1 attributes, Boolean breakOnFirstError)
   at System.ComponentModel.DataAnnotations.Validator.TryValidateValue(Object value, ValidationContext validationContext, ICollection`1 validationResults, IEnumerable`1 validationAttributes)
   at Microsoft.Extensions.Http.Resilience.__HttpRetryStrategyOptionsValidator__.Validate(String name, HttpRetryStrategyOptions options)
   at Microsoft.Extensions.Http.Resilience.Internal.Validators.HttpStandardResilienceOptionsValidator.Validate(String name, HttpStandardResilienceOptions options)
   at Microsoft.Extensions.Options.OptionsFactory`1.Create(String name)
   at Microsoft.Extensions.Options.OptionsMonitor`1.<>c.<Get>b__10_0(String name, IOptionsFactory`1 factory)
   at Microsoft.Extensions.Options.OptionsCache`1.<>c__DisplayClass3_1`1.<GetOrAdd>b__2()
   at System.Lazy`1.ViaFactory(LazyThreadSafetyMode mode)
   at System.Lazy`1.ExecutionAndPublication(LazyHelper executionAndPublication, Boolean useDefaultConstructor)
   at System.Lazy`1.CreateValue()
   at Microsoft.Extensions.Options.OptionsCache`1.GetOrAdd[TArg](String name, Func`3 createOptions, TArg factoryArgument)
   at Microsoft.Extensions.Options.OptionsMonitor`1.Get(String name)
   at Microsoft.Extensions.DependencyInjection.OptionsBuilderExtensions.<>c__DisplayClass0_1`1.<ValidateOnStart>b__1()
   at Microsoft.Extensions.Options.StartupValidator.Validate()
   at Microsoft.Extensions.Hosting.Internal.Host.StartAsync(CancellationToken cancellationToken)

And this is the code that set it up in startup.cs

 builder.Services.AddRefitClient<TApiInterface>()
     .ConfigureHttpClient(c =>
     {
         c.BaseAddress = new Uri(configuration.GetValue<string>(endpointConfigKey)!);
         c.Timeout = TimeSpan.FromSeconds(35);
     })
     .AddHttpMessageHandler<AuthHeaderHandler>()
     .AddStandardResilienceHandler();

And my factory I created that I used with my Fixture setup or I have also tried to just use it in a constructor of my unit test. I also tried to call the create in each test and set the test to not run in parallel, just for testing. Same results on all 3 different ways.

internal class CustomWebApplicationFactory<TProgram> : IAsyncDisposable where TProgram : class
{
    private readonly Action<IServiceCollection> _serviceOverride;
    private WebApplicationFactory<TProgram> _waf;

    public CustomWebApplicationFactory(Action<IServiceCollection> serviceOverride = null)
    {
        _serviceOverride = serviceOverride;
    }

    public HttpClient CreateClient(ITestOutputHelper testOutputHelper = null)
    {
        _waf = new WebApplicationFactory<TProgram>()
           .WithWebHostBuilder(builder =>
           {
               builder.UseEnvironment("IntegrationTesting"); //// This will make it possible for us to have a appsettings.IntegrationTesting.Json file

               builder.ConfigureLogging(x =>
               {
                   x.ClearProviders();
                   if (testOutputHelper != null)
                       x.Services.AddSingleton<ILoggerProvider>(new XUnitLoggerProvider(testOutputHelper));
               });

               // Add mock/test services to the builder here
               builder.ConfigureTestServices(services =>
               {
                   services.AddOptions<ExternalApiConfig>().Configure(options =>
                   {
                       options.Username = "foo";
                       options.Password = "bar";
                   });

                   services.RemoveAll<IProdApi>();
                   services.RemoveAll<ITestApi>();

                   _serviceOverride?.Invoke(services);
               });
           });
        return _waf.CreateClient();
    }

    public ValueTask DisposeAsync()
    {
        return _waf.DisposeAsync();
    }
}

I tried it without the IAsyncDisposable and just IDispose as well.

I'm reusing this factory for different tests that exist in different classes. I have this action so I can easy setup mocks etc... if needed. Thats why the _serviceOverride?.Invoke(services); I have not tried without that one, but I have tried without send in any action when creating the client.

geeknoid commented 6 months ago

Would your code be available publicly somewhere? I'd love to get it and try this locally.

Thanks.

martintmk commented 6 months ago

I have a strong suspicion that this is not related to resilience code, but rather something around options or options validation.

To get resilience out of the picture try to do the following:

var builder = builder.Services.AddRefitClient<TApiInterface>()
     .ConfigureHttpClient(c =>
     {
         c.BaseAddress = new Uri(configuration.GetValue<string>(endpointConfigKey)!);
         c.Timeout = TimeSpan.FromSeconds(35);
     })
     .AddHttpMessageHandler<AuthHeaderHandler>();

builder.AddStandardResilienceHandler();

// This is the key to named options used inside AddStandardResilienceHandler
var optionsName = builder.Name + "-standard";

// Now resolve the options same way the resilience is doing it
var options = services.BuildServiceProvider()
    .GetRequiredService<IOptionsMonitor<HttpStandardResilienceOptions>>()
    .Get(optionsName);

I suspect that the call to retrieve the options will fail.

jnormen commented 6 months ago

Would your code be available publicly somewhere? I'd love to get it and try this locally.

Thanks.

Sadly I do not :/ the code is located in a private repository at a company, so it's not public :( I need to talk with them to see what I can do public or not in that case

jnormen commented 6 months ago

I have a strong suspicion that this is not related to resilience code, but rather something around options or options validation.

To get resilience out of the picture try to do the following:

var builder = builder.Services.AddRefitClient<TApiInterface>()
    .ConfigureHttpClient(c =>
    {
        c.BaseAddress = new Uri(configuration.GetValue<string>(endpointConfigKey)!);
        c.Timeout = TimeSpan.FromSeconds(35);
    })
    .AddHttpMessageHandler<AuthHeaderHandler>();

builder.AddStandardResilienceHandler();

// This is the key to named options used inside AddStandardResilienceHandler
var optionsName = builder.Name + "-standard";

// Now resolve the options same way the resilience is doing it
var options = services.BuildServiceProvider()
   .GetRequiredService<IOptionsMonitor<HttpStandardResilienceOptions>>()
   .Get(optionsName);

I suspect that the call to retrieve the options will fail.

When I added:

   // This is the key to named options used inside AddStandardResilienceHandler
       var optionsName = builder2.Name + "-standard";

       // Now resolve the options same way the resilience is doing it
       var options = builder.Services.BuildServiceProvider()
          .GetRequiredService<IOptionsMonitor<HttpStandardResilienceOptions>>()
          .Get(optionsName);

I did not get random dispiose error. But the random error now is that client could not be created.

System.InvalidOperationException : The entry point exited without ever building an IHost.

I could not find any exception to this more then it failed to create the builder.

I will test one last thing that I have not test yet, becuase of the limitation of time. We are rolling out a system atm so lots of things are in focus

jnormen commented 6 months ago

Here is what I can share

The fixture:

  public class ClientTestFixture : IDisposable
  {
      readonly CustomWebApplicationFactory<Program> factory;
      public ClientTestFixture()
      {
          factory = new CustomWebApplicationFactory<Program>();
      }

      public HttpClient Client
      {
          get { return factory.CreateClient(); }
      }

      public void Dispose()
      {
          Dispose(true);
          GC.SuppressFinalize(this);
      }

My factory code:

   internal class CustomWebApplicationFactory<TProgram> : IAsyncDisposable where TProgram : class
{
    private readonly Action<IServiceCollection> _serviceOverride;
    private WebApplicationFactory<TProgram> _waf;
    private readonly object _lock = new object();

    public CustomWebApplicationFactory(Action<IServiceCollection> serviceOverride = null)
    {
        _serviceOverride = serviceOverride;
    }
    public HttpClient CreateClient(ITestOutputHelper testOutputHelper = null)
    {
        lock (_lock)
        {
            if (_waf == null)
            {
                _waf = InitializeFactory(testOutputHelper);
            }
            return _waf.CreateClient();
        }
    }
    private WebApplicationFactory<TProgram> InitializeFactory(ITestOutputHelper testOutputHelper)
    {
        _waf = new WebApplicationFactory<TProgram>()
           .WithWebHostBuilder(builder =>
           {
               builder.UseEnvironment("ContractTesting"); //// This will make it possible for us to have a appsettings.ContractTesting.Json file

               builder.ConfigureLogging(x =>
               {
                   x.ClearProviders();
                   if (testOutputHelper != null)
                       x.Services.AddSingleton<ILoggerProvider>(new XUnitLoggerProvider(testOutputHelper));
               });

               // Add mock/test services to the builder here
               builder.ConfigureTestServices(services =>
               {
                   services.AddOptions<Enactor>().Configure(options =>
                   {
                       options.Username = "foo";
                       options.Password = "bar";
                   });

                   services.RemoveAll<IProdEnactorApi>();
                   services.RemoveAll<ITestEnactorApi>();

                   services.RemoveAll(typeof(IMediator));

                   var fakeMediator = A.Fake<IMediator>();
                   services.AddScoped((x) => { return fakeMediator; });

                   _serviceOverride?.Invoke(services);
               });
           });
        return _waf;
    }

    public ValueTask DisposeAsync()
    {
        return _waf.DisposeAsync();
    }
}

The Lock part is my last experiment, so In general I do not use that, but it does not matter, same result any way.

One of 2 test. I have two classes for 2 different feautres. Boot have 2 test with same pattern.

public class AddUpdateStoreTest : IClassFixture<ClientTestFixture>
{
    readonly HttpClient client;
    public AddUpdateStoreTest(ITestOutputHelper testOutputHelper, ClientTestFixture fixture)
    {

        client = fixture.Client;
    }
    public static StoreImportRequest GetStoreImportRequest()
    {
        return new StoreImportRequest(
            StoreNumber: 9902,
            IsActive: true,
            OpenDate: DateOnly.FromDateTime(DateTime.Now),
            Name: "Test Store",
            Country: "SE",
            IsTestStore: true,
            Email: "somesotre@somestore.com",
            PhoneNumber: "+46(0)739525320",
            ZipCode: "44 1123",
            Street: "Test Street",
            City: "Göteborg");
    }

    [Fact]
    public async Task StorePostRequest_Should_Be_Valid()
    {
        // Arrange
        var request = AddUpdateStoreTest.GetStoreImportRequest();

        client.DefaultRequestHeaders.Add("x-environment", "test");

        // Act
        using var response = await client.PostAsJsonAsync("store/location", request);

        // Assert
        response.StatusCode.Should().Be(HttpStatusCode.Accepted);
    }

    [Fact]
    public async Task StorePostRequest_Should_Not_Be_Valid()
    {
        // Arrange
        var request = AddUpdateStoreTest.GetStoreImportRequest();

        client.DefaultRequestHeaders.Add("x-environment", "test");

        // "Modify" the record by creating a new copy with a not valid Email
        StoreImportRequest storeImportRequest = request with { Name = string.Empty };

        // Act
        using var response = await client.PostAsJsonAsync("store/location", storeImportRequest);

        // Assert
        response.StatusCode.Should().Be(HttpStatusCode.BadRequest);
    }
}

I'm sorry I cannot share a project for you. If this does not help. I will see if I have time tomorrow to create a whole new project that I can share.

if not, then I turn it off, go back to old polly for a short time period, and see if this is something others will report.

martintmk commented 6 months ago

@jnormen

This is very strange error:

System.InvalidOperationException : The entry point exited without ever building an IHost.

Probably some host setup issue? I am afraid that at this point I am not able to help further, until you provide some repro code.

jnormen commented 6 months ago

@jnormen

This is very strange error:

System.InvalidOperationException : The entry point exited without ever building an IHost.

Probably some host setup issue? I am afraid that at this point I am not able to help further, until you provide some repro code.

I understand that and can see what I can do.

I saw there are some new updates to Polly, resilience lib from 8 to a 8.0.1 or something. I will test that out as soon to see if the problem persists, or might be fixed in those versions.

jjanuszkiewicz commented 6 months ago

I'm getting the Unable to cast object of type 'System.TimeSpan' to type 'System.String' error too (but not the dispose-related one).

What info would you need to diagnose this? Would a complete solution that shows the problem help?

martincostello commented 6 months ago

I've just started noticing this error locally when trying to migrate to the use of ConfigureHttpClientDefaults() from configuring things per-HttpClient. Which tests fail seems to be intermittent - I get failures when I run all the tests, but if I then re-run the individually failing tests in Visual Studio they pass.

The code I'm reproducing this with locally (but not in CI) is here: https://github.com/martincostello/alexa-london-travel/pull/1012/commits/288ef84b45ab146037688ae1144a69d55d5f5341

Wasn't there a code change in this area since 8.0.0 for an issue to do with native AoT? The stack trace is ringing a bell, but that might just be a coincidence. This is the issue I'm thinking of: https://github.com/dotnet/runtime/issues/94799, which was marked as fixed for 8.0 by https://github.com/dotnet/runtime/pull/94857.

Stack trace for a failing test:

System.InvalidCastException: Unable to cast object of type 'System.TimeSpan' to type 'System.String'.
   at __OptionValidationGeneratedAttributes.<Validators_g>F8B1F3D42962A35D8FF1B1489612AEF6C36F3713335EFF79DB68A25973333C495____SourceGen__RangeAttribute.IsValid(Object value)
   at System.ComponentModel.DataAnnotations.ValidationAttribute.IsValid(Object value, ValidationContext validationContext)
   at System.ComponentModel.DataAnnotations.ValidationAttribute.GetValidationResult(Object value, ValidationContext validationContext)
   at System.ComponentModel.DataAnnotations.Validator.TryValidate(Object value, ValidationContext validationContext, ValidationAttribute attribute, ValidationError& validationError)
   at System.ComponentModel.DataAnnotations.Validator.GetValidationErrors(Object value, ValidationContext validationContext, IEnumerable`1 attributes, Boolean breakOnFirstError)
   at System.ComponentModel.DataAnnotations.Validator.TryValidateValue(Object value, ValidationContext validationContext, ICollection`1 validationResults, IEnumerable`1 validationAttributes)
   at Microsoft.Extensions.Http.Resilience.__HttpTimeoutStrategyOptionsValidator__.Validate(String name, HttpTimeoutStrategyOptions options)
   at Microsoft.Extensions.Http.Resilience.Internal.Validators.HttpStandardResilienceOptionsValidator.Validate(String name, HttpStandardResilienceOptions options)
   at Microsoft.Extensions.Options.OptionsFactory`1.Create(String name)
   at System.Lazy`1.ViaFactory(LazyThreadSafetyMode mode)
   at System.Lazy`1.ExecutionAndPublication(LazyHelper executionAndPublication, Boolean useDefaultConstructor)
   at System.Lazy`1.CreateValue()
   at Microsoft.Extensions.Options.OptionsCache`1.GetOrAdd[TArg](String name, Func`3 createOptions, TArg factoryArgument)
   at Microsoft.Extensions.Options.OptionsMonitor`1.Get(String name)
   at Microsoft.Extensions.DependencyInjection.ResilienceHttpClientBuilderExtensions.<>c__DisplayClass15_0.<AddStandardResilienceHandler>b__0(ResiliencePipelineBuilder`1 builder, ResilienceHandlerContext context)
   at Microsoft.Extensions.DependencyInjection.ResilienceHttpClientBuilderExtensions.<>c__DisplayClass8_0.<AddHttpResiliencePipeline>b__0(ResiliencePipelineBuilder`1 builder, AddResiliencePipelineContext`1 context)
   at Polly.PollyServiceCollectionExtensions.<>c__DisplayClass1_1`2.<AddResiliencePipeline>b__2(ResiliencePipelineBuilder`1 builder, ConfigureBuilderContext`1 context)
   at Polly.Registry.RegistryPipelineComponentBuilder`2.CreateBuilder()
   at Polly.Registry.RegistryPipelineComponentBuilder`2.CreateComponent()
   at Polly.Registry.ResiliencePipelineRegistry`1.GenericRegistry`1.<>c__DisplayClass7_0.<GetOrAdd>b__0(TKey k)
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
   at Polly.Registry.ResiliencePipelineRegistry`1.GenericRegistry`1.GetOrAdd(TKey key, Action`2 configure)
   at Polly.Registry.ResiliencePipelineRegistry`1.GenericRegistry`1.TryGet(TKey key, ResiliencePipeline`1& strategy)
   at Polly.Registry.ResiliencePipelineRegistry`1.TryGetPipeline[TResult](TKey key, ResiliencePipeline`1& pipeline)
   at Polly.Registry.ResiliencePipelineProvider`1.GetPipeline[TResult](TKey key)
   at Microsoft.Extensions.DependencyInjection.ResilienceHttpClientBuilderExtensions.CreatePipelineSelector(IServiceProvider serviceProvider, String pipelineName)
   at Microsoft.Extensions.DependencyInjection.ResilienceHttpClientBuilderExtensions.<>c__DisplayClass5_0.<AddResilienceHandler>b__0(IServiceProvider serviceProvider)
   at Microsoft.Extensions.DependencyInjection.HttpClientBuilderExtensions.<>c__DisplayClass3_0.<AddHttpMessageHandler>b__1(HttpMessageHandlerBuilder b)
   at Microsoft.Extensions.Http.DefaultHttpClientFactory.<>c__DisplayClass17_0.<CreateHandlerEntry>g__Configure|0(HttpMessageHandlerBuilder b)
   at Microsoft.Extensions.Http.MetricsFactoryHttpMessageHandlerFilter.<>c__DisplayClass2_0.<Configure>b__0(HttpMessageHandlerBuilder builder)
   at Microsoft.Extensions.Http.LoggingHttpMessageHandlerBuilderFilter.<>c__DisplayClass6_0.<Configure>b__0(HttpMessageHandlerBuilder builder)
   at MartinCostello.LondonTravel.Skill.HttpRequestInterceptionFilter.<>c__DisplayClass2_0.<Configure>b__0(HttpMessageHandlerBuilder builder) in C:\Coding\martincostello\alexa-london-travel\test\LondonTravel.Skill.Tests\HttpRequestInterceptionFilter.cs:line 30
   at Microsoft.Extensions.Http.DefaultHttpClientFactory.CreateHandlerEntry(String name)
   at System.Lazy`1.ViaFactory(LazyThreadSafetyMode mode)
   at System.Lazy`1.ExecutionAndPublication(LazyHelper executionAndPublication, Boolean useDefaultConstructor)
   at System.Lazy`1.CreateValue()
   at Microsoft.Extensions.Http.DefaultHttpClientFactory.CreateHandler(String name)
   at Microsoft.Extensions.Http.DefaultHttpClientFactory.CreateClient(String name)
   at Microsoft.Extensions.DependencyInjection.HttpClientBuilderExtensions.AddTransientHelper[TClient](IServiceProvider s, IHttpClientBuilder builder)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSiteMain(ServiceCallSite callSite, TArgument argument)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitDisposeCache(ServiceCallSite transientCallSite, RuntimeResolverContext context)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSite(ServiceCallSite callSite, TArgument argument)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitConstructor(ConstructorCallSite constructorCallSite, RuntimeResolverContext context)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSiteMain(ServiceCallSite callSite, TArgument argument)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitDisposeCache(ServiceCallSite transientCallSite, RuntimeResolverContext context)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSite(ServiceCallSite callSite, TArgument argument)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.Resolve(ServiceCallSite callSite, ServiceProviderEngineScope scope)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.DynamicServiceProviderEngine.<>c__DisplayClass2_0.<RealizeService>b__0(ServiceProviderEngineScope scope)
   at Microsoft.Extensions.DependencyInjection.ServiceProvider.GetService(ServiceIdentifier serviceIdentifier, ServiceProviderEngineScope serviceProviderEngineScope)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngineScope.GetService(Type serviceType)
   at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider provider, Type serviceType)
   at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService[T](IServiceProvider provider)
   at MartinCostello.LondonTravel.Skill.IntentFactory.Create(Intent intent) in C:\Coding\martincostello\alexa-london-travel\src\LondonTravel.Skill\IntentFactory.cs:line 33
   at MartinCostello.LondonTravel.Skill.AlexaSkill.OnIntentAsync(IIntentRequest intent, Session session) in C:\Coding\martincostello\alexa-london-travel\src\LondonTravel.Skill\AlexaSkill.cs:line 71
   at MartinCostello.LondonTravel.Skill.FunctionHandler.HandleRequestAsync(SkillRequest request) in C:\Coding\martincostello\alexa-london-travel\src\LondonTravel.Skill\FunctionHandler.cs:line 58
martincostello commented 6 months ago

The decompiled code for that method from Microsoft.Extensions.Http.Resilience 8.1.0 is this according to ILSpy is below. Looks like it contains two casts to a string.

____SourceGen__RangeAttribute.IsValid() ```csharp public override bool IsValid(object? value) { if (!Initialized) { if (Minimum == null || Maximum == null) { throw new InvalidOperationException("The minimum and maximum values must be set to valid values."); } if (NeedToConvertMinMax) { CultureInfo formatProvider = (ParseLimitsInInvariantCulture ? CultureInfo.InvariantCulture : CultureInfo.CurrentCulture); if (OperandType == typeof(TimeSpan)) { if (!TimeSpan.TryParse((string)Minimum, formatProvider, out var result) || !TimeSpan.TryParse((string)Maximum, formatProvider, out var result2)) { throw new InvalidOperationException("The minimum and maximum values must be set to valid values."); } Minimum = result; Maximum = result2; } else { Minimum = ConvertValue(Minimum, formatProvider) ?? throw new InvalidOperationException("The minimum and maximum values must be set to valid values."); Maximum = ConvertValue(Maximum, formatProvider) ?? throw new InvalidOperationException("The minimum and maximum values must be set to valid values."); } } int num = ((IComparable)Minimum).CompareTo((IComparable)Maximum); if (num > 0) { throw new InvalidOperationException("The maximum value '{Maximum}' must be greater than or equal to the minimum value '{Minimum}'."); } if (num == 0 && (MinimumIsExclusive || MaximumIsExclusive)) { throw new InvalidOperationException("Cannot use exclusive bounds when the maximum value is equal to the minimum value."); } Initialized = true; } if ((value == null || (value is string text && text.Length == 0)) ? true : false) { return true; } CultureInfo formatProvider2 = (ConvertValueInInvariantCulture ? CultureInfo.InvariantCulture : CultureInfo.CurrentCulture); object obj; if (OperandType == typeof(TimeSpan)) { if (value is TimeSpan) { obj = value; } else { if (!(value is string)) { DefaultInterpolatedStringHandler defaultInterpolatedStringHandler = new DefaultInterpolatedStringHandler(121, 1); defaultInterpolatedStringHandler.AppendLiteral("A value type "); defaultInterpolatedStringHandler.AppendFormatted(value!.GetType()); defaultInterpolatedStringHandler.AppendLiteral(" that is not a TimeSpan or a string has been given. This might indicate a problem with the source generator."); throw new InvalidOperationException(defaultInterpolatedStringHandler.ToStringAndClear()); } if (!TimeSpan.TryParse((string)value, formatProvider2, out var result3)) { return false; } obj = result3; } } else { try { obj = ConvertValue(value, formatProvider2); } catch (Exception ex) when (((ex is FormatException || ex is InvalidCastException || ex is NotSupportedException) ? 1 : 0) != 0) { return false; } } IComparable comparable = (IComparable)Minimum; IComparable comparable2 = (IComparable)Maximum; if (MinimumIsExclusive ? (comparable.CompareTo(obj) < 0) : (comparable.CompareTo(obj) <= 0)) { if (!MaximumIsExclusive) { return comparable2.CompareTo(obj) >= 0; } return comparable2.CompareTo(obj) > 0; } return false; } ```
martincostello commented 6 months ago

Looking at the code, there might be a race condition where the casts (string)Minimum or/and (string)Maximum fail if those values are already initialized with a TimeSpan value if Initialized hasn't been set to true yet if the validator runs concurrently. Source

martincostello commented 6 months ago

One final data point, if I change my xunit configuration to have "parallelizeTestCollections": false then the issue goes away when I run all the tests in the same test run.

jjanuszkiewicz commented 6 months ago

parallelizeTestCollections helps here too.

martincostello commented 6 months ago

This is where the exception comes from in my repro:

image

If I go up the stack enough, the value of value is a TimeSpan whose value is equal to 30 seconds:

image

Here's the state of the attribute, you can see that the Maximum and Minimum are assigned a TimeSpan value already and that Initialized is true.

image

geeknoid commented 6 months ago

@tarekgh I think the InvalidCastException part of this problem has to do with the config validation code gen and how we're handling some racy initialization in the attribute. Can you take a look please?

geeknoid commented 6 months ago

OK, so the problem is this:

ghost commented 6 months ago

Tagging subscribers to this area: @dotnet/area-system-componentmodel-dataannotations See info in area-owners.md if you want to be subscribed.

Issue Details
Problem with .AddStandardResilienceHandler(); when used in integration test with custom web factory and when there is more than one test. I have a service that uses HttpClients to 3rd party endpoints. On this one, I use the.AddStandardResilienceHandler(); I set this up in my startup.cs then I have an integration test where I use the WebApplicationFactory All works fine, but after some lib updates, I start getting random exceptions from my tests. If I run one test at a time with no problem, it seems to be shared resource issues when they run in parallel. The tests randomly give me: ``` Message:  System.ObjectDisposedException : Cannot access a disposed object. Object name: 'IServiceProvider'. ``` And: ``` System.InvalidCastException: Unable to cast object of type 'System.TimeSpan' to type 'System.String'. at __OptionValidationGeneratedAttributes.F8B1F3D42962A35D8FF1B1489612AEF6C36F3713335EFF79DB68A25973333C495____SourceGen__RangeAttribute.IsValid(Object value) at System.ComponentModel.DataAnnotations.ValidationAttribute.IsValid(Object value, ValidationContext validationContext) at System.ComponentModel.DataAnnotations.ValidationAttribute.GetValidationResult(Object value, ValidationContext validationContext) at System.ComponentModel.DataAnnotations.Validator.TryValidate(Object value, ValidationContext validationContext, ValidationAttribute attribute, ValidationError& validationError) at System.ComponentModel.DataAnnotations.Validator.GetValidationErrors(Object value, ValidationContext validationContext, IEnumerable`1 attributes, Boolean breakOnFirstError) at System.ComponentModel.DataAnnotations.Validator.TryValidateValue(Object value, ValidationContext validationContext, ICollection`1 validationResults, IEnumerable`1 validationAttributes) at Microsoft.Extensions.Http.Resilience.__HttpTimeoutStrategyOptionsValidator__. ``` It does not happen all the time but mostly every time. I have tried fixture, using the same client that's created on all test methods but it seems like a disposal is happening. But next time there is some expectation that the resilience services shall exist somehow. Not all services are not resettled. Some singleton was added once I guess. ``` public HttpClient CreateClient(ITestOutputHelper testOutputHelper = null) { _waf = new WebApplicationFactory() .WithWebHostBuilder(builder => { ``` If I remove the .AddStandardResilienceHandler(); from my setup of the HTTP client all works fine. I also tried to remove the added services and override them in ``` builder.ConfigureTestServices(services => { ``` But that will not work because it's still have added the resilience stuff before I reach this area. So it seams like: First it creates my client with resilience, then I ran the fist FACT test. Then a second one. I have 2 tests one for success and one for failure. If I remove one of the test all works fine. But as soon I share the client in many test this happens. Not sure if there is som clean-up, or dispose issues with the resilience setup? I have followed the basic guidelines to setup integration tests in .net Have I missed something? Or is this a known bug? I can see there is a bug reported where it seams to be a shared setting for all services atm so we can't override settings on different once. Maybe that's related here?
Author: jnormen
Assignees: -
Labels: `bug`, `area-System.ComponentModel.DataAnnotations`, `untriaged`
Milestone: -
tarekgh commented 6 months ago

I moved the issue to the runtime repo. I'll take a look.

tarekgh commented 6 months ago

@martincostello is it possible you can share the source generated file from your repro?

martincostello commented 6 months ago

I'm not at a computer until tomorrow now, but I had to use ILSpy to get the code because it was compiled into the Resilience library, rather than being generated into my own project.

ghost commented 6 months ago

Tagging subscribers to this area: @dotnet/area-extensions-options See info in area-owners.md if you want to be subscribed.

Issue Details
Problem with .AddStandardResilienceHandler(); when used in integration test with custom web factory and when there is more than one test. I have a service that uses HttpClients to 3rd party endpoints. On this one, I use the.AddStandardResilienceHandler(); I set this up in my startup.cs then I have an integration test where I use the WebApplicationFactory All works fine, but after some lib updates, I start getting random exceptions from my tests. If I run one test at a time with no problem, it seems to be shared resource issues when they run in parallel. The tests randomly give me: ``` Message:  System.ObjectDisposedException : Cannot access a disposed object. Object name: 'IServiceProvider'. ``` And: ``` System.InvalidCastException: Unable to cast object of type 'System.TimeSpan' to type 'System.String'. at __OptionValidationGeneratedAttributes.F8B1F3D42962A35D8FF1B1489612AEF6C36F3713335EFF79DB68A25973333C495____SourceGen__RangeAttribute.IsValid(Object value) at System.ComponentModel.DataAnnotations.ValidationAttribute.IsValid(Object value, ValidationContext validationContext) at System.ComponentModel.DataAnnotations.ValidationAttribute.GetValidationResult(Object value, ValidationContext validationContext) at System.ComponentModel.DataAnnotations.Validator.TryValidate(Object value, ValidationContext validationContext, ValidationAttribute attribute, ValidationError& validationError) at System.ComponentModel.DataAnnotations.Validator.GetValidationErrors(Object value, ValidationContext validationContext, IEnumerable`1 attributes, Boolean breakOnFirstError) at System.ComponentModel.DataAnnotations.Validator.TryValidateValue(Object value, ValidationContext validationContext, ICollection`1 validationResults, IEnumerable`1 validationAttributes) at Microsoft.Extensions.Http.Resilience.__HttpTimeoutStrategyOptionsValidator__. ``` It does not happen all the time but mostly every time. I have tried fixture, using the same client that's created on all test methods but it seems like a disposal is happening. But next time there is some expectation that the resilience services shall exist somehow. Not all services are not resettled. Some singleton was added once I guess. ``` public HttpClient CreateClient(ITestOutputHelper testOutputHelper = null) { _waf = new WebApplicationFactory() .WithWebHostBuilder(builder => { ``` If I remove the .AddStandardResilienceHandler(); from my setup of the HTTP client all works fine. I also tried to remove the added services and override them in ``` builder.ConfigureTestServices(services => { ``` But that will not work because it's still have added the resilience stuff before I reach this area. So it seams like: First it creates my client with resilience, then I ran the fist FACT test. Then a second one. I have 2 tests one for success and one for failure. If I remove one of the test all works fine. But as soon I share the client in many test this happens. Not sure if there is som clean-up, or dispose issues with the resilience setup? I have followed the basic guidelines to setup integration tests in .net Have I missed something? Or is this a known bug? I can see there is a bug reported where it seams to be a shared setting for all services atm so we can't override settings on different once. Maybe that's related here?
Author: jnormen
Assignees: -
Labels: `bug`, `untriaged`, `area-Extensions-Options`, `source-generator`
Milestone: -
tarekgh commented 6 months ago

I'm not at a computer until tomorrow now, but I had to use ILSpy to get the code because it was compiled into the Resilience library, rather than being generated into my own project.

never mind. I have created an isolated simple repro and I can see the issue now.

dominikjeske commented 6 months ago

@tarekgh PR is in mailstone 9 - can we hope to get in in 8.0.2?

tarekgh commented 6 months ago

PR is in mailstone 9 - can we hope to get in in 8.0.2?

I intend to port the fix to version 8.0 after merging it into the main branch. This will likely be included in 8.0.3, as the cutoff for 8.0.2 has already passed.

tarekgh commented 6 months ago

I have submitted the fix to main and 8.0 servicing branches. Hopefully, this will be included in the February 8.0 servicing release.