From 000fff9cbbf313d4be4e44763eb9ca7ae7451bd1 Mon Sep 17 00:00:00 2001 From: Pavel Krymets Date: Fri, 11 Nov 2016 10:28:48 -0800 Subject: [PATCH 1/2] Dispose services in reverse creation order --- .../DependencyInjectionSpecificationTests.cs | 25 +++++++++ .../Fakes/FakeService.cs | 52 +++++++++++++++++++ .../CallSiteExpressionBuilder.cs | 22 +++++--- .../ServiceLookup/CallSiteRuntimeResolver.cs | 1 + .../ServiceProvider.cs | 11 +--- 5 files changed, 95 insertions(+), 16 deletions(-) diff --git a/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/DependencyInjectionSpecificationTests.cs b/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/DependencyInjectionSpecificationTests.cs index aef2cdbc..1cbc3d0c 100644 --- a/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/DependencyInjectionSpecificationTests.cs +++ b/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/DependencyInjectionSpecificationTests.cs @@ -655,5 +655,30 @@ public void ServiceContainerPicksConstructorWithLongestMatches( Assert.Same(expected.MultipleService, actual.MultipleService); Assert.Same(expected.ScopedService, actual.ScopedService); } + + [Fact] + public void DisposesInReverseOrderOfCreation() + { + // Arrange + var serviceCollection = new TestServiceCollection(); + serviceCollection.AddSingleton(); + serviceCollection.AddSingleton(); + serviceCollection.AddSingleton(); + serviceCollection.AddSingleton(); + serviceCollection.AddSingleton(); + serviceCollection.AddSingleton(); + var serviceProvider = CreateServiceProvider(serviceCollection); + + var callback = serviceProvider.GetService(); + var outer = serviceProvider.GetService(); + + // Act + ((IDisposable)serviceProvider).Dispose(); + + // Assert + Assert.Equal(outer, callback.Disposed[0]); + Assert.Equal(outer.MultipleServices.Reverse(), callback.Disposed.Skip(1).Take(3).OfType()); + Assert.Equal(outer.SingleService, callback.Disposed[4]); + } } } diff --git a/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Fakes/FakeService.cs b/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Fakes/FakeService.cs index 1bedc0a2..13e1eb3d 100644 --- a/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Fakes/FakeService.cs +++ b/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Fakes/FakeService.cs @@ -2,9 +2,61 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Collections.Generic; namespace Microsoft.Extensions.DependencyInjection.Specification.Fakes { + public class FakeDisposeCallback + { + public List Disposed { get; } = new List(); + } + + public class FakeDisposableCallbackService: IDisposable + { + private static int _globalId; + private readonly int _id; + private readonly FakeDisposeCallback _callback; + + public FakeDisposableCallbackService(FakeDisposeCallback callback) + { + _id = _globalId++; + _callback = callback; + } + + public void Dispose() + { + _callback.Disposed.Add(this); + } + + public override string ToString() + { + return _id.ToString(); + } + } + + public class FakeDisposableCallbackOuterService : FakeDisposableCallbackService, IFakeOuterService + { + public FakeDisposableCallbackOuterService( + IFakeService singleService, + IEnumerable multipleServices, + FakeDisposeCallback callback) : base(callback) + { + SingleService = singleService; + MultipleServices = multipleServices; + } + + public IFakeService SingleService { get; } + public IEnumerable MultipleServices { get; } + } + + + public class FakeDisposableCallbackInnerService : FakeDisposableCallbackService, IFakeMultipleService + { + public FakeDisposableCallbackInnerService(FakeDisposeCallback callback) : base(callback) + { + } + } + public class FakeService : IFakeEveryService, IDisposable { public PocoClass Value { get; set; } diff --git a/src/Microsoft.Extensions.DependencyInjection/ServiceLookup/CallSiteExpressionBuilder.cs b/src/Microsoft.Extensions.DependencyInjection/ServiceLookup/CallSiteExpressionBuilder.cs index e8748ef9..21e794a2 100644 --- a/src/Microsoft.Extensions.DependencyInjection/ServiceLookup/CallSiteExpressionBuilder.cs +++ b/src/Microsoft.Extensions.DependencyInjection/ServiceLookup/CallSiteExpressionBuilder.cs @@ -184,7 +184,7 @@ protected override Expression VisitScoped(ScopedCallSite callSite, ParameterExpr callSite.Key, typeof(object)); - var resolvedExpression = Expression.Variable(typeof(object), "resolved"); + var resolvedVariable = Expression.Variable(typeof(object), "resolved"); var resolvedServices = GetResolvedServices(provider); @@ -192,26 +192,34 @@ protected override Expression VisitScoped(ScopedCallSite callSite, ParameterExpr resolvedServices, TryGetValueMethodInfo, keyExpression, - resolvedExpression); + resolvedVariable); var assignExpression = Expression.Assign( - resolvedExpression, VisitCallSite(callSite.ServiceCallSite, provider)); + resolvedVariable, VisitCallSite(callSite.ServiceCallSite, provider)); var addValueExpression = Expression.Call( resolvedServices, AddMethodInfo, keyExpression, - resolvedExpression); + resolvedVariable); + + Expression captureDisposableExpression = Expression.Invoke( + GetCaptureDisposable(provider), + resolvedVariable + ); var blockExpression = Expression.Block( typeof(object), new[] { - resolvedExpression + resolvedVariable }, Expression.IfThen( Expression.Not(tryGetValueExpression), - Expression.Block(assignExpression, addValueExpression)), - resolvedExpression); + Expression.Block( + assignExpression, + addValueExpression, + captureDisposableExpression)), + resolvedVariable); return blockExpression; } diff --git a/src/Microsoft.Extensions.DependencyInjection/ServiceLookup/CallSiteRuntimeResolver.cs b/src/Microsoft.Extensions.DependencyInjection/ServiceLookup/CallSiteRuntimeResolver.cs index 81e9e9ff..b0f8d798 100644 --- a/src/Microsoft.Extensions.DependencyInjection/ServiceLookup/CallSiteRuntimeResolver.cs +++ b/src/Microsoft.Extensions.DependencyInjection/ServiceLookup/CallSiteRuntimeResolver.cs @@ -49,6 +49,7 @@ protected override object VisitScoped(ScopedCallSite scopedCallSite, ServiceProv if (!provider.ResolvedServices.TryGetValue(scopedCallSite.Key, out resolved)) { resolved = VisitCallSite(scopedCallSite.ServiceCallSite, provider); + provider.CaptureDisposable(resolved); provider.ResolvedServices.Add(scopedCallSite.Key, resolved); } } diff --git a/src/Microsoft.Extensions.DependencyInjection/ServiceProvider.cs b/src/Microsoft.Extensions.DependencyInjection/ServiceProvider.cs index f55224db..cfb421a4 100644 --- a/src/Microsoft.Extensions.DependencyInjection/ServiceProvider.cs +++ b/src/Microsoft.Extensions.DependencyInjection/ServiceProvider.cs @@ -169,22 +169,15 @@ public void Dispose() _disposeCalled = true; if (_transientDisposables != null) { - foreach (var disposable in _transientDisposables) + for (int i = _transientDisposables.Count - 1; i >= 0; i--) { + var disposable = _transientDisposables[i]; disposable.Dispose(); } _transientDisposables.Clear(); } - // PERF: We've enumerating the dictionary so that we don't allocate to enumerate. - // .Values allocates a ValueCollection on the heap, enumerating the dictionary allocates - // a struct enumerator - foreach (var entry in ResolvedServices) - { - (entry.Value as IDisposable)?.Dispose(); - } - ResolvedServices.Clear(); } } From 5aec0ad29ff243ad251da2f937c82ec4fcf9997c Mon Sep 17 00:00:00 2001 From: Pavel Krymets Date: Thu, 16 Mar 2017 11:41:34 -0700 Subject: [PATCH 2/2] Add more tests Eliminate redundant CaptureDisposable call from expression trees Split fakes into separate files --- .../DependencyInjectionSpecificationTests.cs | 6 +-- .../FakeDisposableCallbackInnerService.cs | 12 +++++ .../FakeDisposableCallbackOuterService.cs | 22 ++++++++ .../Fakes/FakeDisposableCallbackService.cs | 30 +++++++++++ .../Fakes/FakeDisposeCallback.cs | 12 +++++ .../Fakes/FakeService.cs | 52 ------------------- ...ndencyInjection.Specification.Tests.csproj | 4 ++ .../CallSiteExpressionBuilder.cs | 28 ++++++---- .../ServiceProvider.cs | 2 - .../CallSiteTests.cs | 32 ++++++++---- 10 files changed, 121 insertions(+), 79 deletions(-) create mode 100644 src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Fakes/FakeDisposableCallbackInnerService.cs create mode 100644 src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Fakes/FakeDisposableCallbackOuterService.cs create mode 100644 src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Fakes/FakeDisposableCallbackService.cs create mode 100644 src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Fakes/FakeDisposeCallback.cs diff --git a/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/DependencyInjectionSpecificationTests.cs b/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/DependencyInjectionSpecificationTests.cs index 1cbc3d0c..8901f69b 100644 --- a/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/DependencyInjectionSpecificationTests.cs +++ b/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/DependencyInjectionSpecificationTests.cs @@ -662,10 +662,10 @@ public void DisposesInReverseOrderOfCreation() // Arrange var serviceCollection = new TestServiceCollection(); serviceCollection.AddSingleton(); - serviceCollection.AddSingleton(); - serviceCollection.AddSingleton(); - serviceCollection.AddSingleton(); + serviceCollection.AddTransient(); serviceCollection.AddSingleton(); + serviceCollection.AddScoped(); + serviceCollection.AddTransient(); serviceCollection.AddSingleton(); var serviceProvider = CreateServiceProvider(serviceCollection); diff --git a/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Fakes/FakeDisposableCallbackInnerService.cs b/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Fakes/FakeDisposableCallbackInnerService.cs new file mode 100644 index 00000000..c8581330 --- /dev/null +++ b/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Fakes/FakeDisposableCallbackInnerService.cs @@ -0,0 +1,12 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +namespace Microsoft.Extensions.DependencyInjection.Specification.Fakes +{ + public class FakeDisposableCallbackInnerService : FakeDisposableCallbackService, IFakeMultipleService + { + public FakeDisposableCallbackInnerService(FakeDisposeCallback callback) : base(callback) + { + } + } +} \ No newline at end of file diff --git a/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Fakes/FakeDisposableCallbackOuterService.cs b/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Fakes/FakeDisposableCallbackOuterService.cs new file mode 100644 index 00000000..d400c122 --- /dev/null +++ b/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Fakes/FakeDisposableCallbackOuterService.cs @@ -0,0 +1,22 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.Collections.Generic; + +namespace Microsoft.Extensions.DependencyInjection.Specification.Fakes +{ + public class FakeDisposableCallbackOuterService : FakeDisposableCallbackService, IFakeOuterService + { + public FakeDisposableCallbackOuterService( + IFakeService singleService, + IEnumerable multipleServices, + FakeDisposeCallback callback) : base(callback) + { + SingleService = singleService; + MultipleServices = multipleServices; + } + + public IFakeService SingleService { get; } + public IEnumerable MultipleServices { get; } + } +} \ No newline at end of file diff --git a/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Fakes/FakeDisposableCallbackService.cs b/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Fakes/FakeDisposableCallbackService.cs new file mode 100644 index 00000000..53e09579 --- /dev/null +++ b/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Fakes/FakeDisposableCallbackService.cs @@ -0,0 +1,30 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; + +namespace Microsoft.Extensions.DependencyInjection.Specification.Fakes +{ + public class FakeDisposableCallbackService: IDisposable + { + private static int _globalId; + private readonly int _id; + private readonly FakeDisposeCallback _callback; + + public FakeDisposableCallbackService(FakeDisposeCallback callback) + { + _id = _globalId++; + _callback = callback; + } + + public void Dispose() + { + _callback.Disposed.Add(this); + } + + public override string ToString() + { + return _id.ToString(); + } + } +} \ No newline at end of file diff --git a/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Fakes/FakeDisposeCallback.cs b/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Fakes/FakeDisposeCallback.cs new file mode 100644 index 00000000..4fab8d63 --- /dev/null +++ b/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Fakes/FakeDisposeCallback.cs @@ -0,0 +1,12 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.Collections.Generic; + +namespace Microsoft.Extensions.DependencyInjection.Specification.Fakes +{ + public class FakeDisposeCallback + { + public List Disposed { get; } = new List(); + } +} \ No newline at end of file diff --git a/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Fakes/FakeService.cs b/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Fakes/FakeService.cs index 13e1eb3d..1bedc0a2 100644 --- a/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Fakes/FakeService.cs +++ b/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Fakes/FakeService.cs @@ -2,61 +2,9 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; -using System.Collections.Generic; namespace Microsoft.Extensions.DependencyInjection.Specification.Fakes { - public class FakeDisposeCallback - { - public List Disposed { get; } = new List(); - } - - public class FakeDisposableCallbackService: IDisposable - { - private static int _globalId; - private readonly int _id; - private readonly FakeDisposeCallback _callback; - - public FakeDisposableCallbackService(FakeDisposeCallback callback) - { - _id = _globalId++; - _callback = callback; - } - - public void Dispose() - { - _callback.Disposed.Add(this); - } - - public override string ToString() - { - return _id.ToString(); - } - } - - public class FakeDisposableCallbackOuterService : FakeDisposableCallbackService, IFakeOuterService - { - public FakeDisposableCallbackOuterService( - IFakeService singleService, - IEnumerable multipleServices, - FakeDisposeCallback callback) : base(callback) - { - SingleService = singleService; - MultipleServices = multipleServices; - } - - public IFakeService SingleService { get; } - public IEnumerable MultipleServices { get; } - } - - - public class FakeDisposableCallbackInnerService : FakeDisposableCallbackService, IFakeMultipleService - { - public FakeDisposableCallbackInnerService(FakeDisposeCallback callback) : base(callback) - { - } - } - public class FakeService : IFakeEveryService, IDisposable { public PocoClass Value { get; set; } diff --git a/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Microsoft.Extensions.DependencyInjection.Specification.Tests.csproj b/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Microsoft.Extensions.DependencyInjection.Specification.Tests.csproj index 1b5893e7..90dea721 100644 --- a/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Microsoft.Extensions.DependencyInjection.Specification.Tests.csproj +++ b/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Microsoft.Extensions.DependencyInjection.Specification.Tests.csproj @@ -19,4 +19,8 @@ + + + + diff --git a/src/Microsoft.Extensions.DependencyInjection/ServiceLookup/CallSiteExpressionBuilder.cs b/src/Microsoft.Extensions.DependencyInjection/ServiceLookup/CallSiteExpressionBuilder.cs index 21e794a2..4632cacc 100644 --- a/src/Microsoft.Extensions.DependencyInjection/ServiceLookup/CallSiteExpressionBuilder.cs +++ b/src/Microsoft.Extensions.DependencyInjection/ServiceLookup/CallSiteExpressionBuilder.cs @@ -146,16 +146,24 @@ protected override Expression VisitClosedIEnumerable(ClosedIEnumerableCallSite c protected override Expression VisitTransient(TransientCallSite callSite, ParameterExpression provider) { var implType = callSite.Service.ImplementationType; - // Elide calls to GetCaptureDisposable if the implemenation type isn't disposable + return TryCaptureDisposible( + implType, + provider, + VisitCallSite(callSite.ServiceCallSite, provider)); + } + + private Expression TryCaptureDisposible(Type implType, ParameterExpression provider, Expression service) + { + if (implType != null && !typeof(IDisposable).GetTypeInfo().IsAssignableFrom(implType.GetTypeInfo())) { - return VisitCallSite(callSite.ServiceCallSite, provider); + return service; } return Expression.Invoke(GetCaptureDisposable(provider), - VisitCallSite(callSite.ServiceCallSite, provider)); + service); } protected override Expression VisitConstructor(ConstructorCallSite callSite, ParameterExpression provider) @@ -194,8 +202,12 @@ protected override Expression VisitScoped(ScopedCallSite callSite, ParameterExpr keyExpression, resolvedVariable); + var service = VisitCallSite(callSite.ServiceCallSite, provider); + var captureDisposible = TryCaptureDisposible(callSite.Key.ImplementationType, provider, service); + var assignExpression = Expression.Assign( - resolvedVariable, VisitCallSite(callSite.ServiceCallSite, provider)); + resolvedVariable, + captureDisposible); var addValueExpression = Expression.Call( resolvedServices, @@ -203,11 +215,6 @@ protected override Expression VisitScoped(ScopedCallSite callSite, ParameterExpr keyExpression, resolvedVariable); - Expression captureDisposableExpression = Expression.Invoke( - GetCaptureDisposable(provider), - resolvedVariable - ); - var blockExpression = Expression.Block( typeof(object), new[] { @@ -217,8 +224,7 @@ protected override Expression VisitScoped(ScopedCallSite callSite, ParameterExpr Expression.Not(tryGetValueExpression), Expression.Block( assignExpression, - addValueExpression, - captureDisposableExpression)), + addValueExpression)), resolvedVariable); return blockExpression; diff --git a/src/Microsoft.Extensions.DependencyInjection/ServiceProvider.cs b/src/Microsoft.Extensions.DependencyInjection/ServiceProvider.cs index cfb421a4..39c7b18e 100644 --- a/src/Microsoft.Extensions.DependencyInjection/ServiceProvider.cs +++ b/src/Microsoft.Extensions.DependencyInjection/ServiceProvider.cs @@ -19,7 +19,6 @@ internal class ServiceProvider : IServiceProvider, IDisposable { private readonly CallSiteValidator _callSiteValidator; private readonly ServiceTable _table; - private readonly ServiceProviderOptions _options; private bool _disposeCalled; private List _transientDisposables; @@ -43,7 +42,6 @@ public ServiceProvider(IEnumerable serviceDescriptors, Servic _callSiteValidator = new CallSiteValidator(); } - _options = options; _table = new ServiceTable(serviceDescriptors); _table.Add(typeof(IServiceProvider), new ServiceProviderService()); diff --git a/test/Microsoft.Extensions.DependencyInjection.Tests/CallSiteTests.cs b/test/Microsoft.Extensions.DependencyInjection.Tests/CallSiteTests.cs index 03e1146b..dcd3cae4 100644 --- a/test/Microsoft.Extensions.DependencyInjection.Tests/CallSiteTests.cs +++ b/test/Microsoft.Extensions.DependencyInjection.Tests/CallSiteTests.cs @@ -4,7 +4,6 @@ using System; using System.Collections; using System.Collections.Generic; -using System.Linq.Expressions; using Microsoft.Extensions.DependencyInjection.ServiceLookup; using Microsoft.Extensions.DependencyInjection.Specification.Fakes; using Xunit; @@ -121,12 +120,19 @@ public void BuiltExpressionCanResolveNestedScopedService() Assert.Equal(serviceC, Invoke(callSite, provider)); } - [Fact] - public void BuildExpressionElidesDisposableCaptureForNonDisposableServices() + [Theory] + [InlineData(ServiceLifetime.Scoped)] + [InlineData(ServiceLifetime.Transient)] + // We are not testing singleton here because singleton resolutions always got through + // runtime resolver and there is no sense to eliminating call from there + public void BuildExpressionElidesDisposableCaptureForNonDisposableServices(ServiceLifetime lifetime) { - var descriptors = new ServiceCollection(); - descriptors.AddTransient(); - descriptors.AddTransient(); + IServiceCollection descriptors = new ServiceCollection(); + descriptors.Add(ServiceDescriptor.Describe(typeof(ServiceA), typeof(ServiceA), lifetime)); + descriptors.Add(ServiceDescriptor.Describe(typeof(ServiceB), typeof(ServiceB), lifetime)); + descriptors.Add(ServiceDescriptor.Describe(typeof(ServiceC), typeof(ServiceC), lifetime)); + + descriptors.AddScoped(); descriptors.AddTransient(); var disposables = new List(); @@ -143,12 +149,16 @@ public void BuildExpressionElidesDisposableCaptureForNonDisposableServices() Assert.Equal(0, disposables.Count); } - [Fact] - public void BuildExpressionElidesDisposableCaptureForEnumerableServices() + [Theory] + [InlineData(ServiceLifetime.Scoped)] + [InlineData(ServiceLifetime.Transient)] + // We are not testing singleton here because singleton resolutions always got through + // runtime resolver and there is no sense to eliminating call from there + public void BuildExpressionElidesDisposableCaptureForEnumerableServices(ServiceLifetime lifetime) { - var descriptors = new ServiceCollection(); - descriptors.AddTransient(); - descriptors.AddTransient(); + IServiceCollection descriptors = new ServiceCollection(); + descriptors.Add(ServiceDescriptor.Describe(typeof(ServiceA), typeof(ServiceA), lifetime)); + descriptors.Add(ServiceDescriptor.Describe(typeof(ServiceD), typeof(ServiceD), lifetime)); var disposables = new List(); var provider = new ServiceProvider(descriptors, ServiceProviderOptions.Default);