Scoped Instances owned by a transient are not always unique

[nathansoz]

(May be related to closed issue #156)

I have a service where I noticed a code path being hit that should be impossible if scoped services are scoped per-request. After adding more logging, I found that a transient-scoped object that had a scoped-scoped object would indeed sometimes have the same object used between two requests. This usually only happened when the service was under load and the rate was ~1/1000, so I suspect a race condition, but haven't yet dived in far enough to be sure.

Unfortunately I cannot post my code here, but I have made a small repro project that demonstrates the issue. You can find it here (https://github.com/nathansoz/LamarScopedRepro)

In the project there is a dependency graph that looks like:

 TransientGuidResolver -> 
     TransientGuidProvider ->
         ScopedGuidProvider

The scoped guid provider creates one guid on construction, passes it up through the two transient objects, and then the project aggregates the results over many instances from unique scopes in parallel to find duplicates.

The project also demonstrates that the same condition does not repro on vanilla Microsoft.Extensions.DependencyInjection. I've pasted an example below of what the project does:

// Vanilla DI
var serviceProvider = new ServiceCollection()
                .AddScoped<IScopedGuidProvider, ScopedGuidProvider>()
                .AddTransient<ITransientGuidResolver, TransientGuidResolver>()
                .AddTransient<ITransientGuidProvider, TransientGuidProvider>().BuildServiceProvider();

OUTPUTS

!!!!! RUNNING WITH DOTNET BASIC DI !!!!!
Using 100 degrees of parallelism and generating 1000000 guids
There were 1000000 unique guids

VS

Console.WriteLine("!!!!! RUNNING WITH LAMAR !!!!!");
var container = new Container(x =>
{
    x.For<IScopedGuidProvider>().Use<ScopedGuidProvider>().Scoped();
    x.For<ITransientGuidResolver>().Use<TransientGuidResolver>();
    x.For<ITransientGuidProvider>().Use<TransientGuidProvider>();
});

OUTPUTS

!!!!! RUNNING WITH LAMAR !!!!!
Using 100 degrees of parallelism and generating 1000000 guids
<SNIP>A summary of the duplicates</SNIP>
There were 977543 unique guids

[matthawley]

I was digging into this, and I'm highly confident this due to lambda closures leaking memory within GeneratedInstance.cs, in BuildFuncResolver.

@jeremydmiller I noticed that you started implementing the 3 different typed resolvers, any chance of finishing that work off in hopes that it'll also resolve this? I attempted moving the definition / resolver within the lambda, but that really killed performance.

Here's a unit test I created to test this out: https://github.com/matthawley/lamar/commit/1fd186e57b73ca26c3da6e031fd69cd330d71a3b

[nathansoz]

@CodingGorilla you might be interested in this.

[jeremydmiller]

@matthawley What 3 different typed resolvers are you talking about?

[matthawley]

@jeremydmiller The Scoped/Singleton/Transient resolvers. Also, I stubbed out temporary resolvers as classes instead of lambda closures, and it was still failing.

[jeremydmiller]

@matthawley I don't think it's because of the lambdas, and I don't see how you'd get around that anyway. The classes you're talking about are holdovers from when it was all code generated and I'm not sure how they'd be helpful.

[jeremydmiller]

@nathansoz Hey, it'd be much, much easier to deal with this for me if you could instead create a pull request with a failing test that demonstrates the problem.

[matthawley]

@jeremydmiller Created a PR per your request

[jeremydmiller]

@matthawley I don't know that I'm buying this one from your repro steps. I do see a handful of duplicated Guid values, but when I tracked the number of unique ScopedWidget objects created, it comes up exactly as expected. I don't have anything for you here, other than the Guid behavior doesn't seen to be perfect.

[matthawley]

@jeremydmiller It's possible that we're having guid collision but I would expect to see the same behavior if using raw service collection and other containers. Lamar was the only one we tested that had this problem. Unfortunately due to the scope collision issue we had to rip Lamar out as it was causing a significant number of conflicts under moderate amount of load in production code. Since removing it, we've not had a single collision.

[matthawley]

Also I should note that we're using guide here as purely an example. Our production code is using a scoped object to maintain state for a user within that request. What we were seeing with logging and with metrics, is that objects were being shared and rejecting requests as the object didn't belong to that user.

[jeremydmiller]

@matthawley I wrote the test to use Interlocked.Increment() to test how many objects were being created and it was dead on. Dunno what to tell you.

I'm closing this.

[matthawley]

Unfortunately, I guess that means we'll never move back considering we exhibited the behavior consistently. I suppose the basics is all we need.