Skip to content
This repository was archived by the owner on Nov 2, 2018. It is now read-only.

Commit 0893e6d

Browse files
pakrympakrym
authored
Dispose services in reverse creation order (#505)
1 parent e9def8c commit 0893e6d

File tree

10 files changed

+153
-32
lines changed

10 files changed

+153
-32
lines changed

‎src/Microsoft.Extensions.DependencyInjection.Specification.Tests/DependencyInjectionSpecificationTests.cs

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -655,5 +655,30 @@ public void ServiceContainerPicksConstructorWithLongestMatches(
655655
Assert.Same(expected.MultipleService, actual.MultipleService);
656656
Assert.Same(expected.ScopedService, actual.ScopedService);
657657
}
658+
659+
[Fact]
660+
public void DisposesInReverseOrderOfCreation()
661+
{
662+
// Arrange
663+
var serviceCollection = new TestServiceCollection();
664+
serviceCollection.AddSingleton<FakeDisposeCallback>();
665+
serviceCollection.AddTransient<IFakeOuterService, FakeDisposableCallbackOuterService>();
666+
serviceCollection.AddSingleton<IFakeMultipleService, FakeDisposableCallbackInnerService>();
667+
serviceCollection.AddScoped<IFakeMultipleService, FakeDisposableCallbackInnerService>();
668+
serviceCollection.AddTransient<IFakeMultipleService, FakeDisposableCallbackInnerService>();
669+
serviceCollection.AddSingleton<IFakeService, FakeDisposableCallbackInnerService>();
670+
var serviceProvider = CreateServiceProvider(serviceCollection);
671+
672+
var callback = serviceProvider.GetService<FakeDisposeCallback>();
673+
var outer = serviceProvider.GetService<IFakeOuterService>();
674+
675+
// Act
676+
((IDisposable)serviceProvider).Dispose();
677+
678+
// Assert
679+
Assert.Equal(outer, callback.Disposed[0]);
680+
Assert.Equal(outer.MultipleServices.Reverse(), callback.Disposed.Skip(1).Take(3).OfType<IFakeMultipleService>());
681+
Assert.Equal(outer.SingleService, callback.Disposed[4]);
682+
}
658683
}
659684
}

‎src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Fakes/FakeDisposableCallbackInnerService.cs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
// Copyright (c) .NET Foundation. All rights reserved.
2+
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
3+
4+
namespace Microsoft.Extensions.DependencyInjection.Specification.Fakes
5+
{
6+
public class FakeDisposableCallbackInnerService : FakeDisposableCallbackService, IFakeMultipleService
7+
{
8+
public FakeDisposableCallbackInnerService(FakeDisposeCallback callback) : base(callback)
9+
{
10+
}
11+
}
12+
}

‎src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Fakes/FakeDisposableCallbackOuterService.cs

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
// Copyright (c) .NET Foundation. All rights reserved.
2+
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
3+
4+
using System.Collections.Generic;
5+
6+
namespace Microsoft.Extensions.DependencyInjection.Specification.Fakes
7+
{
8+
public class FakeDisposableCallbackOuterService : FakeDisposableCallbackService, IFakeOuterService
9+
{
10+
public FakeDisposableCallbackOuterService(
11+
IFakeService singleService,
12+
IEnumerable<IFakeMultipleService> multipleServices,
13+
FakeDisposeCallback callback) : base(callback)
14+
{
15+
SingleService = singleService;
16+
MultipleServices = multipleServices;
17+
}
18+
19+
public IFakeService SingleService { get; }
20+
public IEnumerable<IFakeMultipleService> MultipleServices { get; }
21+
}
22+
}

‎src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Fakes/FakeDisposableCallbackService.cs

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
// Copyright (c) .NET Foundation. All rights reserved.
2+
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
3+
4+
using System;
5+
6+
namespace Microsoft.Extensions.DependencyInjection.Specification.Fakes
7+
{
8+
public class FakeDisposableCallbackService: IDisposable
9+
{
10+
private static int _globalId;
11+
private readonly int _id;
12+
private readonly FakeDisposeCallback _callback;
13+
14+
public FakeDisposableCallbackService(FakeDisposeCallback callback)
15+
{
16+
_id = _globalId++;
17+
_callback = callback;
18+
}
19+
20+
public void Dispose()
21+
{
22+
_callback.Disposed.Add(this);
23+
}
24+
25+
public override string ToString()
26+
{
27+
return _id.ToString();
28+
}
29+
}
30+
}

‎src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Fakes/FakeDisposeCallback.cs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
// Copyright (c) .NET Foundation. All rights reserved.
2+
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
3+
4+
using System.Collections.Generic;
5+
6+
namespace Microsoft.Extensions.DependencyInjection.Specification.Fakes
7+
{
8+
public class FakeDisposeCallback
9+
{
10+
public List<object> Disposed { get; } = new List<object>();
11+
}
12+
}

‎src/Microsoft.Extensions.DependencyInjection.Specification.Tests/Microsoft.Extensions.DependencyInjection.Specification.Tests.csproj

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,4 +19,8 @@
1919
<PackageReference Include="xunit.extensibility.core" Version="$(XunitVersion)" />
2020
</ItemGroup>
2121

22+
<ItemGroup>
23+
<Service Include="{82a7f48d-3b50-4b1e-b82e-3ada8210c358}" />
24+
</ItemGroup>
25+
2226
</Project>

‎src/Microsoft.Extensions.DependencyInjection/ServiceLookup/CallSiteExpressionBuilder.cs

Lines changed: 24 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -146,16 +146,24 @@ protected override Expression VisitClosedIEnumerable(ClosedIEnumerableCallSite c
146146
protected override Expression VisitTransient(TransientCallSite callSite, ParameterExpression provider)
147147
{
148148
var implType = callSite.Service.ImplementationType;
149-
150149
// Elide calls to GetCaptureDisposable if the implemenation type isn't disposable
150+
return TryCaptureDisposible(
151+
implType,
152+
provider,
153+
VisitCallSite(callSite.ServiceCallSite, provider));
154+
}
155+
156+
private Expression TryCaptureDisposible(Type implType, ParameterExpression provider, Expression service)
157+
{
158+
151159
if (implType != null &&
152160
!typeof(IDisposable).GetTypeInfo().IsAssignableFrom(implType.GetTypeInfo()))
153161
{
154-
return VisitCallSite(callSite.ServiceCallSite, provider);
162+
return service;
155163
}
156164

157165
return Expression.Invoke(GetCaptureDisposable(provider),
158-
VisitCallSite(callSite.ServiceCallSite, provider));
166+
service);
159167
}
160168

161169
protected override Expression VisitConstructor(ConstructorCallSite callSite, ParameterExpression provider)
@@ -184,34 +192,40 @@ protected override Expression VisitScoped(ScopedCallSite callSite, ParameterExpr
184192
callSite.Key,
185193
typeof(object));
186194

187-
var resolvedExpression = Expression.Variable(typeof(object), "resolved");
195+
var resolvedVariable = Expression.Variable(typeof(object), "resolved");
188196

189197
var resolvedServices = GetResolvedServices(provider);
190198

191199
var tryGetValueExpression = Expression.Call(
192200
resolvedServices,
193201
TryGetValueMethodInfo,
194202
keyExpression,
195-
resolvedExpression);
203+
resolvedVariable);
204+
205+
var service = VisitCallSite(callSite.ServiceCallSite, provider);
206+
var captureDisposible = TryCaptureDisposible(callSite.Key.ImplementationType, provider, service);
196207

197208
var assignExpression = Expression.Assign(
198-
resolvedExpression, VisitCallSite(callSite.ServiceCallSite, provider));
209+
resolvedVariable,
210+
captureDisposible);
199211

200212
var addValueExpression = Expression.Call(
201213
resolvedServices,
202214
AddMethodInfo,
203215
keyExpression,
204-
resolvedExpression);
216+
resolvedVariable);
205217

206218
var blockExpression = Expression.Block(
207219
typeof(object),
208220
new[] {
209-
resolvedExpression
221+
resolvedVariable
210222
},
211223
Expression.IfThen(
212224
Expression.Not(tryGetValueExpression),
213-
Expression.Block(assignExpression, addValueExpression)),
214-
resolvedExpression);
225+
Expression.Block(
226+
assignExpression,
227+
addValueExpression)),
228+
resolvedVariable);
215229

216230
return blockExpression;
217231
}

‎src/Microsoft.Extensions.DependencyInjection/ServiceLookup/CallSiteRuntimeResolver.cs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ protected override object VisitScoped(ScopedCallSite scopedCallSite, ServiceProv
4949
if (!provider.ResolvedServices.TryGetValue(scopedCallSite.Key, out resolved))
5050
{
5151
resolved = VisitCallSite(scopedCallSite.ServiceCallSite, provider);
52+
provider.CaptureDisposable(resolved);
5253
provider.ResolvedServices.Add(scopedCallSite.Key, resolved);
5354
}
5455
}

‎src/Microsoft.Extensions.DependencyInjection/ServiceProvider.cs

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ internal class ServiceProvider : IServiceProvider, IDisposable
1919
{
2020
private readonly CallSiteValidator _callSiteValidator;
2121
private readonly ServiceTable _table;
22-
private readonly ServiceProviderOptions _options;
2322
private bool _disposeCalled;
2423
private List<IDisposable> _transientDisposables;
2524

@@ -43,7 +42,6 @@ public ServiceProvider(IEnumerable<ServiceDescriptor> serviceDescriptors, Servic
4342
_callSiteValidator = new CallSiteValidator();
4443
}
4544

46-
_options = options;
4745
_table = new ServiceTable(serviceDescriptors);
4846

4947
_table.Add(typeof(IServiceProvider), new ServiceProviderService());
@@ -169,22 +167,15 @@ public void Dispose()
169167
_disposeCalled = true;
170168
if (_transientDisposables != null)
171169
{
172-
foreach (var disposable in _transientDisposables)
170+
for (int i = _transientDisposables.Count - 1; i >= 0; i--)
173171
{
172+
var disposable = _transientDisposables[i];
174173
disposable.Dispose();
175174
}
176175

177176
_transientDisposables.Clear();
178177
}
179178

180-
// PERF: We've enumerating the dictionary so that we don't allocate to enumerate.
181-
// .Values allocates a ValueCollection on the heap, enumerating the dictionary allocates
182-
// a struct enumerator
183-
foreach (var entry in ResolvedServices)
184-
{
185-
(entry.Value as IDisposable)?.Dispose();
186-
}
187-
188179
ResolvedServices.Clear();
189180
}
190181
}

‎test/Microsoft.Extensions.DependencyInjection.Tests/CallSiteTests.cs

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
using System;
55
using System.Collections;
66
using System.Collections.Generic;
7-
using System.Linq.Expressions;
87
using Microsoft.Extensions.DependencyInjection.ServiceLookup;
98
using Microsoft.Extensions.DependencyInjection.Specification.Fakes;
109
using Xunit;
@@ -121,12 +120,19 @@ public void BuiltExpressionCanResolveNestedScopedService()
121120
Assert.Equal(serviceC, Invoke(callSite, provider));
122121
}
123122

124-
[Fact]
125-
public void BuildExpressionElidesDisposableCaptureForNonDisposableServices()
123+
[Theory]
124+
[InlineData(ServiceLifetime.Scoped)]
125+
[InlineData(ServiceLifetime.Transient)]
126+
// We are not testing singleton here because singleton resolutions always got through
127+
// runtime resolver and there is no sense to eliminating call from there
128+
public void BuildExpressionElidesDisposableCaptureForNonDisposableServices(ServiceLifetime lifetime)
126129
{
127-
var descriptors = new ServiceCollection();
128-
descriptors.AddTransient<ServiceA>();
129-
descriptors.AddTransient<ServiceB>();
130+
IServiceCollection descriptors = new ServiceCollection();
131+
descriptors.Add(ServiceDescriptor.Describe(typeof(ServiceA), typeof(ServiceA), lifetime));
132+
descriptors.Add(ServiceDescriptor.Describe(typeof(ServiceB), typeof(ServiceB), lifetime));
133+
descriptors.Add(ServiceDescriptor.Describe(typeof(ServiceC), typeof(ServiceC), lifetime));
134+
135+
descriptors.AddScoped<ServiceB>();
130136
descriptors.AddTransient<ServiceC>();
131137

132138
var disposables = new List<object>();
@@ -143,12 +149,16 @@ public void BuildExpressionElidesDisposableCaptureForNonDisposableServices()
143149
Assert.Equal(0, disposables.Count);
144150
}
145151

146-
[Fact]
147-
public void BuildExpressionElidesDisposableCaptureForEnumerableServices()
152+
[Theory]
153+
[InlineData(ServiceLifetime.Scoped)]
154+
[InlineData(ServiceLifetime.Transient)]
155+
// We are not testing singleton here because singleton resolutions always got through
156+
// runtime resolver and there is no sense to eliminating call from there
157+
public void BuildExpressionElidesDisposableCaptureForEnumerableServices(ServiceLifetime lifetime)
148158
{
149-
var descriptors = new ServiceCollection();
150-
descriptors.AddTransient<ServiceA>();
151-
descriptors.AddTransient<ServiceD>();
159+
IServiceCollection descriptors = new ServiceCollection();
160+
descriptors.Add(ServiceDescriptor.Describe(typeof(ServiceA), typeof(ServiceA), lifetime));
161+
descriptors.Add(ServiceDescriptor.Describe(typeof(ServiceD), typeof(ServiceD), lifetime));
152162

153163
var disposables = new List<object>();
154164
var provider = new ServiceProvider(descriptors, ServiceProviderOptions.Default);

0 commit comments

Comments
 (0)