Fix for #635 SingleInstance lock

[avtc]

Use lock only if instance not created yet

ConcurrentDictionary GetOrAdd AddOrUpdate with value factory is not thread safe, so lock still needed
https://github.com/dotnet/corefx/issues/25609

[eugbaranov]

What's the benefit of having ConcurrentDictionary given that adding value is behind the lock?
Technically speaking ConcurrentDictionary is thread-safe but two competing threads might end up filling the same key discarding one of the results. Instead of double locking (explicit _syncRoot and implicit within TryAdd) usually it is better to have ConcurrentDictionary<Guid, Lazy<object>> where Lazy will guarantee execution protection.

[avtc]

Benefit of using ConcurrentDictionary is lock-free Resolve (when instance exists).

Lazy with Execution protection also uses Monitor lock, so it will still be two locks (TryAdd and Lazy):
https://github.com/Microsoft/referencesource/blob/60a4f8b853f60a424e36c7bf60f9b5b5f1973ed1/mscorlib/system/Lazy.cs#L382

Also I am not sure if its possible to retain existing logic for DependencyResolutionException with Lazy
Anyway such change can affect only performance of initializing of SingleInstance-s, which is not critical.

[eugbaranov]

Fair enough. Also on a second thought although Lazy provides finer lock granularity coarser lock it required since #666 (and exception you mentioned) suggests that creator() might result in creating multiple instances. @alexmg will probably know the best.

[tillig]

Hi! I want to acknowledge that I recognize this PR is here, but with the holidays approaching and family/job obligations increasing, I likely won't personally have time to review and/or approve this in the super near future. @alexmg may have some additional available time, but I'm a little swamped. I apologize in advance for any delay.

[alexmg]

I have used Lazy<T> with ConcurrentDictionary.GetOrAdd in the past successfully, but in this case the requirement to throw the DependencyResolutionException from #666 would makes its use rather akward. There is no simple way to know if an existing instance was returned or not, so in the case of the recursive resolve operation the Value property of the Lazy<T> instance would be accessed from inside the value factory function, causing an InvalidOperationException to be thrown, and that would need to be caught as a unreliable means of detecting the cycle.

In addition, because the Func<object> is passed into the LifetimeScope.GetOrCreateAndShare method it would need to be wrapped in a Lazy<T> or Func<Lazy<T>> in order to call ConcurrentDictionary<Guid, Lazy<object>>.GetOrAdd. This extra allocation would be needed every time the method was called, even in cases where the instance already exists in the dictionary. The fact that Lazy<T> is being used as an optimization inside the LifetimeScope class should not be visible externally so changing the signature of GetOrCreateAndShare isn't desirable either.

I think this is a good change overall and the trade off with manual locking to meet the requirement of #666 is reasonable. I'm going to pull this in for the 4.9.0 release.