[Bug]: InvalidCastException when multiple threads retrieve pipeline

[kmcclellan]

Describe the bug

System.ComponentModel.DataAnnotations.RangeAttribute is not thread safe.

ValidationHelper does not synchronize access to Validator and is invoked when initializing pipelines. This can lead to occasional InvalidCastException when multiple threads retrieve a pipeline for the first time (while RangeAttribute is uninitialized).

This was previously reported as #2096 but not fixed.

Expected behavior

No exception is thrown.

Actual behavior

No response

Steps to reproduce

It's pretty difficult to reproduce in the registry, due to the specificity of the race condition and the fact that you only get one chance per process to trigger it.

You can simulate it reliably using just the attribute, however.

using System;
using System.ComponentModel.DataAnnotations;
using System.Threading;
using System.Threading.Tasks;

var exceptions = 0;
var parallelism = 2;
const int repeat = 100_000;

for (var i = 0; i < repeat; i++)
{
    var range = new RangeAttribute(typeof(TimeSpan), "00:00:00", "1:00:00");

    Parallel.For(
        0,
        parallelism,
        new ParallelOptions { MaxDegreeOfParallelism = parallelism },
        index =>
        {
            try
            {
                range.IsValid(null);
            }
            catch (InvalidCastException)
            {
                Interlocked.Increment(ref exceptions);
            }
        });
}

var frequency = exceptions / (double)repeat;
Console.WriteLine($"{exceptions} exceptions encountered ({frequency:P4})");

Example: "25 exceptions encountered (0.0250%)"

Exception(s) (if any)

System.InvalidCastException: Unable to cast object of type 'System.TimeSpan' to type 'System.String'.
   at System.ComponentModel.DataAnnotations.RangeAttribute.SetupConversion()
   at System.ComponentModel.DataAnnotations.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.GetObjectPropertyValidationErrors(Object instance, ValidationContext validationContext, Boolean validateAllProperties, Boolean breakOnFirstError)
   at System.ComponentModel.DataAnnotations.Validator.GetObjectValidationErrors(Object instance, ValidationContext validationContext, Boolean validateAllProperties, Boolean breakOnFirstError)
   at System.ComponentModel.DataAnnotations.Validator.TryValidateObject(Object instance, ValidationContext validationContext, ICollection`1 validationResults, Boolean validateAllProperties)
   at Polly.Utils.ValidationHelper.ValidateObject(ResilienceValidationContext context)
   at Polly.ResiliencePipelineBuilderBase.AddPipelineComponent(Func`2 factory, ResilienceStrategyOptions options)
   at Polly.ResiliencePipelineBuilderExtensions.AddStrategy[TResult](ResiliencePipelineBuilder`1 builder, Func`2 factory, ResilienceStrategyOptions options)
   at Polly.RetryResiliencePipelineBuilderExtensions.AddRetry[TResult](ResiliencePipelineBuilder`1 builder, RetryStrategyOptions`1 options)
   at Firebird.Saga.Client.ServiceCollectionExtensions.<>c__DisplayClass2_0.<AddSagaCommands>b__6(ResiliencePipelineBuilder`1 x, ConfigureBuilderContext`1 y)
   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)

Polly version

8.5.0

.NET Version

net9.0

Anything else?

No response

[martincostello]

If this isn't exactly the same issue as I referenced in the linked discussion, then it's a new different issue that needs to be opened against dotnet/runtime.

If you can repro it without Polly even being involved, then it's not an issue in our code, and not something we can reasonably work around either.

[kmcclellan]

If you read the linked issue, you'll see that the runtime maintainers are choosing not to implement thread safety, which makes it the burden on this library not to invoke Validator from multiple threads.

[martincostello]

The issue I linked from the discussion was to do with thread safety and validation attributes, and was marked as resolved by the poster, hence I assumed that it was either the same, or if it wasn't that it would still be their bug to fix if not (the issue I linked to was fixed).

If that's not the case and it's something we need to do something about, then we'll happily accept a pull request if you'd like to address it if you'd like to expedite a fix.

[kmcclellan]

The issue I linked from the discussion was to do with thread safety and validation attributes, and was marked as resolved by the poster

The fix you are referring to is for .NET source generated options validation (using OptionsValidatorAttribute). Polly's Core library does not reference Microsoft.Extensions.Options and instead leverages Validator to perform options validation. Per dotnet/runtime#110917, .NET team now seems amenable to fixing this in the runtime, but I have opened a pull request in the hope this can be addressed in the meantime.