Fix IHost being disposed of twice

[martincostello]

Fix IHost being disposed of twice

Fix the IHost wrapped by DeferredHost being disposed of twice.

Description

Fix the IHost wrapped by DeferredHost being disposed of twice if the implementation implements IAsyncDisposable, as it would first call IHost.DisposeAsync(), and then Dispose() which would call IHost.Dispose() immediately afterwards.

Now only one of the two dispose methods is called.

This surfaced in the integration tests using Microsoft.AspNetCore.Mvc.Testing in an application I updated from 6.0.0-rc.1.21451.13 to 6.0.100-rc.2.21505.57 where the application uses the Azure Key Vault configuration provider, which throws an ObjectDisposedException if it is disposed of twice:

CancellationTokenSource.Cancel()
AzureKeyVaultConfigurationProvider.Dispose(Boolean disposing)
AzureKeyVaultConfigurationProvider.Dispose()
ConfigurationRoot.Dispose()
ServiceProviderEngineScope.DisposeAsync()
--- End of stack trace from previous location ---
Host.<DisposeAsync>g__DisposeAsync|16_0(Object o)
Host.DisposeAsync()
Host.Dispose()
DeferredHost.Dispose()
HttpServerFixture.Dispose(Boolean disposing) line 160
WebApplicationFactory`1.DisposeAsync()
WebApplicationFactory`1.Dispose(Boolean disposing)
HttpServerFixture.Dispose(Boolean disposing) line 154
WebApplicationFactory`1.Dispose()

I'm not sure what change specifically has precipitated this issue being surfaced by RC2, as DeferredHostBuilder doesn't appear to have been changed for several months - possibly #36832 introduced it by the configuration being added to the service collection.

Happy to add any required tests for this, but I've gotten pushback on adding unit tests for WebApplicationFactory<T> before as they sort of need to be integration tests by design to validate it works (Who tests the tests?).

[campersau]

There is a return statement in line 142 so this shouldn't make any difference or?

[martincostello]

Good point - I missed that. In that case, the fix needs to be somewhere else then 🕵️

[martincostello]

I traced this through further after turning off Just My Code in Visual Studio and it seems the flow of events is:

1. xunit disposes of the WebApplicationFactory, which stops the host (code), which is the DeferredHost;
2. The DeferredHost calls StopAsync() on the inner IHost (code);
3. RunAsync() gets the shutdown signal so disposes of the IHost, which is the WebApplication (code);
4. WebApplication disposes of the inner IHost (code);
5. WebApplicationFactory disposes of the DeferredHostBuilder (code);
6. DeferredHost disposes the inner IHost again (code).

Then calling Dispose() again on my reference to the host (which doesn't know it's disposed of separately) trips over the ObjectDisposedException from the inner host being disposed already.

I'm not 100% convinced these changes are the fix (and the Azure provider should probably be resilient to double-disposal too, I'll look at raising a PR for that over there), but they seem to work towards having the application code work the same as it did in RC1.

[Tratcher]

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?

[martincostello]

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?

[halter73]

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

[martincostello]

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.

[halter73]

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.