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

[swsyn]

I understand that there is CA1065 rule

but

We run into errors during disposing some services in our Blazor Server App. We can wrap all our code in Dispose with try/catch but there can be third party services. It comes quite difficult to find the reason of some services were not disposed (also because Blazor's CircuitHost logs _disposeFailed with Debug level).

It seems difficult to assume that Dispose() will never throw an exception somewhere.

Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection
See info in area-owners.md if you want to be subscribed.

Issue Details

---

I understand that there is CA1065 rule

but

We run into errors during disposing some services in our Blazor Server App. We can wrap all our code in Dispose with try/catch but there can be third party services. It comes quite difficult to find the reason of some services were not disposed (also because Blazor's CircuitHost logs _disposeFailed with Debug level).

It seems difficult to assume that Dispose() will never throw an exception somewhere.

Author:	swsyn
Assignees:	-
Labels:	area-Extensions-DependencyInjection
Milestone:	-

[ericstj]

I don't think we'll want framework components to swallow exceptions here. Is this an ask to try to capture an exception and rethrow as AggregateException when disposing all services in

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

Lines 94 to 171 in 1e42167

	public void Dispose()
	{
	List<object>? toDispose = BeginDispose();
	if (toDispose != null)
	{
	for (int i = toDispose.Count - 1; i >= 0; i--)
	{
	if (toDispose[i] is IDisposable disposable)
	{
	disposable.Dispose();
	}
	else
	{
	throw new InvalidOperationException(SR.Format(SR.AsyncDisposableServiceDispose, TypeNameHelper.GetTypeDisplayName(toDispose[i])));
	}
	}
	}
	}
	public ValueTask DisposeAsync()
	{
	List<object>? toDispose = BeginDispose();
	if (toDispose != null)
	{
	try
	{
	for (int i = toDispose.Count - 1; i >= 0; i--)
	{
	object disposable = toDispose[i];
	if (disposable is IAsyncDisposable asyncDisposable)
	{
	ValueTask vt = asyncDisposable.DisposeAsync();
	if (!vt.IsCompletedSuccessfully)
	{
	return Await(i, vt, toDispose);
	}
	// If its a IValueTaskSource backed ValueTask,
	// inform it its result has been read so it can reset
	vt.GetAwaiter().GetResult();
	}
	else
	{
	((IDisposable)disposable).Dispose();
	}
	}
	}
	catch (Exception ex)
	{
	return new ValueTask(Task.FromException(ex));
	}
	}
	return default;
	static async ValueTask Await(int i, ValueTask vt, List<object> toDispose)
	{
	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();
	}
	}
	}
	}

?

Of is this an ask to make the Blazor CircuitHost to make the dispose exception more visible: https://github.com/dotnet/aspnetcore/blob/39564d529f84f7a3bbac5b28ba11060e8ac30375/src/Components/Server/src/Circuits/CircuitHost.cs#L199

[swsyn]

Yes, it was about both aforementioned variants:

1. We expected that the scope would try to dispose all disposable services and then throw an exception. But the scope just stop disposing.

It is important for us because we have some services which start timers and we need to stop them when the services are disposed. But they are not disposed because of broken service.

public class Service: IDisposable
{
	private readonly ITimerService timerService;
	private ITimer timer;
	public TimeSpan UpdateInterval { get; set; } = TimeSpan.FromMinutes(2);

	public Task SetupAsync()
	{
		timer = timerService.Create();
		timer.Interval = UpdateInterval;
		timer.Tick += TimerOnTick;
		timer.IsEnabled = true;

		return Task.CompletedTask;
	}
	
	private void TimerOnTick(object sender, EventArgs e) { }

	public void Dispose()
	{
		if (timer!=null) {
			timer.IsEnabled = false;
			timer.Tick -= TimerOnTick;
			timer = null;
		}
	}
}

We have modular app structure and some services can be written by other developers.

2. Yes, we also expected that Blazor CircuitHost dispose exception would be more visible.

[ericstj]

@swsyn can you open a separate issue in https://github.com/dotnet/aspnetcore with an ask for Blazor CircuitHost to stop hiding these exceptions?

Let this issue track a request to change the behavior of ServiceProviderEngineScope to not halt on the first Dispose exception, but to instead proceed and aggregate exceptions that might occur.

[swsyn]

dotnet/aspnetcore#48418

[steveharter]

Moving to 9.0; ServiceProviderEngineScope should try to run Dispose() on all services.