GeneratedInstance is not thread-safe

[starteleport]

I've encountered the following error with built-in ASPNET Core (netcoreapp3.1) healthchecks engine.

System.NullReferenceException: Object reference not set to an instance of an object.
   at System.Collections.Generic.Dictionary`2.FindEntry(TKey key)
   at System.Collections.Generic.Dictionary`2.TryGetValue(TKey key, TValue& value)
   at Lamar.IoC.Instances.GeneratedInstance.<>c__DisplayClass5_1.<BuildFuncResolver>b__0(Scope s)
   at Lamar.IoC.Scope.TryGetInstance(Type serviceType)
   at Lamar.IoC.Scope.GetService(Type serviceType)
   at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetService[T](IServiceProvider provider)
   <several stack frames ommitted due to NDA>
   at Microsoft.Extensions.Diagnostics.HealthChecks.DefaultHealthCheckService.RunCheckAsync(IServiceScope scope, HealthCheckRegistration registration, bCancellationToken cancellationToken)

I've got several MongoDB healthchecks that are registered as Scoped. DefaultHealthCheckService starts all the healthchecks in parallel like this:

var tasks = new Task<HealthReportEntry>[registrations.Count];
var index = 0;
using (var scope = _scopeFactory.CreateScope())
{
    foreach (var registration in registrations)
    {
        tasks[index++] = Task.Run(() => RunCheckAsync(scope, registration, cancellationToken), cancellationToken);
    }

    await Task.WhenAll(tasks).ConfigureAwait(false);
}

The problem seems to be that lambda returned from GeneratedInstance.BuildFuncResolver is not thread-safe. The problem reproduces when:

• the first thread acquires the lock and enters s.Services.Add method (here), and then initializes _buckets array and yet to initialize _entries array in Dictionary.Inititalize
• the second thread enters the first s.Services.TryGetValue method outside the lock and gets NullReferenceException here

I get rather stable reproduce on NDA'd code base and so far have not succeeded in creating stand-alone repro.

I've got two questions:

• Is there a reason to use Dictionary+lock vs ConcurrentDictionary?
• I could try to make PR with GeneratedInstance switched to ConcurrentDictionary and report whether the problem goes away. Do I need to put effort in reproducing the problem in stand-alone app, or this description is clear enough?

Thanks.

[jeremydmiller]

@starteleport Sorry for being so late, but I just saw this. Wouldn't you need the lock and ConcurrentDictionary though? For the sake of scoping, you cannot ever allow more than one resolver to be built.

[RyanThomas73]

We're encountering this same issue when accessing the service provider within our health checks. And yes the lock and ConcurrentDictionary should be used to ensure only one instance is ever resolved.

[RyanThomas73]

@jeremydmiller Will you accept a PR for this for the 4.x version of Lamar? We're not in a position to switch to net5 yet for the project's we're encountering this problem in.

For some reason I missed that the 5.0 version is multi-targeting netstandard2.0. I'm putting together a PR for this now.