Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Race in scoped services promoted to singleton caching #52863

Closed
pakrym opened this issue May 17, 2021 · 3 comments · Fixed by #53325
Closed

Race in scoped services promoted to singleton caching #52863

pakrym opened this issue May 17, 2021 · 3 comments · Fixed by #53325

Comments

@pakrym
Copy link
Contributor

pakrym commented May 17, 2021

The #52484 attempted to fix the race where CallSiteRuntimeResolver.Instance.Resolve and compiled service accessors have a different source of truth for a scoped services promoted to singletons caching location.

The former presumes the value is cached in callSite.Value the latter in ResolvedServices dictionary on the scope.

The problem with the fix is that it only tightened the race for directly resolved services but did nothing for indirectly-resolved ones.

For the following chain:

A(Singleton)->B(Scoped)

When A is resolved B is also resolved and cached into the callSite.Value. When a direct compiled accessor for B is executed on the root scope it would bypass the logic in the fix and would use the ResolvedService collection, skipping callSite.Value entirely.

@ghost
Copy link

ghost commented May 17, 2021

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

Issue Details

The #52484 attempted to fix the race where CallSiteRuntimeResolver.Instance.Resolve and compiled service accessors have a different source of truth for a scoped services promoted to singletons caching location.

The former presumes the value is cached in callSite.Value the latter in ResolvedServices dictionary on the scope.

The problem with the fix is that it only tightened the race for directly resolved services but did nothing for indirectly-resolved ones.

For the following chain:

A(Singleton)->B(Scoped)

When A is resolved B is also resolved and cached into the callSite.Value. When a direct compiled accessor for B is executed on the root scope it would bypass the logic in the fix and would use the ResolvedService collection, skipping callSite.Value entirely.

Author: pakrym
Assignees: davidfowl
Labels:

area-Extensions-DependencyInjection, bug

Milestone: -

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label May 17, 2021
@pakrym
Copy link
Contributor Author

pakrym commented May 17, 2021

I think this test illustrates the problem:

        [Fact]
        public void ScopedServiceResolvedFromSingletonAfterCompilation2()
        {
            ServiceProvider sp = new ServiceCollection()
                                .AddScoped<A>()
                                .AddSingleton<IFakeOpenGenericService<A>, FakeOpenGenericService<A>>()
                                .BuildServiceProvider();

            var scope = sp.CreateScope();
            for (int i = 0; i < 10; i++)
            {
                scope.ServiceProvider.GetRequiredService<A>();
                Thread.Sleep(10); // Give the background thread time to compile
            }

            Assert.Same(sp.GetRequiredService<IFakeOpenGenericService<A>>().Value, sp.GetRequiredService<A>());
        }

IFakeOpenGenericService<A> would promote A to the singleton so both IFakeOpenGenericService<A>.Value and A resolved from root should be the same.

@davidfowl
Copy link
Member

cc @maryamariyan @eerhardt

@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label May 17, 2021
@maryamariyan maryamariyan added this to the 6.0.0 milestone May 17, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 27, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 10, 2021
@davidfowl davidfowl assigned maryamariyan and unassigned davidfowl Jun 10, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jul 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants