autofac / Autofac.Mvc

ASP.NET MVC integration for Autofac
MIT License
49 stars 23 forks source link

System.ArgumentOutOfRangeException: Index was out of range. Must be non-negative and less than the size of the collection #26

Closed alohaninja closed 5 years ago

alohaninja commented 5 years ago

We noticed a race condition with the method builder.RegisterFilterProvider() which is not thread-safe. We had multiple modules - some inherited that were also calling this method. Since this method is removing from a shared dictionary System.Web.Mvc.FilterProviders.Providers - it should lock access to prevent the race condition in cases where the filter has already been removed from the shared dictionary.

Race Condition - FilterProviders.Providers

image

Simple Reproduction - Overstated

var builder = new ContainerBuilder();
Parallel.For(1, 10, c => builder.RegisterFilterProvider());

This above code will generate the following exception:

System.ArgumentOutOfRangeException: Index was out of range. Must be non-negative and less than the size of the collection.
Parameter name: index
   at System.ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument argument, ExceptionResource resource)
   at System.Collections.Generic.List`1.RemoveAt(Int32 index)
   at System.Web.Mvc.FilterProviderCollection.RemoveItem(Int32 index)
   at System.Collections.ObjectModel.Collection`1.Remove(T item)
   at Autofac.Integration.Mvc.RegistrationExtensions.RegisterFilterProvider(ContainerBuilder builder)

Note: his bug has taken us months to identify and narrow down to this single line - hopefully we can squash this for others benefit.

tillig commented 5 years ago

Per the docs:

ContainerBuilder is not thread-safe and is designed to be used only on a single thread at the time the application starts up.

I'm sorry you ran into this issue, but the solution is to not register things in parallel. Even if you solve this one thing, there's no guarantee other things also won't cause trouble down the road.

The benefit of parallel registration is likely negligible. Given it should happen only once - app startup, test initialization - optimizing for that use case isn't terribly interesting.

However, should it seem interesting, you'd need to start all the way down in core Autofac with the ContainerBuilder itself. We'd be interested in seeing some benchmark tests on what sort of performance differences there are in both single threaded registration and parallel if locks are in place down the stack.

alohaninja commented 5 years ago

@tillig - thanks for the direction and pointer to the docs reference regarding threading support!

We are tracing down a heisenbug - this came out of stress-testing the Autofac container registration, building, and resolution workflow in the same App Domain. For now - we will stress test our container startup using separate app domains to isolate our container issue. Will report back if we find anything interesting. šŸ‘