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.

Sign up for GitHub

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

Jump to bottom

Fixes race condition on captive scoped services #53325

Merged
merged 8 commits into from
Jun 10, 2021
Merged

Fixes race condition on captive scoped services #53325

merged 8 commits into from
Jun 10, 2021

Conversation

maryamariyan
Copy link
Member

@maryamariyan maryamariyan commented May 27, 2021 •
edited
Loading

Fixes: #52863

TODO:

  • Consider changing IL generated code instead

@ghost
Copy link

ghost commented May 27, 2021

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

Issue Details

Fixes: #52221

TODO:

  • Consider changing IL generated code instead
Author: maryamariyan
Assignees: -
Labels:

area-Extensions-DependencyInjection
Milestone: -

@gfoidl
Copy link
Member

gfoidl commented May 27, 2021

@maryamariyan I think you have linked the wrong issue. Should be #52863?

@maryamariyan
Copy link
Member Author

Updated IL generation logic and applied feedback.

The Expression tree side will need to be changed as well, that would be run for .NET Core 2.1.

davidfowl
davidfowl reviewed Jun 3, 2021
View reviewed changes
...s/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs
context.Generator.Emit(OpCodes.Brfalse_S, defaultLabel);

context.Generator.Emit(OpCodes.Call, CallSiteRuntimeResolverInstanceField);
AddConstant(context, callSite);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice change. @pakrym but we're paying for bounds checks!! 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can change AddConstant to produce unsafe code with fixed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think it matters though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is locking and has a try finally so I'd be surprised if it mattered as well.

We can change AddConstant to produce unsafe code with fixed.

Unsafe.Add

@maryamariyan maryamariyan marked this pull request as ready for review June 5, 2021 22:26
@maryamariyan
Copy link
Member Author

Fixed the Expression Resolver Builder portion as well. Marked PR as ready for review.

@pakrym
Copy link
Contributor

pakrym commented Jun 7, 2021

@maryamariyan please make sure modified tets still reproduce the original issue.

@pakrym
Copy link
Contributor

pakrym commented Jun 7, 2021

Can you please add the test from the original issue?

davidfowl
davidfowl reviewed Jun 7, 2021
View reviewed changes
...ies/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs Outdated
@@ -1119,11 +1119,9 @@ private void ScopedServiceResolvedFromSingletonAfterCompilation2(ServiceProvider
var scope = sp.CreateScope();
for (int i = 0; i < 50; i++)
{
scope.ServiceProvider.GetRequiredService<A>();
Assert.Same(sp.GetRequiredService<IFakeOpenGenericService<A>>().Value, sp.GetRequiredService<A>());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scope is supposed to be used here no?

@maryamariyan
Copy link
Member Author

maryamariyan commented Jun 7, 2021 •
edited
Loading

Can you please add the test from the original issue?

@pakrym the test from comment #52863 (comment) is included in the PR.

The scope is supposed to be used here no?

I confirmed that we get a repro with the unit tests as-is without the fix.

davidfowl
davidfowl reviewed Jun 7, 2021
View reviewed changes
...ies/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs
@@ -11,7 +11,7 @@ namespace Microsoft.Extensions.DependencyInjection.ServiceLookup
internal sealed class ServiceProviderEngineScope : IServiceScope, IServiceProvider, IAsyncDisposable, IServiceScopeFactory
{
// For testing only
internal Action<object> _captureDisposableCallback;
internal IList<object> Disposables => _disposables ?? (IList<object>)Array.Empty<object>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removes a pointer 😄. Saving the planet.

@maryamariyan
Copy link
Member Author

maryamariyan commented Jun 8, 2021 •
edited
Loading

Test failure:

Microsoft.Extensions.Caching.Memory.MemoryCacheSetAndRemoveTests.AddAndReplaceEntries_AreThreadSafe

Assert.Equal() Failure
Expected: 3
Actual:   7

On Libraries Test Run release coreclr Linux x64 Debug Didn't repro locally. Rerunning to check if intermittent.

UPDATE: CI is green

davidfowl
davidfowl approved these changes Jun 9, 2021
View reviewed changes
Copy link
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see how this affects the performance of scoped service resolution.

@davidfowl
Copy link
Member

I think we can also remove these checks in the dynamic resolver:

runtime/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/DynamicServiceProviderEngine.cs

Line 26 in 8671908

if (scope.IsRootScope && callSite.Value != null)

and

runtime/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/DynamicServiceProviderEngine.cs

Line 39 in 8671908

if (scope.IsRootScope && callSite.Value != null)

@maryamariyan maryamariyan merged commit 7b3564a into dotnet:main Jun 10, 2021
@DrewScoggins DrewScoggins mentioned this pull request Jun 15, 2021
Open
@eerhardt eerhardt mentioned this pull request Jun 17, 2021
Closed
@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.
Reviewers

@eerhardt eerhardt eerhardt left review comments

@pakrym pakrym pakrym left review comments

@davidfowl davidfowl davidfowl approved these changes

Assignees
No one assigned
Labels
area-Extensions-DependencyInjection
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Race in scoped services promoted to singleton caching
5 participants
@maryamariyan @gfoidl @pakrym @davidfowl @eerhardt