testfx icon indicating copy to clipboard operation
testfx copied to clipboard

Fix double disposal of ITestHostApplicationLifetime implementing IAsyncCleanableExtension

Open Copilot opened this issue 4 months ago • 2 comments

Fix double call to IAsyncCleanableExtension::CleanupAsync method

Issue: When an MTP extension implements both ITestHostApplicationLifetime and IAsyncCleanableExtension, the CleanupAsync method is called twice - once after AfterRunAsync in RunTestAppAsync and again in DisposeServiceProviderAsync.

Changes Made:

  • [x] Analyze the issue and understand the code flow
  • [x] Track disposed ITestHostApplicationLifetime services in RunTestAppAsync using a field _alreadyDisposedServices
  • [x] Pass the list of already disposed services to DisposeServiceProviderAsync to prevent double disposal
  • [x] Fix redundant duplicate check for ITestHostApplicationLifetime on lines 246-247
  • [x] Fix existing bug where dataConsumer was not being added to alreadyDisposed list (was adding service instead)
  • [x] Add unit tests for DisposeHelper to prevent regression
  • [x] Update tests to use MSTest assertions (Assert.AreEqual)
  • [x] Run code review (completed with no issues)
  • [x] Run security scan (completed with no issues)

Technical Details:

  1. Added _alreadyDisposedServices field to track services that have been disposed in RunTestAppAsync
  2. Modified RunTestAppAsync to add each ITestHostApplicationLifetime service to _alreadyDisposedServices after disposing it
  3. Modified RunAsync to pass _alreadyDisposedServices to DisposeServiceProviderAsync
  4. DisposeServiceProviderAsync already has logic to skip services in the alreadyDisposed list, preventing double disposal
  5. Removed duplicate ITestHostApplicationLifetime check in the service disposal condition
  6. Fixed pre-existing bug where dataConsumer was not being tracked in alreadyDisposed list (line 274)
  7. Added comprehensive unit tests in DisposeHelperTests.cs to verify disposal behavior and prevent regression
  8. Updated tests to use MSTest assertions following project conventions

Security Summary: No security vulnerabilities were introduced by these changes. CodeQL analysis found no issues.

Original prompt

This section details on the original issue you should resolve

<issue_title>IAsyncCleanableExtension::CleanupAsync method is called twice</issue_title> <issue_description>## Describe the bug When MTP extension implements both ITestHostApplicationLifetime/IAsyncCleanableExtension. IAsyncCleanableExtension::CleanupAsync method seems to be called twice.

It's confirmed it can reproduced on version 2.0.0-preview.25372.6. And it also reproduced with version 1.7.2 (Using ITestApplicationLifecycleCallbacks)

Steps To Reproduce

1. Create console application 2. Reference Microsoft.Testing.Platform 2.0.0-preview.25372.6 package 3. Paste following test code and run console app

using Microsoft.Testing.Platform.Builder;
using Microsoft.Testing.Platform.Capabilities.TestFramework;
using Microsoft.Testing.Platform.Extensions;
using Microsoft.Testing.Platform.Extensions.Messages;
using Microsoft.Testing.Platform.Extensions.TestFramework;
using Microsoft.Testing.Platform.Extensions.TestHost;
using Microsoft.Testing.Platform.Requests;

namespace TestConsoleApp;

#pragma  warning disable TPEXP

internal class Program
{
    static async Task<int> Main(string[] args)
    {
        var builder = await TestApplication.CreateBuilderAsync(args);

        var testFramework = new DummyTestAdapter();
        builder.RegisterTestFramework(_ => new TestFrameworkCapabilities(), (_, _) => testFramework);

        var myExtension = new MyExtension(testFramework);

        builder.TestHost.AddTestHostApplicationLifetime(_ => myExtension);
        // builder.TestHost.AddDataConsumer(_ => myExtension);

        var app = await builder.BuildAsync();
        return await app.RunAsync();
    }
}

internal class MyExtension(IExtension extension) : ITestHostApplicationLifetime, IAsyncCleanableExtension
{
    public Task<bool> IsEnabledAsync() => extension.IsEnabledAsync();
    public string Uid => extension.Uid;
    public string Version => extension.Version;
    public string DisplayName => extension.DisplayName;
    public string Description => extension.Description;

    public Task BeforeRunAsync(CancellationToken cancellationToken)
    {
        Console.WriteLine("BeforeRunAsync");
        return Task.CompletedTask;
    }

    public Task AfterRunAsync(int exitCode, CancellationToken cancellation)
    {
        Console.WriteLine("AfterRunAsync");
        return Task.CompletedTask;
    }

    public Task CleanupAsync()
    {
        Console.WriteLine("CleanupAsync");
        return Task.CompletedTask;
    }
}

internal class DummyTestAdapter : ITestFramework, IDataProducer
{
    public Task<bool> IsEnabledAsync() => Task.FromResult(true);
    public string Uid => nameof(DummyTestAdapter);
    public string Version => string.Empty;
    public string DisplayName => string.Empty;
    public string Description => string.Empty;

    public Type[] DataTypesProduced => [typeof(TestNodeUpdateMessage)];
    public Task<CloseTestSessionResult> CloseTestSessionAsync(CloseTestSessionContext context) => Task.FromResult(new CloseTestSessionResult { IsSuccess = true });
    public Task<CreateTestSessionResult> CreateTestSessionAsync(CreateTestSessionContext context) => Task.FromResult(new CreateTestSessionResult { IsSuccess = true });

    public async Task ExecuteRequestAsync(ExecuteRequestContext context)
    {
        switch (context.Request)
        {
            case DiscoverTestExecutionRequest discoverRequest:
            case RunTestExecutionRequest runRequest:
                await Task.Yield();
                //await HandleRequest(context);
                break;
            default:
                break;
        }

        context.Complete();
    }
}

Expected behavior

CleanupAsync is expected to be called exactly once. Because CleanupAsync method has no special note that it may be called multiple times like Dispose/DisposeAsync.

Actual behavior

CleanupAsync log outputted twice..

Additional context

It seems to be disposed at following lines.

  • https://github.com/microsoft/testfx/blob/v4.0.0-preview.25372.6/src/Platform/Microsoft.Testing.Platform/Hosts/CommonTestHost.cs#L123
  • https://github.com/microsoft/testfx/blob/v4.0.0-preview.25372.6/src/Platform/Microsoft.Testing.Platform/Hosts/CommonTestHost.cs#L256

Additionally, Following lines seems redundant because it checks same types. https://github.com/microsoft/testfx/blob/v4.0.0-preview.25372.6/src/Platform/Microsoft.Testing.Platform/Hosts/CommonTestHost.cs#L246-L247</issue_description>

Comments on the Issue (you are @copilot in this section)

  • Fixes microsoft/testfx#6181

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot avatar Oct 26 '25 18:10 Copilot

@copilot let's add a test so we don't regress

Added unit tests in DisposeHelperTests.cs to verify that CleanupAsync is called correctly and to prevent regression. The tests verify:

  1. Extensions implementing IAsyncCleanableExtension have CleanupAsync called exactly once
  2. Extensions implementing both ITestHostApplicationLifetime and IAsyncCleanableExtension are properly handled
  3. The disposal flow works correctly

Commit: 2253eaf

[!WARNING]

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • 7tjvsblobprodcus341.vsblob.vsassets.io
    • Triggering command: /home/REDACTED/work/testfx/testfx/.dotnet/dotnet msbuild /m /nologo /clp:Summary /v:minimal /nr:true /warnaserror /p:TreatWarningsAsErrors=true /p:ContinuousIntegrationBuild=false /home/REDACTED/work/testfx/testfx/artifacts/toolset/restore.proj /t:__WriteToolsetLocation /clp:ErrorsOnly;NoSummary /p:__ToolsetLocationOutputFile=/home/REDACTED/work/testfx/testfx/artifacts/toolset/11.0.0-beta.25524.1.txt (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot avatar Oct 26 '25 19:10 Copilot

Moving back to draft for manual validation.

Evangelink avatar Oct 30 '25 08:10 Evangelink