Threading issue in LifeTimeScope.GetOrCreateAndShare

[hackfaq]

I have WCF service, and on method calls I get exception:

An item with the same key has already been added.

System.ThrowHelper.ThrowArgumentException(ExceptionResource resource)
   в System.Collections.Generic.Dictionary`2.Insert(TKey key, TValue value, Boolean add)
   в System.Collections.Generic.Dictionary`2.Add(TKey key, TValue value)
   в Autofac.Core.Lifetime.LifetimeScope.GetOrCreateAndShare(Guid id, Func`1 creator)

as I investigated sources such error can't happen, because it checks before add.

[tillig]

Can you please provide a repro and post it somewhere? Given many people are successfully using Autofac with WCF and not seeing this error, we'll need to see it ourselves in order to troubleshoot.

[hackfaq]

look at image, I already have TicketController in _sharedInstances with id, but it tries tp insert again.

[hackfaq]

I have rewrited method:GetOrCreateAndShare, added check for key, and it it works now.

public object GetOrCreateAndShare(Guid id, Func<object> creator)
        {
            if (creator == null)
            {
                throw new ArgumentNullException("creator");
            }
            lock (_synchRoot)
            {
                object result;
                if (!_sharedInstances.TryGetValue(id, out result))
                {
                    result = creator();
                    lock (_synchRoot)
                    {
                        if (!_sharedInstances.ContainsKey(id))
                            _sharedInstances.Add(id, result);
                    }
                }
                return result;
            }
        }

[alexmg]

A unit test showing the issue along with the fix would be best. Wondering what exactly the cause is and a screen capture isn't something we can execute to understand the problem.

[hackfaq]

Dictionary is not thread safe, so it should be double checked, I think

[alexmg]

I think I see the problem. I'll take a look. I suspect we should also prevent the creator function from being invoked a second time too, even if the second lock prevents the instance from being added to the dictionary.

[tillig]

I'm guessing the ContainsKey call is what made the difference (though it escapes me how, since it should have been covered by the try-get). The second/nested lock won't have any effect on the use of the dictionary because no other threads can get into that critical section while the outer lock is held.

[hackfaq]

reproduced in unit test:
https://www.dropbox.com/s/pbccako97rr8h7g/AutofacBugTest.zip?dl=0

[alexmg]

This isn't a threading issue at all. The problem is that while creating an instance of A, you are invoking a factory during its construction that creates another instance of A. The factory generated A gets added to the dictionary because its first to complete creation, and when the original A is finally being added to the dictionary it's too late. We don't detect this in the circular dependency check because the initial and pending activation of A hasn't yet completed.

Your second unit test passes because when B is created first, it creates the instance of A via the factory, and when it then requests A as its second constructor parameter, the instance already exists and no attempt to create it is made.

The code change you made basically allows a second instance to be created even though you are only supposed to receive one per lifetime scope (A is configured as InstancePerLifetimeScope). This is being prevented in the current code through the exception, though a meaningful exception warning you what is happening would be better. I'm attempting to do exactly that but have to battle through the DNX RC2 upgrade first.

[alexmg]

I have updated LifetimeScope to throw a DependencyResolutionException with the message below:

The constructor of type '{0}' attempted to create another instance of itself. This is not permitted because the service is configured to only allowed a single instance per lifetime scope.

That should be more helpful than the dictionary exception.