ServiceProviderEngineScope should aggregate exceptions in Dispose rather than throwing on the first

[rosebyte]

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.

[Copilot]

Pull request overview

This PR changes the disposal behavior of ServiceProviderEngineScope to aggregate exceptions during disposal rather than throwing on the first exception. Previously, when a disposable service threw an exception during disposal, the first exception would propagate immediately, leaving subsequent disposables undisposed. With this change, all disposables are disposed, and exceptions are aggregated into an AggregateException if multiple exceptions occur, or thrown as a single exception if only one occurs.

Changes:

• Modified Dispose() and DisposeAsync() methods to use exception aggregation with try-catch blocks
• Added comprehensive test coverage for both single and multiple exception scenarios in synchronous and asynchronous disposal
• Implemented a while/for loop structure to continue disposal after catching exceptions

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.

File	Description
ServiceProviderEngineScope.cs	Modified Dispose() and DisposeAsync() to aggregate exceptions and continue disposal after exceptions occur
ServiceProviderEngineScopeTests.cs	Added 4 new test methods and a TestDisposable helper class to verify exception aggregation behavior

Comments suppressed due to low confidence (2)

src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs:234

• The Await helper method does not aggregate exceptions. If any disposal throws an exception, it will propagate immediately without disposing remaining items. This contradicts the PR's goal to ensure all disposables are disposed before propagating exceptions. The method needs to be refactored with try-catch blocks to collect exceptions and continue disposal, similar to the synchronous path in the DisposeAsync method.

            {
                await vt.ConfigureAwait(false);
                // vt is acting on the disposable at index i,
                // decrement it and move to the next iteration
                i--;

                for (; i >= 0; i--)
                {
                    object disposable = toDispose[i];
                    if (disposable is IAsyncDisposable asyncDisposable)
                    {
                        await asyncDisposable.DisposeAsync().ConfigureAwait(false);
                    }
                    else
                    {
                        ((IDisposable)disposable).Dispose();
                    }
                }
            }
        }

src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs:8

• The using directive for System.Linq is unused and should be removed.

using System.Threading.Tasks;

[CarnaViire]

Do I understand correctly that before, if any of the services happened to be IAsyncDisposable-only, it literally was breaking all the further disposals? 😅

Should we add a test for this, and also for the mix of IDisposable-only and IAsyncDisposable, to verify it works?

---

Also, I found that we actually state in the docs regarding IAsyncDisposables in DI:

Specifically, implementations of IDisposable and IAsyncDisposable are properly disposed at the end of their specified lifetime.

The way I'd interpret it, we promise here to clean up IAsyncDisposables regardless, but the implementation doesn't follow on this promise? 🤔

There is no warnings about this exception in the AsyncServiceScope.Dispose() API docs either... Is it documented somewhere?..

It might be smth out of scope of this PR, but it feels like something we might potentially want to fix? Especially given we already handle this case above in the CaptureDisposable method.

---

Re: "catch outside of for" optimization -- I'd vote for keeping it simple, i.e. ordinary for/foreach, and catch inside it, unless are we actually sure this will give any kind of perf gain (i.e. a microbenchmark). Am I missing smth? I don't think I've seen this pattern in the libraries, e.g. here is a straightforward foreach:

runtime/src/libraries/System.Threading.RateLimiting/src/System/Threading/RateLimiting/DefaultPartitionedRateLimiter.cs

Lines 128 to 139 in 531dd27

	foreach (KeyValuePair<TKey, Lazy<RateLimiter>> limiter in _limiters)
	{
	try
	{
	limiter.Value.Value.Dispose();
	}
	catch (Exception ex)
	{
	exceptions ??= new List<Exception>();
	exceptions.Add(ex);
	}
	}

[rosebyte]

That`s a good point, how about making an issue specifically about this so we collect more information and fix it in another PR?

[rosebyte]

I`ve created an issue to further discuss the behaviour, #123620.