AnyConcreteTypeNotAlreadyRegisteredSource concurrency issue

[MaratFaskhiev]

Hello,

Please check the code below:

using System.Threading.Tasks;
using Autofac;
using Autofac.Features.ResolveAnything;

namespace Test
{
    public class TestFoo
    {
    }

    internal class Program
    {
        public static void Main(string[] args)
        {
            for (int i = 0; i < 1000000; i++) {
                var builder = new ContainerBuilder();
                builder.RegisterSource(new AnyConcreteTypeNotAlreadyRegisteredSource());
                var container = builder.Build();
                Parallel.Invoke(
                    () => container.Resolve<TestFoo>(),
                    () => container.Resolve<TestFoo>());
            }
        }
    }
}

It fails with the following exception:

Unhandled Exception: System.AggregateException: One or more errors occurred. ---> System.InvalidOperationException: Sequence contains no elements
   at System.Linq.Enumerable.First[TSource](IEnumerable`1 source)
   at Autofac.Core.Registration.ServiceRegistrationInfo.TryGetRegistration(IComponentRegistration& registration) in C:\projects\autofac\src\Autofac\Core\Registration\ServiceRegistrationInfo.cs:line 175
   at Autofac.Core.Registration.ComponentRegistry.TryGetRegistration(Service service, IComponentRegistration& registration) in C:\projects\autofac\src\Autofac\Core\Registration\ComponentRegistry.cs:line 123
   at Autofac.ResolutionExtensions.TryResolveService(IComponentContext context, Service service, IEnumerable`1 parameters, Object& instance) in C:\projects\autofac\src\Autofac\ResolutionExtensions.cs:line 1035
   at Autofac.ResolutionExtensions.ResolveService(IComponentContext context, Service service, IEnumerable`1 parameters) in C:\projects\autofac\src\Autofac\ResolutionExtensions.cs:line 871
   at Autofac.ResolutionExtensions.Resolve[TService](IComponentContext context, IEnumerable`1 parameters) in C:\projects\autofac\src\Autofac\ResolutionExtensions.cs:line 300
   at Test.Program.<>c__DisplayClass0_0.<Main>b__1() in C:\Test\Test\Program.cs:line 21
   at System.Threading.Tasks.Task.InnerInvoke()
   at System.Threading.Tasks.Task.Execute()
   --- End of inner exception stack trace ---
   at System.Threading.Tasks.Task.FastWaitAll(Task[] tasks)
   at System.Threading.Tasks.Parallel.Invoke(ParallelOptions parallelOptions, Action[] actions)
   at System.Threading.Tasks.Parallel.Invoke(Action[] actions)
   at Test.Program.Main(String[] args) in C:\Test\Test\Program.cs:line 19

I checked Autofac 4.9.4. And the same issue occurs for the 5.0.0.

[tillig]

What's the use case for creating so many containers? Standard apps have just one. Can this be demonstrated with a more common use case?

[tillig]

Also, we need to see TestFoo. The stack trace here doesn't come from the repro.

[MaratFaskhiev]

As I can see this issue occurs because AnyConcreteTypeNotAlreadyRegisteredSource invokes registrationAccessor(service).Any(). During this invocation ComponentRegistry invokes info.CompleteInitialization(). It marks ServiceRegistrationInfo as initialized. And until AnyConcreteTypeNotAlreadyRegisteredSource created new registration there are no implementations for initialized ServiceRegistrationInfo.

What's the use case for creating so many containers? Standard apps have just one. Can this be demonstrated with a more common use case?

Thank you for the fast repsonse.

It is just a test example. Because this issue is a concurrent issue it is not occurs regularly. So I invoke the same code multiple times to reproduce the issue.

Also, we need to see TestFoo. The stack trace here doesn't come from the repro.

It is available in the first message:

    public class TestFoo
    {
    }

[MaratFaskhiev]

The stack trace here doesn't come from the repro.

Sorry. I updated it.

[MaratFaskhiev]

I can suggest the following solution. We can use ReaderWriterLockSlim instead of lock statement. I tested by using 4.9.4. It works fine with the test in my initial post. Also all tests from Autofac solution are passed. Please check the code below:

// This software is part of the Autofac IoC container
// Copyright © 2011 Autofac Contributors
// https://autofac.org
//
// Permission is hereby granted, free of charge, to any person
// obtaining a copy of this software and associated documentation
// files (the "Software"), to deal in the Software without
// restriction, including without limitation the rights to use,
// copy, modify, merge, publish, distribute, sublicense, and/or sell
// copies of the Software, and to permit persons to whom the
// Software is furnished to do so, subject to the following
// conditions:
//
// The above copyright notice and this permission notice shall be
// included in all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
// EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
// OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
// NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
// HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
// WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
// FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
// OTHER DEALINGS IN THE SOFTWARE.

using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Diagnostics;
using System.Globalization;
using System.Linq;
using System.Runtime.CompilerServices;
using System.Threading;
using Autofac.Builder;
using Autofac.Features.Decorators;
using Autofac.Util;

namespace Autofac.Core.Registration
{
    /// <summary>
    /// Maps services onto the components that provide them.
    /// </summary>
    /// <remarks>
    /// The component registry provides services directly from components,
    /// and also uses <see cref="IRegistrationSource"/> to generate components
    /// on-the-fly or as adapters for other components. A component registry
    /// is normally used through a <see cref="ContainerBuilder"/>, and not
    /// directly by application code.
    /// </remarks>
    public class ComponentRegistry : Disposable, IComponentRegistry
    {
        /// <summary>
        /// Protects instance variables from concurrent access.
        /// </summary>
        private readonly ReaderWriterLockSlim _lock = new ReaderWriterLockSlim(LockRecursionPolicy.SupportsRecursion);

        /// <summary>
        /// External registration sources.
        /// </summary>
        private readonly List<IRegistrationSource> _dynamicRegistrationSources = new List<IRegistrationSource>();

        /// <summary>
        /// All registrations.
        /// </summary>
        private readonly List<IComponentRegistration> _registrations = new List<IComponentRegistration>();

        /// <summary>
        /// Keeps track of the status of registered services.
        /// </summary>
        private readonly ConcurrentDictionary<Service, ServiceRegistrationInfo> _serviceInfo = new ConcurrentDictionary<Service, ServiceRegistrationInfo>();

        private readonly ConcurrentDictionary<IComponentRegistration, IEnumerable<IComponentRegistration>> _decorators
            = new ConcurrentDictionary<IComponentRegistration, IEnumerable<IComponentRegistration>>();

        /// <summary>
        /// Initializes a new instance of the <see cref="ComponentRegistry"/> class.
        /// </summary>
        public ComponentRegistry()
            : this(new Dictionary<string, object>())
        {
        }

        /// <summary>
        /// Initializes a new instance of the <see cref="ComponentRegistry"/> class.
        /// </summary>
        /// <param name="properties">The properties used during component registration.</param>
        internal ComponentRegistry(IDictionary<string, object> properties)
        {
            Properties = properties;
        }

        /// <summary>
        /// Gets the set of properties used during component registration.
        /// </summary>
        /// <value>
        /// An <see cref="IDictionary{TKey, TValue}"/> that can be used to share
        /// context across registrations.
        /// </value>
        public IDictionary<string, object> Properties { get; }

        /// <summary>
        /// Releases unmanaged and - optionally - managed resources.
        /// </summary>
        /// <param name="disposing"><c>true</c> to release both managed and unmanaged resources; <c>false</c> to release only unmanaged resources.</param>
        protected override void Dispose(bool disposing)
        {
            foreach (var registration in _registrations)
                registration.Dispose();

            base.Dispose(disposing);
        }

        /// <summary>
        /// Attempts to find a default registration for the specified service.
        /// </summary>
        /// <param name="service">The service to look up.</param>
        /// <param name="registration">The default registration for the service.</param>
        /// <returns>True if a registration exists.</returns>
        public bool TryGetRegistration(Service service, out IComponentRegistration registration)
        {
            if (service == null) throw new ArgumentNullException(nameof(service));

            _lock.EnterUpgradeableReadLock();
            try
            {
                var info = GetInitializedServiceInfoOrDefault(service);
                if (info != null && info.TryGetRegistration(out registration))
                    return true;

                _lock.EnterWriteLock();
                try
                {
                    info = GetInitializedServiceInfo(service);
                    return info.TryGetRegistration(out registration);
                }
                finally
                {
                    _lock.ExitWriteLock();
                }
            }
            finally
            {
                _lock.ExitUpgradeableReadLock();
            }
        }

        /// <summary>
        /// Determines whether the specified service is registered.
        /// </summary>
        /// <param name="service">The service to test.</param>
        /// <returns>True if the service is registered.</returns>
        public bool IsRegistered(Service service)
        {
            if (service == null) throw new ArgumentNullException(nameof(service));

            _lock.EnterUpgradeableReadLock();
            try
            {
                var info = GetInitializedServiceInfoOrDefault(service);
                if (info != null && info.IsRegistered)
                    return true;

                _lock.EnterWriteLock();
                try
                {
                    return GetInitializedServiceInfo(service).IsRegistered;
                }
                finally
                {
                    _lock.ExitWriteLock();
                }
            }
            finally
            {
                _lock.ExitUpgradeableReadLock();
            }
        }

        /// <summary>
        /// Register a component.
        /// </summary>
        /// <param name="registration">The component registration.</param>
        public void Register(IComponentRegistration registration)
        {
            Register(registration, false);
        }

        /// <summary>
        /// Register a component.
        /// </summary>
        /// <param name="registration">The component registration.</param>
        /// <param name="preserveDefaults">If true, existing defaults for the services provided by the
        /// component will not be changed.</param>
        public void Register(IComponentRegistration registration, bool preserveDefaults)
        {
            if (registration == null) throw new ArgumentNullException(nameof(registration));

            _lock.EnterWriteLock();
            try
            {
                AddRegistration(registration, preserveDefaults);
                UpdateInitialisedAdapters(registration);
            }
            finally
            {
                _lock.ExitWriteLock();
            }
        }

        private void UpdateInitialisedAdapters(IComponentRegistration registration)
        {
            var adapterServices = new List<Service>();
            foreach (var serviceInfo in _serviceInfo)
            {
                if (serviceInfo.Value.ShouldRecalculateAdaptersOn(registration))
                {
                    adapterServices.Add(serviceInfo.Key);
                }
            }

            if (adapterServices.Count == 0)
                return;

            Debug.WriteLine(
                string.Format(
                    CultureInfo.InvariantCulture,
                    "[Autofac] Component '{0}' provides services that have already been adapted. Consider refactoring to ContainerBuilder.Build() rather than Update().",
                    registration));

            var adaptationSandbox = new AdaptationSandbox(
                _dynamicRegistrationSources.Where(rs => rs.IsAdapterForIndividualComponents),
                registration,
                adapterServices);

            // Adapter registrations come from sources, so they are added with originatedFromSource: true
            var adapters = adaptationSandbox.GetAdapters();
            foreach (var adapter in adapters)
                AddRegistration(adapter, true, true);
        }

        protected virtual void AddRegistration(IComponentRegistration registration, bool preserveDefaults, bool originatedFromSource = false)
        {
            foreach (var service in registration.Services)
            {
                var info = GetServiceInfo(service);
                info.AddImplementation(registration, preserveDefaults, originatedFromSource);
            }

            _registrations.Add(registration);

            GetRegistered()?.Invoke(this, new ComponentRegisteredEventArgs(this, registration));
        }

        /// <summary>
        /// Gets the registered components.
        /// </summary>
        public IEnumerable<IComponentRegistration> Registrations
        {
            get
            {
                _lock.EnterWriteLock();
                try
                {
                    return _registrations.ToArray();
                }
                finally
                {
                    _lock.ExitWriteLock();
                }
            }
        }

        /// <summary>
        /// Selects from the available registrations after ensuring that any
        /// dynamic registration sources that may provide <paramref name="service"/>
        /// have been invoked.
        /// </summary>
        /// <param name="service">The service for which registrations are sought.</param>
        /// <returns>Registrations supporting <paramref name="service"/>.</returns>
        public IEnumerable<IComponentRegistration> RegistrationsFor(Service service)
        {
            if (service == null) throw new ArgumentNullException(nameof(service));

            _lock.EnterUpgradeableReadLock();
            try
            {
                var info = GetInitializedServiceInfoOrDefault(service);
                if (info != null)
                    return info.Implementations.ToArray();

                _lock.EnterWriteLock();
                try
                {
                    info = GetInitializedServiceInfo(service);
                    return info.Implementations.ToArray();
                }
                finally
                {
                    _lock.ExitWriteLock();
                }
            }
            finally
            {
                _lock.ExitUpgradeableReadLock();
            }
        }

        /// <inheritdoc />
        public IEnumerable<IComponentRegistration> DecoratorsFor(IComponentRegistration registration)
        {
            if (registration == null) throw new ArgumentNullException(nameof(registration));

            return _decorators.GetOrAdd(registration, r =>
            {
                var result = new List<IComponentRegistration>();

                foreach (var service in r.Services)
                {
                    if (service is DecoratorService || !(service is IServiceWithType swt)) continue;

                    var decoratorService = new DecoratorService(swt.ServiceType);
                    var decoratorRegistrations = RegistrationsFor(decoratorService);
                    result.AddRange(decoratorRegistrations);
                }

                return result.OrderBy(d => d.GetRegistrationOrder()).ToArray();
            });
        }

        /// <summary>
        /// Fired whenever a component is registered - either explicitly or via a
        /// <see cref="IRegistrationSource"/>.
        /// </summary>
        public event EventHandler<ComponentRegisteredEventArgs> Registered
        {
            add
            {
                _lock.EnterWriteLock();
                try
                {
                    Properties[MetadataKeys.RegisteredPropertyKey] = GetRegistered() + value;
                }
                finally
                {
                    _lock.ExitWriteLock();
                }
            }

            remove
            {
                _lock.EnterWriteLock();
                try
                {
                    Properties[MetadataKeys.RegisteredPropertyKey] = GetRegistered() - value;
                }
                finally
                {
                    _lock.ExitWriteLock();
                }
            }
        }

        /// <summary>
        /// Add a registration source that will provide registrations on-the-fly.
        /// </summary>
        /// <param name="source">The source to register.</param>
        public void AddRegistrationSource(IRegistrationSource source)
        {
            if (source == null) throw new ArgumentNullException(nameof(source));

            _lock.EnterWriteLock();
            try
            {
                _dynamicRegistrationSources.Insert(0, source);
                foreach (var serviceRegistrationInfo in _serviceInfo)
                    serviceRegistrationInfo.Value.Include(source);

                var handler = RegistrationSourceAdded;
                handler?.Invoke(this, new RegistrationSourceAddedEventArgs(this, source));
            }
            finally
            {
                _lock.ExitWriteLock();
            }
        }

        /// <summary>
        /// Gets the registration sources that are used by the registry.
        /// </summary>
        public IEnumerable<IRegistrationSource> Sources
        {
            get
            {
                _lock.EnterWriteLock();
                try
                {
                    return _dynamicRegistrationSources.ToArray();
                }
                finally
                {
                    _lock.ExitWriteLock();
                }
            }
        }

        /// <summary>
        /// Gets a value indicating whether the registry contains its own components.
        /// True if the registry contains its own components; false if it is forwarding
        /// registrations from another external registry.
        /// </summary>
        /// <remarks>This property is used when walking up the scope tree looking for
        /// registrations for a new customised scope.</remarks>
        public bool HasLocalComponents => true;

        /// <summary>
        /// Fired when an <see cref="IRegistrationSource"/> is added to the registry.
        /// </summary>
        public event EventHandler<RegistrationSourceAddedEventArgs> RegistrationSourceAdded;

        private ServiceRegistrationInfo GetInitializedServiceInfo(Service service)
        {
            var info = GetServiceInfo(service);
            if (info.IsInitialized)
                return info;

            if (!info.IsInitializing)
                info.BeginInitialization(_dynamicRegistrationSources);

            while (info.HasSourcesToQuery)
            {
                var next = info.DequeueNextSource();
                foreach (var provided in next.RegistrationsFor(service, RegistrationsFor))
                {
                    // This ensures that multiple services provided by the same
                    // component share a single component (we don't re-query for them)
                    foreach (var additionalService in provided.Services)
                    {
                        var additionalInfo = GetServiceInfo(additionalService);
                        if (additionalInfo.IsInitialized || additionalInfo == info) continue;

                        if (!additionalInfo.IsInitializing)
                            additionalInfo.BeginInitialization(_dynamicRegistrationSources.Where(src => src != next));
                        else
                            additionalInfo.SkipSource(next);
                    }

                    AddRegistration(provided, true, true);
                }
            }

            info.CompleteInitialization();
            return info;
        }

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        private ServiceRegistrationInfo GetServiceInfo(Service service)
        {
            if (_serviceInfo.TryGetValue(service, out var existing))
                return existing;

            var info = new ServiceRegistrationInfo(service);
            _serviceInfo.TryAdd(service, info);
            return info;
        }

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        private ServiceRegistrationInfo GetInitializedServiceInfoOrDefault(Service service)
        {
            if (_serviceInfo.TryGetValue(service, out var existing) && existing.IsInitialized)
                return existing;

            return null;
        }

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        private EventHandler<ComponentRegisteredEventArgs> GetRegistered()
        {
            if (Properties.TryGetValue(MetadataKeys.RegisteredPropertyKey, out var registered))
                return (EventHandler<ComponentRegisteredEventArgs>)registered;

            return null;
        }
    }
}

[tillig]

Ok, we'll have to look into this, reproduce with your code, and check out the fix. Unfortunately you've caught us in the middle of updating things post-Autofac-5.0 release (eg the other 20+ integration packages) so it'll be a bit before we get here.

If you'd like it faster, you could...

• Add a failing test and the fix as a pull request.
• Run the benchmark tests before and after your fix to ensure we won't be losing perf. If there isn't a benchmark around ACTNARS maybe add one. We get nailed pretty hard on perf in the community.

[MaratFaskhiev]

@tillig Thank you. Actually, I'm using 4.9.4 now. I read this document: https://github.com/autofac/Autofac/blob/develop/CONTRIBUTING.md

And it looks like I can create PR to the develop branch only. So this fix will be available in 5.x only. Am I correct?

[tillig]

That's correct. We don't back-port fixes or features.

[MaratFaskhiev]

Ok. Thank you. It looks like most of the logic was migrated from ComponentRegistry to DefaultRegisteredServicesTracker. I'll try to provide a PR.

[MaratFaskhiev]

@tillig I created a PR: #1076

But I see some performance degradation at Concurrency benchmark. Possibly there exists a better solution than my.

[ilabutin]

@MaratFaskhiev Could you post 'before' and 'after' numbers here from the Concurrency benchmark?

[MaratFaskhiev]

@ilabutin Here it is:

Concurrency

Before:
Mean - 1.671 ms
Error - 0.0214 ms
StdDev - 0.0200 ms

After:
Mean - 8.832 ms
Error - 0.0728 ms
StdDev - 0.0646 ms

ConcurrencyNestedScopeBenchmark

Before:
Mean - 3.173 ms
Error - 0.0823 ms
StdDev - 0.2414 ms

After:
Mean - 6.666 ms
Error - 0.1322 ms
StdDev - 0.1358 ms

[alexmg]

Reopening issue after noticing test failure during CI build.

https://ci.appveyor.com/project/Autofac/autofac/build/tests

[alexmg]

We may have to return the original and safest locking approach. See this comment for benchmarks.

#1076 (comment)

[tillig]

That sucks since we get nailed on perf constantly, but I don't immediately see a better way. I come up with random, complicated stuff like keeping a separate "count" of the implementations that could be used instead of the Any call but then I realized that whole check in that line with the Any needs to be atomic, which is what locks do. Darn.

[weelink]

I was wondering why ServiceRegistrationInfo.Implementations is being calculated everytime it is being accessed. One possible option is to cache the result and recalculate it only when a registration has been added. I created a PR where this has been replaced with a Lazy<IEnumerable<IComponentRegistration>>.

#1080

[alexmg]

I can see what @weelink has done in the PR but worry that we will just keep moving the threading problem from one place to another, leaving behind a trail of obscure fixes that could be easily broken. The PR with the caching may be good for performance but from a safety perspective I think the original locking needs to come back.