Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix IHost being disposed of twice #37631

Closed
wants to merge 2 commits into from
Closed

Fix IHost being disposed of twice #37631

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
721cf1f
Fix IHost being disposed of twice
martincostello Oct 17, 2021
7301823
Do not dispose the host twice
martincostello Oct 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions src/DefaultBuilder/src/WebApplication.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ public sealed class WebApplication : IHost, IApplicationBuilder, IEndpointRouteB
private readonly IHost _host;
private readonly List<EndpointDataSource> _dataSources = new();

private bool _disposed;

internal WebApplication(IHost host)
{
_host = host;
Expand Down Expand Up @@ -162,12 +164,26 @@ public void Run(string? url = null)
/// <summary>
/// Disposes the application.
/// </summary>
void IDisposable.Dispose() => _host.Dispose();
void IDisposable.Dispose()
{
if (!_disposed)
{
_host.Dispose();
_disposed = true;
}
}
Comment on lines +167 to +174
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern seems like a contradiction, it asserts two things:
A) Dispose should no-op if called a second time.
B) It's not safe to call IHost.Dispose a second time.

Why are different requirements being applied to WebApplication's implementation of Dispose vs IHost's? If IHost's implementation of Dispose does something similar then why does WA need any additional logic?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "only doing it once bit" I've taken from just the standard IDisposable implementation pattern, but I think your question is really getting at the fact that I'm not sure exactly why this has started happening in RC2.

At first I thought it was because DeferredHost actually was disposing twice in the same call, but it turned out I'd just misread the code. After that, it just seemed correct to have both these types (which were to only two I looked at in this repo) follow that pattern anyway.

You're right though that Host itself also doesn't follow this pattern either, which is why I don't think this change is the fix as two different objects are disposing the same underlying IHost twice overall, so even with these guards it's still disposed of twice, just from two different places that don't know about each other.

https://github.com/dotnet/runtime/blob/8608dca513a9be1f1bfc6a31deb8b22639a33d9f/src/libraries/Microsoft.Extensions.Hosting/src/Internal/Host.cs#L162-L195

Maybe the fix is to instead (or also) make a similar change there too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even Host.DisposeAsync() is just disposing other types. Should AzureKeyVaultConfigurationProvider be fixed so dispose can be called more than once?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened a PR for that, not sure when it'll be available in NuGet to consume though. Azure/azure-sdk-for-net#24769

I think at this point this PR has devolved into more of an "issue" (what changed to make this start happening, and is that bad?) as what I originally thought was the fix was a red-herring.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should probably close this then. I'm not a big fan of having _dispose flags in every class that just disposes its fields in Dispose() unless it fixes some sort of pervasive problem.


/// <summary>
/// Disposes the application.
/// </summary>
public ValueTask DisposeAsync() => ((IAsyncDisposable)_host).DisposeAsync();
public async ValueTask DisposeAsync()
{
if (!_disposed)
{
await ((IAsyncDisposable)_host).DisposeAsync();
_disposed = true;
}
}

internal RequestDelegate BuildRequestDelegate() => ApplicationBuilder.Build();
RequestDelegate IApplicationBuilder.Build() => BuildRequestDelegate();
Expand Down
16 changes: 11 additions & 5 deletions src/Mvc/Mvc.Testing/src/DeferredHostBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,6 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Generic;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
Expand Down Expand Up @@ -124,6 +120,8 @@ private class DeferredHost : IHost, IAsyncDisposable
private readonly IHost _host;
private readonly TaskCompletionSource _hostStartedTcs;

private bool _disposed;

public DeferredHost(IHost host, TaskCompletionSource hostStartedTcs)
{
_host = host;
Expand All @@ -132,13 +130,21 @@ public DeferredHost(IHost host, TaskCompletionSource hostStartedTcs)

public IServiceProvider Services => _host.Services;

public void Dispose() => _host.Dispose();
public void Dispose()
{
if (!_disposed)
{
_host.Dispose();
_disposed = true;
}
}

public async ValueTask DisposeAsync()
{
if (_host is IAsyncDisposable disposable)
{
await disposable.DisposeAsync().ConfigureAwait(false);
_disposed = true;
return;
}
Dispose();
Expand Down