Autofac: Exception `InvalidOperationException: Collection was modified; enumeration operation may not execute` while disposing the scope

[Alexandr-Gurinovich]

Exception stacktrace:

Xamarin caused by: android.runtime.JavaProxyThrowable: System.InvalidOperationException: Collection was modified; enumeration operation may not execute.
System.Collections.Generic.List<T>.Enumerator.MoveNext()
Autofac.Core.Registration.ComponentRegistry.Dispose(bool disposing)
Autofac.Util.Disposable.Dispose()
Autofac.Core.Disposer.Dispose(bool disposing)
Autofac.Util.Disposable.Dispose()
Autofac.Core.Lifetime.LifetimeScope.Dispose(bool disposing)
Autofac.Util.Disposable.Dispose()
Autofac.AutofacContainer.Dispose()

Test covering this case:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Autofac.Builder;
using Autofac.Core;
using Autofac.Core.Registration;
using Autofac.Features.Collections;
using Autofac.Features.GeneratedFactories;
using Autofac.Features.Metadata;
using Autofac.Test.Scenarios.RegistrationSources;
using Xunit;

namespace Autofac.Test.Core.Registration
{
    public class ComponentRegistryTests
    {
        [Fact]
        public async Task DisposeComponentRegistry_RegisteringComponentAsync_NoExceptionRaised()
        {
            var registry = new ComponentRegistry();
            registry.AddRegistrationSource(new MetaRegistrationSource());
            var token = new CancellationTokenSource();

            Task.Run(
                () =>
                {
                    try
                    {
                        while (true)
                        {
                            var first = RegistrationBuilder.ForType<object>().CreateRegistration();
                            registry.Register(first);
                            if (token.IsCancellationRequested)
                            {
                                break;
                            }
                        }
                    }
                    catch (ObjectDisposedException)
                    {
                    }
                });
            await Task.Delay(1);
            registry.Dispose();
            token.Cancel();
        }
    }
}

In my case I did not call Register method directly, but was resolving some Service which lead to invocation of AddRegistration method. The test gives coverage of the case with the same exception thrown (modification of _registrations collection while Dispose is in progress).

[tillig]

Let me restate what it sounds like happened and let me know if I'm wrong:

• Thread 1 started resolving a service.
• Thread 2 called Dispose on the same lifetime scope (or a parent scope) that Thread 1 is using.
• Thread 1 caused Register to be called.
• Thread 2 threw the InvalidOperationException.

So, short version - you were trying to resolve from a scope / registry whilst disposing that same scope / registry. Is that correct?

[Alexandr-Gurinovich]

Correct. Exactly the same situation.

[tillig]

What do you expect to happen? Would you rather have an ObjectDisposedException? (In either case it's up to your app code to manage safe creation/disposal of scopes...)

[Alexandr-Gurinovich]

If Dispose method looked like this

        protected override void Dispose(bool disposing)
        {
            foreach (var registration in Registrations)
                registration.Dispose();

            base.Dispose(disposing);
        } 

or

        protected override void Dispose(bool disposing)
        {
            lock (_synchRoot)
            {
                foreach (var registration in _registrations)
                    registration.Dispose();
            }

            base.Dispose(disposing);
        }

we would have all the registrations disposed. Except those which had been added during Dispose method execution.

Now we have an exception and random number of not disposed registrations. The issue is more references to leaks than to the exception handling from my side. If you have a solution to finish disposing after exception handling I would really appreciate you.

@tillig what do you think?

[tillig]

Well, that's really the problem - once you dispose the registry, you don't get to add registrations. We'd not only have to add the lock, but also a check on every operation to ensure the object hasn't already been disposed. Anything added post-disposal will get ObjectDisposedException instead of the InvalidOperationException. It'd be a nicer exception, to be sure, but wouldn't solve the root cause of your problem, which is: In _your app code_ you need to ensure you're not doing what you're doing - resolving on one thread while disposing on another.

I don't anticipate we'll add support for any case where one thread is adding registrations while another is disposing... and then somehow safely recover and continue disposing after the other registrations were added on the other thread.

[Alexandr-Gurinovich]

If we check all the collection _registrations modification/access calls we'll see that all of them are surrounded with lock (_synchRoot). Dispose method is the only place without the lock. I think it should be added there.

According to documentation Container and ComponentRegistry are thread-safe types so my scenario is a valid situation.

[tillig]

I'm not arguing the lock. I'm saying even with the lock if you're trying to resolve in one thread and dispose on a different thread simultaneously something bad is going to happen and your app code is going to have to guard against that. You're not properly isolating your unit of work.

Again, lock during dispose, no problem. Ensure we throw an ObjectDisposedException if someone tries to access after that, no problem. I don't think that will fix the root issue here because you'll still get an exception in your app if you are disposing out from under yourself.

[Alexandr-Gurinovich]

ObjectDisposedException is covered from my side when I'm using Resolve. Covering Dispose will fix the root issue in my case. I did check it.

[marchewek]

In either case it's up to your app code to manage safe creation/disposal of scopes...

While I agree that Register and Dispose could be thread safe... I would still advise to protect initialization and disposing code with a lock - at the very minimum a single Register should be protected, but personally I would protect entire registration. Moreover, as a rule of thumb you should not introduce logic relying on the ObjectDisposedException occurence - when you encounter this exception, you should know that you've done something wrong in the first place ;) obviously, such solution would make disposing last longer and imo it should take as much time as needed for ensuring graceful closure.

Btw wouldn't this problem be solved by calling token.Cancel() before registry.Dispose()?