Skip to content

Commit 800f26a

Browse files
rosebyteJaroslav Ruzickaam11janvorliEgorBo
authored
ServiceProviderEngineScope should aggregate exceptions in Dispose rather than throwing on the first (#123342)
Fixes: #86426 This PR introduces a behavior‑breaking change: when a DI scope is disposed, the first exception thrown currently propagates immediately, leaving all subsequent disposables in the scope undisposed. With this PR: All disposables in the scope will be disposed, and if any exceptions are thrown, they will be propagated only after disposal completes. If multiple exceptions are thrown, an aggregated exception will be propagated. This breaking change may be acceptable since throwing from Dispose methods goes against .NET coding guidelines. Additionally, there are no clear scenarios where users rely on specific exception types thrown during DI scope disposal. The try/catch blocks were placed outside the foreach loop to avoid repetitive Exception Handling Table writes, which may have performance implications when a scope contains many disposables. While disposing a DI scope typically isn't a hot path, it can still impose unnecessary CPU overhead in high‑throughput scenarios. For example, ASP.NET disposes a DI scope after each request, which can result in many EHT writes on heavily loaded servers. That said, if this feels over‑engineered, we can rewrite it for improved readability. For further optimization, we could consider allocating the list of exceptions only when multiple exceptions occur. This adjustment would be straightforward to implement. --------- Co-authored-by: Jaroslav Ruzicka <jruzicka@microsoft.com> Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com> Co-authored-by: Jan Vorlicek <janvorli@microsoft.com> Co-authored-by: Egor Bogatov <egorbo@gmail.com> Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: Jan Kotas <jkotas@microsoft.com> Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com> Co-authored-by: Mitchell Hwang <16830051+mdh1418@users.noreply.github.com> Co-authored-by: steveisok <471438+steveisok@users.noreply.github.com> Co-authored-by: Xu Liangyu <xuliangyu@loongson.cn> Co-authored-by: Kevin Jones <kevin@vcsjones.com> Co-authored-by: Jeremy Barton <jbarton@microsoft.com> Co-authored-by: Hakan Fıstık <HakamFostok@users.noreply.github.com> Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com> Co-authored-by: prozolic <42107886+prozolic@users.noreply.github.com> Co-authored-by: Stephen Toub <stoub@microsoft.com> Co-authored-by: Rachel <rachel.jarvi@gmail.com> Co-authored-by: Gordon Ross <Gordon.W.Ross@gmail.com> Co-authored-by: Austin Wise <AustinWise@gmail.com>
1 parent c4ef76a commit 800f26a

File tree

2 files changed

+226
-30
lines changed

2 files changed

+226
-30
lines changed

‎src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs‎

Lines changed: 92 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
using System.Collections.Generic;
66
using System.Diagnostics;
77
using System.Diagnostics.CodeAnalysis;
8+
using System.Runtime.ExceptionServices;
89
using System.Threading.Tasks;
910
using Microsoft.Extensions.Internal;
1011

@@ -120,10 +121,15 @@ public object GetRequiredKeyedService(Type serviceType, object? serviceKey)
120121
public void Dispose()
121122
{
122123
List<object>? toDispose = BeginDispose();
124+
if (toDispose is null)
125+
{
126+
return;
127+
}
123128

124-
if (toDispose != null)
129+
object? exceptionsCache = null;
130+
for (var i = toDispose.Count - 1; i >= 0; i--)
125131
{
126-
for (int i = toDispose.Count - 1; i >= 0; i--)
132+
try
127133
{
128134
if (toDispose[i] is IDisposable disposable)
129135
{
@@ -134,68 +140,126 @@ public void Dispose()
134140
throw new InvalidOperationException(SR.Format(SR.AsyncDisposableServiceDispose, TypeNameHelper.GetTypeDisplayName(toDispose[i])));
135141
}
136142
}
143+
catch (Exception exception)
144+
{
145+
AddExceptionToCache(ref exceptionsCache, exception);
146+
}
137147
}
148+
149+
CheckExceptionCache(exceptionsCache);
138150
}
139151

140152
public ValueTask DisposeAsync()
141153
{
142154
List<object>? toDispose = BeginDispose();
155+
if (toDispose is null)
156+
{
157+
return default;
158+
}
143159

144-
if (toDispose != null)
160+
object? exceptionsCache = null;
161+
for (var i = toDispose.Count - 1; i >= 0; i--)
145162
{
146163
try
147164
{
148-
for (int i = toDispose.Count - 1; i >= 0; i--)
165+
object disposable = toDispose[i];
166+
if (disposable is IAsyncDisposable asyncDisposable)
149167
{
150-
object disposable = toDispose[i];
151-
if (disposable is IAsyncDisposable asyncDisposable)
152-
{
153-
ValueTask vt = asyncDisposable.DisposeAsync();
154-
if (!vt.IsCompletedSuccessfully)
155-
{
156-
return Await(i, vt, toDispose);
157-
}
158-
159-
// If its a IValueTaskSource backed ValueTask,
160-
// inform it its result has been read so it can reset
161-
vt.GetAwaiter().GetResult();
162-
}
163-
else
168+
ValueTask vt = asyncDisposable.DisposeAsync();
169+
if (!vt.IsCompletedSuccessfully)
164170
{
165-
((IDisposable)disposable).Dispose();
171+
return Await(i, vt, toDispose, exceptionsCache);
166172
}
173+
174+
// If its a IValueTaskSource backed ValueTask,
175+
// inform it its result has been read so it can reset
176+
vt.GetAwaiter().GetResult();
177+
}
178+
else
179+
{
180+
((IDisposable)disposable).Dispose();
167181
}
168182
}
169-
catch (Exception ex)
183+
catch (Exception exception)
170184
{
171-
return new ValueTask(Task.FromException(ex));
185+
AddExceptionToCache(ref exceptionsCache, exception);
172186
}
173187
}
174188

189+
CheckExceptionCache(exceptionsCache);
190+
175191
return default;
176192

177-
static async ValueTask Await(int i, ValueTask vt, List<object> toDispose)
193+
static async ValueTask Await(int i, ValueTask vt, List<object> toDispose, object? exceptionsCache)
178194
{
179-
await vt.ConfigureAwait(false);
195+
try
196+
{
197+
await vt.ConfigureAwait(false);
198+
}
199+
catch (Exception exception)
200+
{
201+
AddExceptionToCache(ref exceptionsCache, exception);
202+
}
203+
180204
// vt is acting on the disposable at index i,
181205
// decrement it and move to the next iteration
182206
i--;
183207

184208
for (; i >= 0; i--)
185209
{
186-
object disposable = toDispose[i];
187-
if (disposable is IAsyncDisposable asyncDisposable)
210+
try
188211
{
189-
await asyncDisposable.DisposeAsync().ConfigureAwait(false);
212+
object disposable = toDispose[i];
213+
if (disposable is IAsyncDisposable asyncDisposable)
214+
{
215+
await asyncDisposable.DisposeAsync().ConfigureAwait(false);
216+
}
217+
else
218+
{
219+
((IDisposable)disposable).Dispose();
220+
}
190221
}
191-
else
222+
catch (Exception exception)
192223
{
193-
((IDisposable)disposable).Dispose();
224+
AddExceptionToCache(ref exceptionsCache, exception);
194225
}
195226
}
227+
228+
CheckExceptionCache(exceptionsCache);
196229
}
197230
}
198231

232+
private static void AddExceptionToCache(ref object? exceptionsCache, Exception exception)
233+
{
234+
if (exceptionsCache is null)
235+
{
236+
exceptionsCache = ExceptionDispatchInfo.Capture(exception);
237+
}
238+
else if (exceptionsCache is ExceptionDispatchInfo exceptionInfo)
239+
{
240+
exceptionsCache = new List<Exception> { exceptionInfo.SourceException, exception };
241+
}
242+
else
243+
{
244+
((List<Exception>)exceptionsCache).Add(exception);
245+
}
246+
}
247+
248+
private static void CheckExceptionCache(object? exceptionsCache)
249+
{
250+
if (exceptionsCache is null)
251+
{
252+
return;
253+
}
254+
255+
if (exceptionsCache is ExceptionDispatchInfo exceptionInfo)
256+
{
257+
exceptionInfo.Throw();
258+
}
259+
260+
throw new AggregateException((List<Exception>)exceptionsCache);
261+
}
262+
199263
private List<object>? BeginDispose()
200264
{
201265
lock (Sync)

‎src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderEngineScopeTests.cs‎

Lines changed: 134 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,9 @@
22
// The .NET Foundation licenses this file to you under the MIT license.
33

44
using System;
5-
using System.Collections.Generic;
5+
using System.Threading.Tasks;
66
using Microsoft.Extensions.DependencyInjection.Specification.Fakes;
77
using Xunit;
8-
using Xunit.Abstractions;
98

109
namespace Microsoft.Extensions.DependencyInjection.ServiceLookup
1110
{
@@ -41,5 +40,138 @@ public void ServiceProviderEngineScope_ImplementsAllServiceProviderInterfaces()
4140
Assert.Contains(serviceProviderInterface, engineScopeInterfaces);
4241
}
4342
}
43+
44+
[Fact]
45+
public void Dispose_ServiceThrows_DisposesAllAndThrows()
46+
{
47+
var services = new ServiceCollection();
48+
services.AddKeyedTransient("throws", (_, _) => new TestDisposable(true));
49+
services.AddKeyedTransient("doesnotthrow", (_, _) => new TestDisposable(false));
50+
51+
var scope = services.BuildServiceProvider().GetRequiredService<IServiceScopeFactory>().CreateScope().ServiceProvider;
52+
53+
var disposables = new TestDisposable[]
54+
{
55+
scope.GetRequiredKeyedService<TestDisposable>("throws"),
56+
scope.GetRequiredKeyedService<TestDisposable>("doesnotthrow")
57+
};
58+
59+
var exception = Assert.Throws<InvalidOperationException>(() => ((IDisposable)scope).Dispose());
60+
Assert.Equal(TestDisposable.ErrorMessage, exception.Message);
61+
Assert.All(disposables, disposable => Assert.True(disposable.IsDisposed));
62+
}
63+
64+
[Fact]
65+
public void Dispose_TwoServicesThrows_DisposesAllAndThrowsAggregateException()
66+
{
67+
var services = new ServiceCollection();
68+
services.AddKeyedTransient("throws", (_, _) => new TestDisposable(true));
69+
services.AddKeyedTransient("doesnotthrow", (_, _) => new TestDisposable(false));
70+
71+
var scope = services.BuildServiceProvider().GetRequiredService<IServiceScopeFactory>().CreateScope().ServiceProvider;
72+
73+
var disposables = new TestDisposable[]
74+
{
75+
scope.GetRequiredKeyedService<TestDisposable>("throws"),
76+
scope.GetRequiredKeyedService<TestDisposable>("doesnotthrow"),
77+
scope.GetRequiredKeyedService<TestDisposable>("throws"),
78+
scope.GetRequiredKeyedService<TestDisposable>("doesnotthrow"),
79+
};
80+
81+
var exception = Assert.Throws<AggregateException>(() => ((IDisposable)scope).Dispose());
82+
Assert.Equal(2, exception.InnerExceptions.Count);
83+
Assert.All(exception.InnerExceptions, ex => Assert.IsType<InvalidOperationException>(ex));
84+
Assert.All(disposables, disposable => Assert.True(disposable.IsDisposed));
85+
}
86+
87+
[Theory]
88+
[InlineData(true)]
89+
[InlineData(false)]
90+
public async Task DisposeAsync_ServiceThrows_DisposesAllAndThrows(bool synchronous)
91+
{
92+
var services = new ServiceCollection();
93+
services.AddKeyedTransient("throws", (_, _) => new TestDisposable(true, synchronous));
94+
services.AddKeyedTransient("doesnotthrow", (_, _) => new TestDisposable(false, synchronous));
95+
96+
var scope = services.BuildServiceProvider().GetRequiredService<IServiceScopeFactory>().CreateScope().ServiceProvider;
97+
98+
var disposables = new TestDisposable[]
99+
{
100+
scope.GetRequiredKeyedService<TestDisposable>("throws"),
101+
scope.GetRequiredKeyedService<TestDisposable>("doesnotthrow")
102+
};
103+
104+
var exception = await Assert.ThrowsAsync<InvalidOperationException>(async () => await ((IAsyncDisposable)scope).DisposeAsync());
105+
Assert.Equal(TestDisposable.ErrorMessage, exception.Message);
106+
Assert.All(disposables, disposable => Assert.True(disposable.IsDisposed));
107+
}
108+
109+
[Theory]
110+
[InlineData(true)]
111+
[InlineData(false)]
112+
public async Task DisposeAsync_TwoServicesThrows_DisposesAllAndThrowsAggregateException(bool synchronous)
113+
{
114+
var services = new ServiceCollection();
115+
services.AddKeyedTransient("throws", (_, _) => new TestDisposable(true, synchronous));
116+
services.AddKeyedTransient("doesnotthrow", (_, _) => new TestDisposable(false, synchronous));
117+
118+
var scope = services.BuildServiceProvider().GetRequiredService<IServiceScopeFactory>().CreateScope().ServiceProvider;
119+
120+
var disposables = new TestDisposable[]
121+
{
122+
scope.GetRequiredKeyedService<TestDisposable>("throws"),
123+
scope.GetRequiredKeyedService<TestDisposable>("doesnotthrow"),
124+
scope.GetRequiredKeyedService<TestDisposable>("throws"),
125+
scope.GetRequiredKeyedService<TestDisposable>("doesnotthrow"),
126+
};
127+
128+
var exception = await Assert.ThrowsAsync<AggregateException>(async () => await ((IAsyncDisposable)scope).DisposeAsync());
129+
Assert.Equal(2, exception.InnerExceptions.Count);
130+
Assert.All(exception.InnerExceptions, ex => Assert.IsType<InvalidOperationException>(ex));
131+
Assert.All(disposables, disposable => Assert.True(disposable.IsDisposed));
132+
}
133+
134+
private class TestDisposable : IDisposable, IAsyncDisposable
135+
{
136+
public const string ErrorMessage = "Dispose failed.";
137+
138+
private readonly bool _throwsOnDispose;
139+
private readonly bool _synchronous;
140+
141+
public bool IsDisposed { get; private set; }
142+
143+
public TestDisposable(bool throwsOnDispose = false, bool synchronous = false)
144+
{
145+
_throwsOnDispose = throwsOnDispose;
146+
_synchronous = synchronous;
147+
}
148+
149+
public void Dispose()
150+
{
151+
IsDisposed = true;
152+
153+
if (_throwsOnDispose)
154+
{
155+
throw new InvalidOperationException(ErrorMessage);
156+
}
157+
}
158+
159+
public ValueTask DisposeAsync()
160+
{
161+
if (_synchronous)
162+
{
163+
Dispose();
164+
return default;
165+
}
166+
167+
return new ValueTask(DisposeAsyncInternal());
168+
169+
async Task DisposeAsyncInternal()
170+
{
171+
await Task.Yield();
172+
Dispose();
173+
}
174+
}
175+
}
44176
}
45177
}

0 commit comments

Comments
 (0)