InvalidOperationException thrown when binding operation underway during configuration reload

[martincostello]

Describe the bug

If IConfigurationRoot.Reload() is called and the application is serving requests that require an options object to be bound from configuration on other threads, sometimes an InvalidOperationException is thrown causing HTTP 500 responses while the typed config is being bound. An example stack trace is below.

Stack trace

System.InvalidOperationException: Collection was modified; enumeration operation may not execute.
   at System.Collections.Generic.Dictionary`2.Enumerator.MoveNext()
   at System.Linq.Enumerable.WhereSelectEnumerableIterator`2.MoveNext()
   at System.Collections.Generic.LargeArrayBuilder`1.AddRange(IEnumerable`1 items)
   at System.Collections.Generic.SparseArrayBuilder`1.ReserveOrAdd(IEnumerable`1 items)
   at System.Linq.Enumerable.Concat2Iterator`1.ToArray()
   at System.Linq.Buffer`1..ctor(IEnumerable`1 source)
   at System.Linq.OrderedEnumerable`1.GetEnumerator()+MoveNext()
   at System.Collections.Generic.LargeArrayBuilder`1.AddRange(IEnumerable`1 items)
   at System.Collections.Generic.SparseArrayBuilder`1.ReserveOrAdd(IEnumerable`1 items)
   at System.Linq.Enumerable.Concat2Iterator`1.ToArray()
   at System.Linq.Buffer`1..ctor(IEnumerable`1 source)
   at System.Linq.OrderedEnumerable`1.GetEnumerator()+MoveNext()
   at System.Collections.Generic.LargeArrayBuilder`1.AddRange(IEnumerable`1 items)
   at System.Collections.Generic.SparseArrayBuilder`1.ReserveOrAdd(IEnumerable`1 items)
   at System.Linq.Enumerable.Concat2Iterator`1.ToArray()
   at System.Linq.Buffer`1..ctor(IEnumerable`1 source)
   at System.Linq.OrderedEnumerable`1.GetEnumerator()+MoveNext()
   at System.Collections.Generic.LargeArrayBuilder`1.AddRange(IEnumerable`1 items)
   at System.Collections.Generic.SparseArrayBuilder`1.ReserveOrAdd(IEnumerable`1 items)
   at System.Linq.Enumerable.Concat2Iterator`1.ToArray()
   at System.Linq.Buffer`1..ctor(IEnumerable`1 source)
   at System.Linq.OrderedEnumerable`1.GetEnumerator()+MoveNext()
   at System.Collections.Generic.LargeArrayBuilder`1.AddRange(IEnumerable`1 items)
   at System.Collections.Generic.SparseArrayBuilder`1.ReserveOrAdd(IEnumerable`1 items)
   at System.Linq.Enumerable.Concat2Iterator`1.ToArray()
   at System.Linq.Buffer`1..ctor(IEnumerable`1 source)
   at System.Linq.OrderedEnumerable`1.GetEnumerator()+MoveNext()
   at System.Linq.Enumerable.DistinctIterator`1.MoveNext()
   at System.Linq.Enumerable.SelectEnumerableIterator`2.MoveNext()
   at System.Linq.Enumerable.Any[TSource](IEnumerable`1 source)
   at Microsoft.Extensions.Configuration.ConfigurationBinder.BindInstance(Type type, Object instance, IConfiguration config, BinderOptions options)
   at Microsoft.Extensions.Configuration.ConfigurationBinder.BindProperty(PropertyInfo property, Object instance, IConfiguration config, BinderOptions options)
   at Microsoft.Extensions.Configuration.ConfigurationBinder.BindNonScalar(IConfiguration configuration, Object instance, BinderOptions options)
   at Microsoft.Extensions.Configuration.ConfigurationBinder.BindInstance(Type type, Object instance, IConfiguration config, BinderOptions options)
   at Microsoft.Extensions.Configuration.ConfigurationBinder.BindDictionary(Object dictionary, Type dictionaryType, IConfiguration config, BinderOptions options)
   at Microsoft.Extensions.Configuration.ConfigurationBinder.AttemptBindToCollectionInterfaces(Type type, IConfiguration config, BinderOptions options)
   at Microsoft.Extensions.Configuration.ConfigurationBinder.BindInstance(Type type, Object instance, IConfiguration config, BinderOptions options)
   at Microsoft.Extensions.Configuration.ConfigurationBinder.BindProperty(PropertyInfo property, Object instance, IConfiguration config, BinderOptions options)
   at Microsoft.Extensions.Configuration.ConfigurationBinder.BindNonScalar(IConfiguration configuration, Object instance, BinderOptions options)
   at Microsoft.Extensions.Configuration.ConfigurationBinder.BindInstance(Type type, Object instance, IConfiguration config, BinderOptions options)
   at Microsoft.Extensions.Configuration.ConfigurationBinder.BindProperty(PropertyInfo property, Object instance, IConfiguration config, BinderOptions options)
   at Microsoft.Extensions.Configuration.ConfigurationBinder.BindNonScalar(IConfiguration configuration, Object instance, BinderOptions options)
   at Microsoft.Extensions.Configuration.ConfigurationBinder.BindInstance(Type type, Object instance, IConfiguration config, BinderOptions options)
   at Microsoft.Extensions.Configuration.ConfigurationBinder.Bind(IConfiguration configuration, Object instance, Action`1 configureOptions)
   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 lambda_method(Closure , ServiceProviderEngineScope )
   at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider provider, Type serviceType)
   at Microsoft.AspNetCore.Mvc.ServiceFilterAttribute.CreateInstance(IServiceProvider serviceProvider)
   at Microsoft.AspNetCore.Mvc.Internal.DefaultFilterProvider.ProvideFilter(FilterProviderContext context, FilterItem filterItem)
   at Microsoft.AspNetCore.Mvc.Internal.DefaultFilterProvider.OnProvidersExecuting(FilterProviderContext context)
   at Microsoft.AspNetCore.Mvc.Internal.FilterFactory.CreateUncachedFiltersCore(IFilterProvider[] filterProviders, ActionContext actionContext, List`1 filterItems)
   at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvokerCache.GetCachedResult(ControllerContext controllerContext)
   at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvokerProvider.OnProvidersExecuting(ActionInvokerProviderContext context)
   at Microsoft.AspNetCore.Mvc.Internal.ActionInvokerFactory.CreateInvoker(ActionContext actionContext)
   at Microsoft.AspNetCore.Mvc.Internal.MvcAttributeRouteHandler.<>c__DisplayClass12_0.<RouteAsync>b__0(HttpContext c)
   at Microsoft.AspNetCore.Builder.RouterMiddleware.Invoke(HttpContext httpContext)
   at Microsoft.AspNetCore.Builder.Extensions.MapWhenMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Builder.Extensions.MapWhenMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.StaticFiles.StaticFileMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Diagnostics.StatusCodePagesMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware.Invoke(HttpContext context)

To Reproduce

With the application under load with strongly-typed objects used via IOptions<T> being resolved by requests, call IConfigurationRoot.Reload() by resolving it through DI. The configuration objects probably need to contain several levels of nested objects to cause the issue.

Expected behavior

• Requests started after Reload() is called then configuration should be bound from the new configuration.
• Requests started before and during reload should bind configuration to the old (or preferably new) configuration without throwing an exception and successfully return a response as intended by the application.

Additional context

The application this exception has been observed with is running ASP.NET Core 2.2.2.

[martincostello]

Tracing it through, it seems like if the Data dictionary in ConfigurationProvider (here) is swapped or resized by the reload, then this call to Any():

https://github.com/aspnet/Extensions/blob/260b3e2c8b1ef76036304fd87effb1a767fb8c1d/src/Configuration/Config.Binder/src/ConfigurationBinder.cs#L306

causes this enumerable to be walked:

https://github.com/aspnet/Extensions/blob/260b3e2c8b1ef76036304fd87effb1a767fb8c1d/src/Configuration/Config/src/InternalConfigurationRootExtensions.cs#L20-L24

which then will call through one of these two LINQ queries:

https://github.com/aspnet/Extensions/blob/260b3e2c8b1ef76036304fd87effb1a767fb8c1d/src/Configuration/Config/src/ConfigurationProvider.cs#L67-L71

https://github.com/aspnet/Extensions/blob/260b3e2c8b1ef76036304fd87effb1a767fb8c1d/src/Configuration/Config/src/ChainedConfigurationProvider.cs#L78-L83

I guess that somehow a reference to the dictionary property, rather than the actual dictionary instance, is being captured somewhere, and then when the reference changes mid-enumeration the exception is thrown by the enumerator.

[martincostello]

I'm not sure if it's the root cause, but while looking into this I found that the EnvironmentVariables
and KeyPerFile providers do not atomically swap the configuration during reload like many of the other providers do.

Have opened #1202 to change that.

[martincostello]

It's possible that this is actually a bug in an internal custom configuration source we use, which I'll look into and check on Monday.

However, I can consistently reproduce this exception with this test where Data is changed instead of being swapped:

[Fact]
public void BindingDoesNotThrowIfReloadedDuringBinding()
{
    var dic = new Dictionary<string, string>
    {
        {"Section:Integer", "-2"},
        {"Section:Boolean", "TRUe"},
        {"Section:Nested:Integer", "11"},
        {"Section:Virtual", "Sup"}
    };
    var configurationBuilder = new ConfigurationBuilder();
    configurationBuilder.AddInMemoryCollection(dic);
    configurationBuilder.Add(new CustomConfigurationSource());
    var config = configurationBuilder.Build();

    using (var cts = new CancellationTokenSource(TimeSpan.FromMilliseconds(250)))
    {
        void ReloadLoop()
        {
            while (!cts.IsCancellationRequested)
            {
                config.Reload();
            }
        }

        _ = Task.Run(ReloadLoop);

        while (!cts.IsCancellationRequested)
        {
            config.Get<ComplexOptions>();
        }
    }
}

private sealed class CustomConfigurationSource : IConfigurationSource
{
    public IConfigurationProvider Build(IConfigurationBuilder builder)
    {
        return new CustomConfigurationProvider();
    }
}

private sealed class CustomConfigurationProvider : ConfigurationProvider
{
    public override void Load()
    {
        Data["Section:Virtual"] = Guid.NewGuid().ToString();
    }
}

Test Name:	Microsoft.Extensions.Configuration.Binder.Test.ConfigurationBinderTests.BindingDoesNotThrowIfReloadedDuringBinding
Test FullName:	Microsoft.Extensions.Configuration.Binder.Test.ConfigurationBinderTests.BindingDoesNotThrowIfReloadedDuringBinding
Test Source:	C:\Coding\aspnet\Extensions\src\Configuration\Config.Binder\test\ConfigurationBinderTests.cs : line 760
Test Outcome:	Failed
Test Duration:	0:00:00.018

Result StackTrace:	
at System.Collections.Generic.Dictionary`2.Enumerator.MoveNext()
   at System.Linq.Enumerable.WhereSelectEnumerableIterator`2.MoveNext()
   at System.Collections.Generic.LargeArrayBuilder`1.AddRange(IEnumerable`1 items)
   at System.Collections.Generic.SparseArrayBuilder`1.AddRange(IEnumerable`1 items)
   at System.Collections.Generic.SparseArrayBuilder`1.ReserveOrAdd(IEnumerable`1 items)
   at System.Linq.Enumerable.Concat2Iterator`1.ToArray()
   at System.Linq.Buffer`1..ctor(IEnumerable`1 source)
   at System.Linq.OrderedEnumerable`1.GetEnumerator()+MoveNext()
   at System.Linq.Enumerable.DistinctIterator`1.MoveNext()
   at System.Linq.Enumerable.SelectEnumerableIterator`2.MoveNext()
   at System.Linq.Enumerable.Any[TSource](IEnumerable`1 source)
   at Microsoft.Extensions.Configuration.ConfigurationBinder.BindInstance(Type type, Object instance, IConfiguration config, BinderOptions options) in C:\Coding\aspnet\Extensions\src\Configuration\Config.Binder\src\ConfigurationBinder.cs:line 306
   at Microsoft.Extensions.Configuration.ConfigurationBinder.Get(IConfiguration configuration, Type type, Action`1 configureOptions) in C:\Coding\aspnet\Extensions\src\Configuration\Config.Binder\src\ConfigurationBinder.cs:line 82
   at Microsoft.Extensions.Configuration.ConfigurationBinder.Get[T](IConfiguration configuration, Action`1 configureOptions) in C:\Coding\aspnet\Extensions\src\Configuration\Config.Binder\src\ConfigurationBinder.cs:line 45
   at Microsoft.Extensions.Configuration.ConfigurationBinder.Get[T](IConfiguration configuration) in C:\Coding\aspnet\Extensions\src\Configuration\Config.Binder\src\ConfigurationBinder.cs:line 27
   at Microsoft.Extensions.Configuration.Binder.Test.ConfigurationBinderTests.BindingDoesNotThrowIfReloadedDuringBinding() in C:\Coding\aspnet\Extensions\src\Configuration\Config.Binder\test\ConfigurationBinderTests.cs:line 788
Result Message:	System.InvalidOperationException : Collection was modified; enumeration operation may not execute.

[martincostello]

Having looked into this more, I think the root cause is the non-atomic reload of the configuration data in the environment variables provider, which is fixed by #1202.

[martincostello]

Assuming this fix is approved, would it be eligible for back-porting to 2.1 and/or 2.2? If so, I'd be happy to raise the appropriate PRs.

[Tratcher]

The patch bar is quite high, the issue has to affect a significant number of users. Calling Reload seems pretty uncommon, what's your scenario for that? How frequently do you call Reload and how often does it fail?

[martincostello]

We have a custom IConfigurationProvider implementation that is wired up to our Consul cluster's key-value-pair store.

When a value is changed in Consul, we send a signal to the running instance(s) of the application(s) affected signaling that they should reload their configuration to pick up whatever these changes are.

The frequency of the changes cannot be predicted as they are made on-demand by our engineers as and when they need to update the configuration of an application.

In the last 30 days we have had 41 failed HTTP requests due to this specific exception and call path.

[Tratcher]

Fixed in 3.0 via the PR. @muratg for the patch discussion.

That's a fairly low error rate, and it's not clear that many people have this reload scenario. However the fix was in the EnvironmentVariable provider which every app uses.

[muratg]

• @ajcvickers for the patch bar.

[ajcvickers]

@muratg @Tratcher I would say yes for 2.2 patch--the bug is pretty bad; I think it should be patched in current.

For 2.1, probably not, but I think we should discuss with shiproom before making a final call. The only reason we might take it is that, since it's a race condition bug, it improves the overall stability of 2.1 and these issues are often hard for customers to report effectively because it can be hard to get them to consistently repro.

@muratg Are you going to prepare for shiproom, or shall I?

[muratg]

@ajcvickers Please go ahead. I'm guessing it'll just be a PR for 2.2 branch.

[ajcvickers]

@Tratcher Could you send out a PR porting this change to the release/2.2 branch? I'll then fill in the details for shiproom.