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.

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

Add DisposeAsync to RemoteInvokeHandle #14807

Open
liveans opened this issue May 31, 2024 · 1 comment
Open

Add DisposeAsync to RemoteInvokeHandle #14807

liveans opened this issue May 31, 2024 · 1 comment

Comments

@liveans
Copy link
Member

liveans commented May 31, 2024

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 (dotnet/runtime#102699) to decrease CI noise on our tests, but it would be nice to have a proper DisposeAsync.

/cc @dotnet/ncl

@ViktorHofer
Copy link
Member

cc @stephentoub

@missymessa missymessa added this to the Tracking for other teams milestone Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants