Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/1077 component registration activating properties autowired #1079

Conversation

weelink
Copy link
Contributor

@weelink weelink commented Feb 7, 2020

No description provided.

@weelink
Copy link
Contributor Author

weelink commented Feb 7, 2020

When a registration is added during the lookup of a ServiceRegistrationInfo, the Registered and RegistrationSourceAdded events aren't raised. In order to raise these events, a reference to a IComponentRegistryBuilder is required.

This introduces a nasty design issue. ComponentRegistryBuilder depends on a IRegisteredServicesTracker and DefaultRegisteredServicesTracker depends on a ComponentRegistryBuilder, but only when raising these events.

The two failed tests (WhenRegistrationIsMade_ComponentRegisteredEventFired and WhenASourceIsAddedToTheRegistry_TheSourceAddedEventIsRaised) also indicate that there is some design problem.

The underlying problem that needs to be fixed is how to raise these events from DefaultRegisteredServicesTracker?

@weelink weelink marked this pull request as ready for review February 7, 2020 14:01
@weelink weelink force-pushed the feature/1077_ComponentRegistrationActivating_PropertiesAutowired branch from 0293e6f to 520e6bd Compare February 7, 2020 14:03
@weelink
Copy link
Contributor Author

weelink commented Feb 7, 2020

The failing test is WhenSeveralThreadsResolveNotAlreadyRegisteredType_DoesNotThrow. This PR doesn't touch any code related to this test as far as I can tell.

This may be an unrelated bug. @tillig any thoughts?

@tillig
Copy link
Member

tillig commented Feb 7, 2020

Entirely from the top of my head, because unfortunately I'm super swamped with the old day job at the moment and all my "free time" was used up by the Autofac 5 release and ensuing set of other releases, bugs, etc...

The test in question:

        [Fact]
        public void WhenSeveralThreadsResolveNotAlreadyRegisteredType_DoesNotThrow()
        {
            for (var i = 0; i < 20_000; i++)
            {
                var builder = new ContainerBuilder();
                builder.RegisterSource(new AnyConcreteTypeNotAlreadyRegisteredSource());
                var container = builder.Build();
                Parallel.Invoke(() => container.Resolve<A>(), () => container.Resolve<A>());
            }
        }

And the failed test error stack trace:

  Error Message:
   System.AggregateException : One or more errors occurred.
---- System.InvalidOperationException : Collection was modified; enumeration operation may not execute.
  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 Autofac.Test.Concurrency.ConcurrencyTests.WhenSeveralThreadsResolveNotAlreadyRegisteredType_DoesNotThrow() in C:\projects\autofac\test\Autofac.Test\Concurrency\ConcurrencyTests.cs:line 144
----- Inner Stack Trace -----
   at System.ThrowHelper.ThrowInvalidOperationException(ExceptionResource resource)
   at System.Collections.Generic.List`1.Enumerator.MoveNextRare()
   at System.Collections.Generic.List`1.Enumerator.MoveNext()
   at System.Linq.Enumerable.<ConcatIterator>d__59`1.MoveNext()
   at System.Linq.Enumerable.Any[TSource](IEnumerable`1 source)
   at Autofac.Core.Registration.DefaultRegisteredServicesTracker.TryGetRegistration(Service service, IComponentRegistration& registration) in C:\projects\autofac\src\Autofac\Core\Registration\DefaultRegisteredServicesTracker.cs:line 147
   at Autofac.Core.Registration.ComponentRegistry.TryGetRegistration(Service service, IComponentRegistration& registration) in C:\projects\autofac\src\Autofac\Core\Registration\ComponentRegistry.cs:line 92
   at Autofac.ResolutionExtensions.TryResolveService(IComponentContext context, Service service, IEnumerable`1 parameters, Object& instance) in C:\projects\autofac\src\Autofac\ResolutionExtensions.cs:line 1064
   at Autofac.ResolutionExtensions.ResolveService(IComponentContext context, Service service, IEnumerable`1 parameters) in C:\projects\autofac\src\Autofac\ResolutionExtensions.cs:line 892
   at Autofac.ResolutionExtensions.Resolve[TService](IComponentContext context, IEnumerable`1 parameters) in C:\projects\autofac\src\Autofac\ResolutionExtensions.cs:line 314
   at Autofac.Test.Concurrency.ConcurrencyTests.<>c__DisplayClass5_0.<WhenSeveralThreadsResolveNotAlreadyRegisteredType_DoesNotThrow>b__1() in C:\projects\autofac\test\Autofac.Test\Concurrency\ConcurrencyTests.cs:line 144
   at System.Threading.Tasks.Task.InnerInvoke()
   at System.Threading.Tasks.Task.Execute()

This is potentially related to a recent change @alexmg worked on to try to solve a threading issue where ACTNARS was being problematic (#1073).

After inlining, it appears the problem is when it's looking at the Any call here to see if there are any existing implementations. That was inlined from the call on line 147.

That code:

if (_serviceInfo.TryGetValue(service, out var existing) && existing.IsInitialized && existing.Implementations.Any())
    return existing;

My gut feel is that potentially the fix for #1073 without the locking may have been passing... coincidentally? That is, before your changes the ACTNARS register-the-type-on-the-fly was happening fast enough that it would do the whole registration on one thread before the other thread could do its check for Any(). However, with the new changes where the Registered event is getting raised as things are added on the fly, there's enough delay that the thread is switching. So it's like...

  • A: Get pretty far into the registration process so it's about to add the registration on the fly.
  • B: Start into the registration process. Get into that line above, dereference existing.Implementations, and switch threads.
  • A: Finish the registration process, raise the event (adding a delay), switch threads.
  • B: existing.Implementations starts enumerating Any() and sees thread A changed the collection. Explode.

Roughly.

I don't know off the top of my head how to fix that. Is there a need for a concurrent collection for the implementations set? How will that affect perf? Is there a better call than Any() to make? I'm stabbing in the dark, brainstorming, not recommending any of these things. That's my off-the-cuff hypothesis with the small amount of time that I can look at this right now....

...which, unfortunately, just hit like half an hour so I'm toooootally out on this for now. Perhaps @alexmg, @alistairjevans, @alsami, or one of the other members of the @autofac/autofac org can help.

Sync autofac/Autofac develop
@weelink weelink force-pushed the feature/1077_ComponentRegistrationActivating_PropertiesAutowired branch from 520e6bd to b6340ac Compare February 12, 2020 13:06
@alexmg alexmg merged commit e938b26 into autofac:develop Feb 12, 2020
@weelink weelink deleted the feature/1077_ComponentRegistrationActivating_PropertiesAutowired branch February 17, 2020 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants