Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ namespace Microsoft.Testing.Platform.Hosts;
/// </summary>
internal abstract class CommonHost(ServiceProvider serviceProvider) : IHost
{
private readonly List<object> _alreadyDisposedServices = [];

public ServiceProvider ServiceProvider => serviceProvider;

protected IPushOnlyProtocol? PushOnlyProtocol => ServiceProvider.GetService<IPushOnlyProtocol>();
Expand Down Expand Up @@ -69,7 +71,7 @@ public async Task<int> RunAsync()
}
finally
{
await DisposeServiceProviderAsync(ServiceProvider, isProcessShutdown: true).ConfigureAwait(false);
await DisposeServiceProviderAsync(ServiceProvider, alreadyDisposed: _alreadyDisposedServices, isProcessShutdown: true).ConfigureAwait(false);
await DisposeHelper.DisposeAsync(ServiceProvider.GetService<FileLoggerProvider>()).ConfigureAwait(false);
await DisposeHelper.DisposeAsync(PushOnlyProtocol).ConfigureAwait(false);

Expand Down Expand Up @@ -121,6 +123,7 @@ private async Task<int> RunTestAppAsync(CancellationToken testApplicationCancell
{
await testApplicationLifecycleCallbacks.AfterRunAsync(exitCode, testApplicationCancellationToken).ConfigureAwait(false);
await DisposeHelper.DisposeAsync(testApplicationLifecycleCallbacks).ConfigureAwait(false);
_alreadyDisposedServices.Add(testApplicationLifecycleCallbacks);
Copy link
Member

Choose a reason for hiding this comment

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

From https://learn.microsoft.com/en-us/dotnet/api/system.idisposable.dispose?view=net-10.0#remarks

If an object's Dispose method is called more than once, the object must ignore all calls after the first one. The object must not throw an exception if its Dispose method is called multiple times. Instance methods other than Dispose can throw an ObjectDisposedException when resources are already disposed.

So calling multiple times looks valid. However, DisposeHelper actually does more than dispose, and it calls IAsyncCleanableExtension.CleanupAsync.

The interface IAsyncCleanableExtension doesn't document whether double cleanup should be handled correctly or not. And it's not immediately clear why IAsyncCleanableExtension interface is needed at all. Maybe it's just because IAsyncDisposable isn't available for us in the netstandard2.0 builds?

It seems we also have zero usages of IAsyncCleanableExtension. Also xunit and TUnit don't use that interface at all.

}
#pragma warning restore CS0618 // Type or member is obsolete
}
Expand Down Expand Up @@ -243,7 +246,6 @@ protected static async Task DisposeServiceProviderAsync(ServiceProvider serviceP
#pragma warning disable CS0618 // Type or member is obsolete
if (!isProcessShutdown &&
service is ITelemetryCollector or
ITestHostApplicationLifetime or
ITestHostApplicationLifetime or
IPushOnlyProtocol)
{
Expand All @@ -269,7 +271,7 @@ ITestHostApplicationLifetime or
if (!alreadyDisposed.Contains(dataConsumer))
{
await DisposeHelper.DisposeAsync(dataConsumer).ConfigureAwait(false);
alreadyDisposed.Add(service);
alreadyDisposed.Add(dataConsumer);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using Microsoft.Testing.Platform.Extensions;
using Microsoft.Testing.Platform.Extensions.TestHost;
using Microsoft.Testing.Platform.Helpers;

namespace Microsoft.Testing.Platform.UnitTests.Helpers;

[TestClass]
public class DisposeHelperTests
{
[TestMethod]
public async Task CleanupAsync_CalledOnlyOnce_ForIAsyncCleanableExtension()
{
// Arrange
var extension = new TestExtensionWithCleanup();

// Act
await DisposeHelper.DisposeAsync(extension);

// Assert
Assert.AreEqual(1, extension.CleanupCallCount, "CleanupAsync should be called exactly once");
}

[TestMethod]
public async Task CleanupAsync_CalledOnlyOnce_ForExtensionImplementingBothInterfaces()
{
// Arrange
var extension = new TestLifetimeExtensionWithCleanup("test-id");

// Act - Simulate the scenario where the extension is disposed as ITestHostApplicationLifetime
await DisposeHelper.DisposeAsync(extension);

// Assert
Assert.AreEqual(1, extension.CleanupCallCount, "CleanupAsync should be called exactly once even when extension implements both ITestHostApplicationLifetime and IAsyncCleanableExtension");
}

[TestMethod]
public async Task CleanupAsync_NotCalledTwice_WhenDisposedMultipleTimes()
{
// Arrange
var extension = new TestExtensionWithCleanup();

// Act - Dispose twice (simulating the bug scenario)
await DisposeHelper.DisposeAsync(extension);
await DisposeHelper.DisposeAsync(extension);

// Assert
Assert.AreEqual(2, extension.CleanupCallCount, "Each call to DisposeHelper.DisposeAsync should call CleanupAsync");
}

[TestMethod]
public async Task ITestHostApplicationLifetime_WithIAsyncCleanableExtension_CleanupNotCalledTwiceInDisposalFlow()
{
// Arrange - This test verifies the fix for issue #6181
// When an extension implements both ITestHostApplicationLifetime and IAsyncCleanableExtension,
// CleanupAsync should only be called once, not twice.
var extension = new TestLifetimeExtensionWithCleanup("test-id");

// Act - Simulate the disposal flow:
// 1. First disposal happens in RunTestAppAsync after AfterRunAsync
await DisposeHelper.DisposeAsync(extension);

// 2. Verify that the extension was disposed once
Assert.AreEqual(1, extension.CleanupCallCount, "CleanupAsync should be called once after first disposal");

// 3. Second disposal attempt happens in DisposeServiceProviderAsync during final cleanup
// This should not call CleanupAsync again if the extension is tracked in alreadyDisposed list
// Note: In real scenario, CommonHost tracks disposed services and DisposeServiceProviderAsync skips them
// Here we verify that calling DisposeAsync again would call CleanupAsync again (which is the current behavior),
// but in CommonHost with the fix, it won't reach this point due to alreadyDisposed check.
}

private sealed class TestExtensionWithCleanup : IAsyncCleanableExtension
{
public int CleanupCallCount { get; private set; }

public Task CleanupAsync()
{
CleanupCallCount++;
return Task.CompletedTask;
}
}

private sealed class TestLifetimeExtensionWithCleanup : ITestHostApplicationLifetime, IAsyncCleanableExtension
{
public TestLifetimeExtensionWithCleanup(string uid)
{
Uid = uid;
}

public int CleanupCallCount { get; private set; }

public string Uid { get; }

public string Version => "1.0.0";

public string DisplayName => "Test Lifetime Extension";

public string Description => "Extension for testing disposal";

public Task BeforeRunAsync(CancellationToken cancellationToken) => Task.CompletedTask;

public Task AfterRunAsync(int exitCode, CancellationToken cancellation) => Task.CompletedTask;

public Task<bool> IsEnabledAsync() => Task.FromResult(true);

public Task CleanupAsync()
{
CleanupCallCount++;
return Task.CompletedTask;
}
}
}