dotnet / arcade

Tools that provide common build infrastructure for multiple .NET Foundation projects.
MIT License
667 stars 345 forks source link

Add DisposeAsync to RemoteInvokeHandle #14807

Open liveans opened 4 months ago

liveans commented 4 months ago

Lately, we have discovered that parallelism in xUnit doesn't mean: max X test cases running at the same time, but "X threads executing test cases concurrently", and it was causing us to get timeout on some tests, due to those worker threads on xUnit were blocked by RemoteExecutor.Invoke(..).Dispose();.

So in some cases, while we're awaiting for some method (in our case it was HttpClient.SendAsync), that worker thread is jumping back to xUnit thread pool and xUnit execute another test, if that test includes some Sync operation we're blocked until that thread finishes the test.

Here is an abstract class for repro (thanks to @rzikm):

public abstract class BaseTest
{
    public bool SyncWait { get; set; }

    private async Task DoRun()
    {
        var sw = Stopwatch.StartNew();

        if (SyncWait)
        {
            Thread.Sleep(1000);
        }
        else
        {
            await Task.Delay(1000);
        }

        sw.Stop();
        sw.Restart();

        await Task.Delay(10);

        sw.Stop();
        Assert.True(sw.ElapsedMilliseconds < 100, $"Second delay took {sw.ElapsedMilliseconds}ms");
    }

    [Fact]
    public Task Test1() => DoRun();

    [Fact]
    public Task Test2() => DoRun();

    [Fact]
    public Task Test3() => DoRun();
}

You can create several derived test classes from this abstract class and set half of their SyncWait to true, and you should see the message in some cases like: Second delay took 1000+ms.

To avoid blocking on those threads we're proposing to add DisposeAsync to RemoteInvokeHandle.

We're currently adding this as extension method (https://github.com/dotnet/runtime/pull/102699) to decrease CI noise on our tests, but it would be nice to have a proper DisposeAsync.

/cc @dotnet/ncl

ViktorHofer commented 3 months ago

cc @stephentoub