Fixes DI deadlock when resolving singletons

[maryamariyan]

Introducing per singleton lock for the code path which resolves singletons.

The repro test case in #35986 (comment) gets fixed with this change.

Fixes: #35986

[ghost]

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Introducing per singleton lock for the code path which resolves singletons.

The repro test case in #35986 (comment) gets fixed with this change.

TODO:

• slight code cleanup
• consider preparing a simpler test case.
• then switch out from draft PR.

Fixes: #35986

Author:	maryamariyan
Assignees:	maryamariyan
Labels:	area-Extensions-DependencyInjection
Milestone:	-

[davidfowl]

I think you should try to use the call site itself as the lock for singletons

[eerhardt]

Since this is pretty core logic that affects a lot of ASP.NET, is it possible to run all of ASP.NET's tests on this change before merging? How easy would that be @davidfowl @Tratcher @pranavkm ?

[pranavkm]

How easy would that be

You could create a draft PR pointing to a build of this package and let the public CI do it's job. That shouldn't be too hard and could instill some confidence prior to merging this change.

[maryamariyan]

You could create a draft PR pointing to a build of this package and let the public CI do it's job. That shouldn't be too hard and could instill some confidence prior to merging this change.

As part of this exercise I downloaded the DI package from this PR (postfixed with -ci). But it's not straightforward to test the package against aspnetcore CI since they also need the package to point to a nuget source somewhere the CI can access. 

Testing the DI package against all aspnetcore projects locally is a bit of a hassle too. I'm working with @pranavkm on this and perhaps worst case we could merge this, and test off the published package from merge, and if problems occur then revert accordingly.

[maryamariyan]

as per @eerhardt 's recommendation updated the tests to use two ManualResetEvents so we can enforce the expected order of execution we want to test for deadlock.

[maryamariyan]

Added an aspnetcore PR (dotnet/aspnetcore#29437) which triggers their tests on changes made here.

UPDATE on status of the aspnetcore PR:

• DI related tests passed
• All of the failures are template tests (performing restore) and runtime pack tests (just complaining on the way the PR includes the nupkg packages).

[eerhardt]

LGTM.

The tests changes look good, and are deterministic now. Thanks for the good work here.

[davidfowl]

If github supported gifs, I would post a happy dance! This is a long standing issue with the DI container that has crippled many services.

[davidfowl]

@pakrym notice that this check here is problematic and can no longer be used since we're taking a unique lock per singleton. We don't want to skip it here anymore.

[maryamariyan]

I see. I'll fix both comments with test. Thanks.

UPDATE:

Added PR #47310 to address these comments