From 556768a8962afb069e733fa1936e3de8efdacc58 Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Wed, 26 May 2021 18:19:01 -0700 Subject: [PATCH 1/8] - Fixes race with captive scoped services Fixes: #52221 --- .../ILEmit/ILEmitResolverBuilder.cs | 7 ++- .../DI.Tests/ServiceProviderContainerTests.cs | 47 +++++++++++++++++++ 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs index 227a2f8c3688fe..a98d33e6e120d4 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs @@ -52,7 +52,10 @@ public ILEmitResolverBuilder(ServiceProvider serviceProvider) public Func Build(ServiceCallSite callSite) { - return BuildType(callSite).Lambda; + Func lambda = BuildType(callSite).Lambda; + + return (scope) => scope.IsRootScope ? + _runtimeResolver.Resolve(callSite, scope) : lambda(scope); } private GeneratedMethod BuildType(ServiceCallSite callSite) @@ -99,7 +102,7 @@ private GeneratedMethod BuildTypeNoCache(ServiceCallSite callSite) "ResolveService", MethodAttributes.Public | MethodAttributes.Static, CallingConventions.Standard, typeof(object), new[] { typeof(ILEmitResolverBuilderRuntimeContext), typeof(ServiceProviderEngineScope) }); - GenerateMethodBody(callSite, method.GetILGenerator(), info); + GenerateMethodBody(callSite, method.GetILGenerator()); type.CreateTypeInfo(); assembly.Save(assemblyName + ".dll"); #endif diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs index b0c09d3bd724b2..0b0ccb6509a3db 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs @@ -1102,6 +1102,45 @@ public void ScopedServiceResolvedFromSingletonAfterCompilation() Thread.Sleep(10); // Give the background thread time to compile } } + + [Fact] + public void ScopedServiceResolvedFromSingletonAfterCompilation2() + { + ServiceProvider sp = new ServiceCollection() + .AddScoped() + .AddSingleton, FakeOpenGenericService>() + .BuildServiceProvider(); + + var scope = sp.CreateScope(); + for (int i = 0; i < 50; i++) + { + scope.ServiceProvider.GetRequiredService(); + Thread.Sleep(10); // Give the background thread time to compile + } + + Assert.Same(sp.GetRequiredService>().Value, sp.GetRequiredService()); + } + + [Fact] + public void ScopedServiceResolvedFromSingletonAfterCompilation3() + { + // Singleton IFakeX -> Scoped A -> Scoped Aa + ServiceProvider sp = new ServiceCollection() + .AddScoped() + .AddScoped() + .AddSingleton, FakeOpenGenericService>() + .BuildServiceProvider(); + + var scope = sp.CreateScope(); + for (int i = 0; i < 50; i++) + { + Assert.Same(sp.GetRequiredService>().Value.PropertyA, sp.GetRequiredService()); + Thread.Sleep(10); // Give the background thread time to compile + } + + Assert.Same(sp.GetRequiredService>().Value.PropertyA, sp.GetRequiredService()); + } + private async Task ResolveUniqueServicesConcurrently() { @@ -1150,5 +1189,13 @@ private class G { } private class H { } private class I { } private class J { } + private class Aa + { + public Aa(A a) + { + PropertyA = a; + } + public A PropertyA { get; } + } } } From c8e427beee69bb8a7cbad169c000f6904d9f34a3 Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Thu, 27 May 2021 20:28:42 -0700 Subject: [PATCH 2/8] Fix compile after merge --- .../src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs | 3 ++- .../tests/DI.Tests/ServiceProviderContainerTests.cs | 1 - 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs index a98d33e6e120d4..85dcde1c5a783e 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs @@ -6,6 +6,7 @@ using System.Collections.Generic; using System.Reflection; using System.Reflection.Emit; +using Microsoft.Extensions.DependencyInjection.ServiceLookup; namespace Microsoft.Extensions.DependencyInjection.ServiceLookup { @@ -55,7 +56,7 @@ public Func Build(ServiceCallSite callSite) Func lambda = BuildType(callSite).Lambda; return (scope) => scope.IsRootScope ? - _runtimeResolver.Resolve(callSite, scope) : lambda(scope); + CallSiteRuntimeResolver.Instance.Resolve(callSite, scope) : lambda(scope); } private GeneratedMethod BuildType(ServiceCallSite callSite) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs index 0b0ccb6509a3db..65f7a9c22c0365 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs @@ -1141,7 +1141,6 @@ public void ScopedServiceResolvedFromSingletonAfterCompilation3() Assert.Same(sp.GetRequiredService>().Value.PropertyA, sp.GetRequiredService()); } - private async Task ResolveUniqueServicesConcurrently() { var types = new Type[] From 0edaee45b2aa97d446b9990f8f08cd184995912f Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Thu, 3 Jun 2021 11:56:27 -0700 Subject: [PATCH 3/8] Fix IL generated code --- .../ILEmit/ILEmitResolverBuilder.cs | 26 +++++++++++++++++++ .../DI.Tests/ServiceProviderContainerTests.cs | 6 ++--- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs index 85dcde1c5a783e..51a5e329122928 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs @@ -18,6 +18,15 @@ internal sealed class ILEmitResolverBuilder : CallSiteVisitor() .AddSingleton, FakeOpenGenericService>() - .BuildServiceProvider(); + .BuildServiceProvider(ServiceProviderMode.ILEmit); var scope = sp.CreateScope(); for (int i = 0; i < 50; i++) @@ -1129,7 +1129,7 @@ public void ScopedServiceResolvedFromSingletonAfterCompilation3() .AddScoped() .AddScoped() .AddSingleton, FakeOpenGenericService>() - .BuildServiceProvider(); + .BuildServiceProvider(ServiceProviderMode.ILEmit); var scope = sp.CreateScope(); for (int i = 0; i < 50; i++) From 13ce81f2a8382a12f82ed0cfd82734192663aea0 Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Thu, 3 Jun 2021 21:44:13 -0700 Subject: [PATCH 4/8] undo first fix --- .../src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs index 51a5e329122928..a9015e12a99f08 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs @@ -62,10 +62,7 @@ public ILEmitResolverBuilder(ServiceProvider serviceProvider) public Func Build(ServiceCallSite callSite) { - Func lambda = BuildType(callSite).Lambda; - - return (scope) => scope.IsRootScope ? - CallSiteRuntimeResolver.Instance.Resolve(callSite, scope) : lambda(scope); + return BuildType(callSite).Lambda; } private GeneratedMethod BuildType(ServiceCallSite callSite) From 840e943fcf7b10d1a488488cd43127e5605c1e36 Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Sat, 5 Jun 2021 15:24:11 -0700 Subject: [PATCH 5/8] Fix race condition in Expression Resolver Builder --- .../Expressions/ExpressionResolverBuilder.cs | 39 ++++++++++++++++--- .../DI.Tests/ServiceProviderContainerTests.cs | 11 ++++-- 2 files changed, 42 insertions(+), 8 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/Expressions/ExpressionResolverBuilder.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/Expressions/ExpressionResolverBuilder.cs index 3fd07268b8424a..54285d666365d8 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/Expressions/ExpressionResolverBuilder.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/Expressions/ExpressionResolverBuilder.cs @@ -17,6 +17,7 @@ internal sealed class ExpressionResolverBuilder : CallSiteVisitor, IServiceProvider>>((a, b) => a.Invoke(b)); internal static readonly MethodInfo CaptureDisposableMethodInfo = GetMethodInfo>((a, b) => a.CaptureDisposable(b)); internal static readonly MethodInfo TryGetValueMethodInfo = GetMethodInfo, ServiceCacheKey, object, bool>>((a, b, c) => a.TryGetValue(b, out c)); + internal static readonly MethodInfo ResolveCallSiteAndScopeMethodInfo = GetMethodInfo>((a, b, c) => a.Resolve(b, c)); internal static readonly MethodInfo AddMethodInfo = GetMethodInfo, ServiceCacheKey, object>>((a, b, c) => a.Add(b, c)); internal static readonly MethodInfo MonitorEnterMethodInfo = GetMethodInfo>((lockObj, lockTaken) => Monitor.Enter(lockObj, ref lockTaken)); internal static readonly MethodInfo MonitorExitMethodInfo = GetMethodInfo>(lockObj => Monitor.Exit(lockObj)); @@ -27,12 +28,19 @@ internal sealed class ExpressionResolverBuilder : CallSiteVisitor), ScopeParameter.Name + "resolvedServices"); private static readonly ParameterExpression Sync = Expression.Variable(typeof(object), ScopeParameter.Name + "sync"); + private static readonly ParameterExpression IsRootScope = Expression.Variable(typeof(bool), ScopeParameter.Name + "isRootScope"); private static readonly BinaryExpression ResolvedServicesVariableAssignment = Expression.Assign(ResolvedServices, Expression.Property( ScopeParameter, typeof(ServiceProviderEngineScope).GetProperty(nameof(ServiceProviderEngineScope.ResolvedServices), BindingFlags.Instance | BindingFlags.NonPublic))); + private static readonly BinaryExpression IsRootScopeVariableAssignment = + Expression.Assign(IsRootScope, + Expression.Property( + ScopeParameter, + typeof(ServiceProviderEngineScope).GetProperty(nameof(ServiceProviderEngineScope.IsRootScope), BindingFlags.Instance | BindingFlags.Public))); + private static readonly BinaryExpression SyncVariableAssignment = Expression.Assign(Sync, Expression.Property( @@ -85,7 +93,8 @@ private Expression> BuildExpression(Ser { return Expression.Lambda>( Expression.Block( - new[] { ResolvedServices, Sync }, + new[] { IsRootScope, ResolvedServices, Sync }, + IsRootScopeVariableAssignment, ResolvedServicesVariableAssignment, SyncVariableAssignment, BuildScopedExpression(callSite)), @@ -203,6 +212,20 @@ protected override Expression VisitScopeCache(ServiceCallSite callSite, object c // Move off the main stack private Expression BuildScopedExpression(ServiceCallSite callSite) { + ConstantExpression callSiteExpression = Expression.Constant( + callSite, + typeof(ServiceCallSite)); + + ConstantExpression resolverExpression = Expression.Constant( + CallSiteRuntimeResolver.Instance, + typeof(CallSiteRuntimeResolver)); + + MethodCallExpression resolveRootScopeExpression = Expression.Call( + resolverExpression, + ResolveCallSiteAndScopeMethodInfo, + callSiteExpression, + ScopeParameter); + ConstantExpression keyExpression = Expression.Constant( callSite.Cache.Key, typeof(ServiceCacheKey)); @@ -211,6 +234,8 @@ private Expression BuildScopedExpression(ServiceCallSite callSite) ParameterExpression resolvedServices = ResolvedServices; + ParameterExpression isRootScope = IsRootScope; + MethodCallExpression tryGetValueExpression = Expression.Call( resolvedServices, TryGetValueMethodInfo, @@ -254,10 +279,14 @@ private Expression BuildScopedExpression(ServiceCallSite callSite) BlockExpression tryBody = Expression.Block(monitorEnter, blockExpression); ConditionalExpression finallyBody = Expression.IfThen(lockWasTaken, monitorExit); - return Expression.Block( - typeof(object), - new[] { lockWasTaken }, - Expression.TryFinally(tryBody, finallyBody)); + return Expression.Condition( + isRootScope, + resolveRootScopeExpression, + Expression.Block( + typeof(object), + new[] { lockWasTaken }, + Expression.TryFinally(tryBody, finallyBody)) + ); } private static MethodInfo GetMethodInfo(Expression expr) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs index f4488a944a21c7..6482b47fb5d055 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs @@ -1103,13 +1103,18 @@ public void ScopedServiceResolvedFromSingletonAfterCompilation() } } - [Fact] - public void ScopedServiceResolvedFromSingletonAfterCompilation2() + [Theory] + [InlineData(ServiceProviderMode.Default)] + [InlineData(ServiceProviderMode.Dynamic)] + [InlineData(ServiceProviderMode.Runtime)] + [InlineData(ServiceProviderMode.Expressions)] + [InlineData(ServiceProviderMode.ILEmit)] + private void ScopedServiceResolvedFromSingletonAfterCompilation2(ServiceProviderMode mode) { ServiceProvider sp = new ServiceCollection() .AddScoped() .AddSingleton, FakeOpenGenericService>() - .BuildServiceProvider(ServiceProviderMode.ILEmit); + .BuildServiceProvider(mode); var scope = sp.CreateScope(); for (int i = 0; i < 50; i++) From 40924b92ce2053c1ee9bf228ecbf6c818714d002 Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Mon, 7 Jun 2021 08:37:30 -0700 Subject: [PATCH 6/8] Apply PR feedback --- .../Expressions/ExpressionResolverBuilder.cs | 30 ++++++++----------- .../DI.Tests/ServiceProviderContainerTests.cs | 6 +--- 2 files changed, 13 insertions(+), 23 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/Expressions/ExpressionResolverBuilder.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/Expressions/ExpressionResolverBuilder.cs index 54285d666365d8..562f31397740a5 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/Expressions/ExpressionResolverBuilder.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/Expressions/ExpressionResolverBuilder.cs @@ -35,12 +35,6 @@ internal sealed class ExpressionResolverBuilder : CallSiteVisitor> _scopeResolverCache; @@ -94,7 +92,6 @@ private Expression> BuildExpression(Ser return Expression.Lambda>( Expression.Block( new[] { IsRootScope, ResolvedServices, Sync }, - IsRootScopeVariableAssignment, ResolvedServicesVariableAssignment, SyncVariableAssignment, BuildScopedExpression(callSite)), @@ -216,12 +213,8 @@ private Expression BuildScopedExpression(ServiceCallSite callSite) callSite, typeof(ServiceCallSite)); - ConstantExpression resolverExpression = Expression.Constant( - CallSiteRuntimeResolver.Instance, - typeof(CallSiteRuntimeResolver)); - MethodCallExpression resolveRootScopeExpression = Expression.Call( - resolverExpression, + CallSiteRuntimeResolverInstanceExpression, ResolveCallSiteAndScopeMethodInfo, callSiteExpression, ScopeParameter); @@ -234,8 +227,6 @@ private Expression BuildScopedExpression(ServiceCallSite callSite) ParameterExpression resolvedServices = ResolvedServices; - ParameterExpression isRootScope = IsRootScope; - MethodCallExpression tryGetValueExpression = Expression.Call( resolvedServices, TryGetValueMethodInfo, @@ -280,12 +271,15 @@ private Expression BuildScopedExpression(ServiceCallSite callSite) ConditionalExpression finallyBody = Expression.IfThen(lockWasTaken, monitorExit); return Expression.Condition( - isRootScope, + Expression.Property( + ScopeParameter, + typeof(ServiceProviderEngineScope) + .GetProperty(nameof(ServiceProviderEngineScope.IsRootScope), BindingFlags.Instance | BindingFlags.Public)), resolveRootScopeExpression, Expression.Block( - typeof(object), - new[] { lockWasTaken }, - Expression.TryFinally(tryBody, finallyBody)) + typeof(object), + new[] { lockWasTaken }, + Expression.TryFinally(tryBody, finallyBody)) ); } diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs index 6482b47fb5d055..7e3b6f251828fd 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs @@ -1119,11 +1119,9 @@ private void ScopedServiceResolvedFromSingletonAfterCompilation2(ServiceProvider var scope = sp.CreateScope(); for (int i = 0; i < 50; i++) { - scope.ServiceProvider.GetRequiredService(); + Assert.Same(sp.GetRequiredService>().Value, sp.GetRequiredService()); Thread.Sleep(10); // Give the background thread time to compile } - - Assert.Same(sp.GetRequiredService>().Value, sp.GetRequiredService()); } [Fact] @@ -1142,8 +1140,6 @@ public void ScopedServiceResolvedFromSingletonAfterCompilation3() Assert.Same(sp.GetRequiredService>().Value.PropertyA, sp.GetRequiredService()); Thread.Sleep(10); // Give the background thread time to compile } - - Assert.Same(sp.GetRequiredService>().Value.PropertyA, sp.GetRequiredService()); } private async Task ResolveUniqueServicesConcurrently() From 45fff13e8042e195892ea2a4d872fccb20c4cfca Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Mon, 7 Jun 2021 15:52:49 -0700 Subject: [PATCH 7/8] Update tests / Code cleanup --- .../Expressions/ExpressionResolverBuilder.cs | 3 +- .../ServiceProviderEngineScope.cs | 4 +-- .../tests/DI.Tests/CallSiteTests.cs | 28 ++++++------------- .../DI.Tests/ServiceProviderContainerTests.cs | 19 +++++++++---- 4 files changed, 24 insertions(+), 30 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/Expressions/ExpressionResolverBuilder.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/Expressions/ExpressionResolverBuilder.cs index 562f31397740a5..f0a2d4c712bc9c 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/Expressions/ExpressionResolverBuilder.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/Expressions/ExpressionResolverBuilder.cs @@ -28,7 +28,6 @@ internal sealed class ExpressionResolverBuilder : CallSiteVisitor), ScopeParameter.Name + "resolvedServices"); private static readonly ParameterExpression Sync = Expression.Variable(typeof(object), ScopeParameter.Name + "sync"); - private static readonly ParameterExpression IsRootScope = Expression.Variable(typeof(bool), ScopeParameter.Name + "isRootScope"); private static readonly BinaryExpression ResolvedServicesVariableAssignment = Expression.Assign(ResolvedServices, Expression.Property( @@ -91,7 +90,7 @@ private Expression> BuildExpression(Ser { return Expression.Lambda>( Expression.Block( - new[] { IsRootScope, ResolvedServices, Sync }, + new[] { ResolvedServices, Sync }, ResolvedServicesVariableAssignment, SyncVariableAssignment, BuildScopedExpression(callSite)), diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs index fce0f12152a808..06498ff98baac8 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs +++ b/src/libraries/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 _captureDisposableCallback; + internal IList Disposables => _disposables ?? (IList)Array.Empty(); private bool _disposed; private List _disposables; @@ -49,8 +49,6 @@ public object GetService(Type serviceType) internal object CaptureDisposable(object service) { - _captureDisposableCallback?.Invoke(service); - if (ReferenceEquals(this, service) || !(service is IDisposable || service is IAsyncDisposable)) { return service; diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/CallSiteTests.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/CallSiteTests.cs index c79594a92eb8e3..6c121a5c417ebb 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/CallSiteTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/CallSiteTests.cs @@ -135,16 +135,13 @@ public void BuildExpressionAddsDisposableCaptureForDisposableServices(ServiceLif var disposables = new List(); var provider = new ServiceProvider(descriptors, ServiceProviderOptions.Default); - provider.Root._captureDisposableCallback = obj => - { - disposables.Add(obj); - }; + var callSite = provider.CallSiteFactory.GetCallSite(typeof(ServiceC), new CallSiteChain()); var compiledCallSite = CompileCallSite(callSite, provider); var serviceC = (DisposableServiceC)compiledCallSite(provider.Root); - Assert.Equal(3, disposables.Count); + Assert.Equal(3, provider.Root.Disposables.Count); } [Theory] @@ -161,16 +158,13 @@ public void BuildExpressionAddsDisposableCaptureForDisposableFactoryServices(Ser var disposables = new List(); var provider = new ServiceProvider(descriptors, ServiceProviderOptions.Default); - provider.Root._captureDisposableCallback = obj => - { - disposables.Add(obj); - }; + var callSite = provider.CallSiteFactory.GetCallSite(typeof(ServiceC), new CallSiteChain()); var compiledCallSite = CompileCallSite(callSite, provider); var serviceC = (DisposableServiceC)compiledCallSite(provider.Root); - Assert.Equal(3, disposables.Count); + Assert.Equal(3, provider.Root.Disposables.Count); } [Theory] @@ -190,16 +184,13 @@ public void BuildExpressionElidesDisposableCaptureForNonDisposableServices(Servi var disposables = new List(); var provider = new ServiceProvider(descriptors, ServiceProviderOptions.Default); - provider.Root._captureDisposableCallback = obj => - { - disposables.Add(obj); - }; + var callSite = provider.CallSiteFactory.GetCallSite(typeof(ServiceC), new CallSiteChain()); var compiledCallSite = CompileCallSite(callSite, provider); var serviceC = (ServiceC)compiledCallSite(provider.Root); - Assert.Empty(disposables); + Assert.Empty(provider.Root.Disposables); } [Theory] @@ -215,16 +206,13 @@ public void BuildExpressionElidesDisposableCaptureForEnumerableServices(ServiceL var disposables = new List(); var provider = new ServiceProvider(descriptors, ServiceProviderOptions.Default); - provider.Root._captureDisposableCallback = obj => - { - disposables.Add(obj); - }; + var callSite = provider.CallSiteFactory.GetCallSite(typeof(ServiceD), new CallSiteChain()); var compiledCallSite = CompileCallSite(callSite, provider); var serviceD = (ServiceD)compiledCallSite(provider.Root); - Assert.Empty(disposables); + Assert.Empty(provider.Root.Disposables); } [Fact] diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs index 7e3b6f251828fd..22fd778450fc08 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs @@ -1119,27 +1119,36 @@ private void ScopedServiceResolvedFromSingletonAfterCompilation2(ServiceProvider var scope = sp.CreateScope(); for (int i = 0; i < 50; i++) { - Assert.Same(sp.GetRequiredService>().Value, sp.GetRequiredService()); + scope.ServiceProvider.GetRequiredService(); Thread.Sleep(10); // Give the background thread time to compile } + + Assert.Same(sp.GetRequiredService>().Value, sp.GetRequiredService()); } - [Fact] - public void ScopedServiceResolvedFromSingletonAfterCompilation3() + [Theory] + [InlineData(ServiceProviderMode.Default)] + [InlineData(ServiceProviderMode.Dynamic)] + [InlineData(ServiceProviderMode.Runtime)] + [InlineData(ServiceProviderMode.Expressions)] + [InlineData(ServiceProviderMode.ILEmit)] + private void ScopedServiceResolvedFromSingletonAfterCompilation3(ServiceProviderMode mode) { // Singleton IFakeX -> Scoped A -> Scoped Aa ServiceProvider sp = new ServiceCollection() .AddScoped() .AddScoped() .AddSingleton, FakeOpenGenericService>() - .BuildServiceProvider(ServiceProviderMode.ILEmit); + .BuildServiceProvider(mode); var scope = sp.CreateScope(); for (int i = 0; i < 50; i++) { - Assert.Same(sp.GetRequiredService>().Value.PropertyA, sp.GetRequiredService()); + scope.ServiceProvider.GetRequiredService(); Thread.Sleep(10); // Give the background thread time to compile } + + Assert.Same(sp.GetRequiredService>().Value.PropertyA, sp.GetRequiredService()); } private async Task ResolveUniqueServicesConcurrently() From 6557644a8cf0184a512f8fd44b841d5130874cb1 Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Wed, 9 Jun 2021 12:54:42 -0700 Subject: [PATCH 8/8] Code cleanup --- .../ServiceLookup/DynamicServiceProviderEngine.cs | 15 --------------- .../Expressions/ExpressionResolverBuilder.cs | 3 +++ 2 files changed, 3 insertions(+), 15 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/DynamicServiceProviderEngine.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/DynamicServiceProviderEngine.cs index 23914ec50c73bf..ee124f31be8eff 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/DynamicServiceProviderEngine.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/DynamicServiceProviderEngine.cs @@ -20,27 +20,12 @@ public override Func RealizeService(ServiceC int callCount = 0; return scope => { - // We want to directly use the callsite value if it's set and the scope is the root scope. - // We've already called into the RuntimeResolver and pre-computed any singletons or root scope - // Avoid the compilation for singletons (or promoted singletons) - if (scope.IsRootScope && callSite.Value != null) - { - return callSite.Value; - } - // Resolve the result before we increment the call count, this ensures that singletons // won't cause any side effects during the compilation of the resolve function. var result = CallSiteRuntimeResolver.Instance.Resolve(callSite, scope); if (Interlocked.Increment(ref callCount) == 2) { - // This second check is to avoid the race where we end up kicking off a background thread - // if multiple calls to GetService race and resolve the values for singletons before the initial check above. - if (scope.IsRootScope && callSite.Value != null) - { - return callSite.Value; - } - // Don't capture the ExecutionContext when forking to build the compiled version of the // resolve function _ = ThreadPool.UnsafeQueueUserWorkItem(_ => diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/Expressions/ExpressionResolverBuilder.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/Expressions/ExpressionResolverBuilder.cs index f0a2d4c712bc9c..07ff50a44d6711 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/Expressions/ExpressionResolverBuilder.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/Expressions/ExpressionResolverBuilder.cs @@ -212,6 +212,9 @@ private Expression BuildScopedExpression(ServiceCallSite callSite) callSite, typeof(ServiceCallSite)); + // We want to directly use the callsite value if it's set and the scope is the root scope. + // We've already called into the RuntimeResolver and pre-computed any singletons or root scope + // Avoid the compilation for singletons (or promoted singletons) MethodCallExpression resolveRootScopeExpression = Expression.Call( CallSiteRuntimeResolverInstanceExpression, ResolveCallSiteAndScopeMethodInfo,