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

Rework ReturnValueValidator and ExecutionValidator #2114

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

YegorStepanov
Copy link
Contributor

@YegorStepanov YegorStepanov commented Sep 21, 2022

Each benchmark is run on the new instance.

ReturnValueValidator:

  • Compare Task value instead task objects itself

@YegorStepanov
Copy link
Contributor Author

YegorStepanov commented Sep 25, 2022

Deadlock in tests. I cannot reproduce it on local machine (Windows). I ran them few times, the deadlock happens every time.

Github Actions: deadlock on Windows/Linux. MacOS is ok
Azure Pipelines: deadlock on Linux. Window/MacOS is ok
AppVeyor: deadlock

I need to execute a method via Reflection, await it (if the method is async: Task or ValueTask) and return result (if the Task is not void).

MethodInfo benchmarkMethod = ...
var result = benchmarkMethod.Invoke(benchmarkClassInstance, arguments); // (arguments.Any() ? arguments : null) not helped

if (TryAwaitTask(result, out var taskResult))
    result = taskResult;

// returns true if the method is async
private static bool TryAwaitTask(object task, out object result)
{
    result = null;

    if (task is null)
    {
        return false;
    }
    
    // ValueTask<T>
    var returnType = task.GetType();
    if (returnType.IsGenericType && returnType.GetGenericTypeDefinition() == typeof(ValueTask<>))
    {
        var asTaskMethod = task.GetType().GetMethod("AsTask");
        task = asTaskMethod.Invoke(task, null);
    }

    if (task is ValueTask valueTask)
        task = valueTask.AsTask();
    
    // Task or Task<T>
    if (task is Task t)
    {
        if (TryGetTaskResult(t, out var taskResult))
            result = taskResult;

        return true;
    }

    return false;
}

// https://stackoverflow.com/a/52500763
private static bool TryGetTaskResult(Task task, out object result)
{
    result = null;

    var voidTaskType = typeof(Task<>).MakeGenericType(Type.GetType("System.Threading.Tasks.VoidTaskResult"));
    if (voidTaskType.IsInstanceOfType(task))
    {
        task.GetAwaiter().GetResult();
        return false;
    }

    var property = task.GetType().GetProperty("Result", BindingFlags.Public | BindingFlags.Instance);
    if (property is null)
    {
        return false;
    }

    result = property.GetValue(task);
    return true;
}

The deadlock occurs on executing these async methods in parallel

public class AsyncGenericValueTaskGlobalSetup
{
    [GlobalSetup]
    public async ValueTask<int> GlobalSetup()
    {
        await Task.Delay(1);
        return 42;
    }
}

public class AsyncSetupIsSupportedClass
{
    [Benchmark]
    public async Task<int> Foo()
    {
        await Task.Delay(1);
        return 1;
    }
}

No one else uses the task.

valueTask.AsTask() creates a new normal task? valueTask.AsTask().Result is a correct way to sync wait?

@stephentoub, only You know what's going on here. Maybe it's a BLC bug?

@timcassell
Copy link
Collaborator

timcassell commented Sep 26, 2022

I don't see anything wrong with that code. But I just updated my AwaitHelper in #2108 to work for multiple threads, and I wonder if you tried the same thing if it would make a difference.

private static bool TryAwaitTask(object task, out object result)
{
    result = null;
    if (task is null)
    {
        return false;
    }
    var getResultMethod = AwaitHelper.GetGetResultMethod(task.GetType());
    if (getResultMethod is null)
    {
        return false;
    }
    result = getResultMethod.Invoke(null, new[] { task });
    return true;
    // Or
    // return getResultMethod.ReturnType != typeof(void);
}

@YegorStepanov
Copy link
Contributor Author

Hey, Tim! I tried your solution, deadlock still exists.

Changing ValueTask to Task doesn't help.
I tried to lock invocation, but it didn't help. Although a new object is created on each ExecutionValidatorBase.Validate call. And each test method is isolated.

The deadlock occurs on execution ExecutionValidatorTests and ReturnValueValidatorTests in parallel. If I remove all async methods from any of these files - deadlock is done.

I deleted all other tests except these and all sync methods in these tests - the situation is the same.

I think the problem is due to the version of .NET Framework (net461) or with XUnit.

@YegorStepanov
Copy link
Contributor Author

YegorStepanov commented Sep 28, 2022

What I did...

Summary:

  • Try Tim's multithreaded AwaitHelper.GetGetResultMethod()
  • Change ValueTask to Task
  • Add lock to both validators and to their base class
  • Bumped .Net Framework from 4.61/4.62 to 4.8 in solution
  • Bumped .Net Core from netcoreapp2.1/net6.0 to 6.0 in solution
  • Updated xUnit packages
  • Updated Microsoft.NET.Test.Sdk, Microsoft.NETCore.Platforms, Microsoft.NETFramework.ReferenceAssemblies
  • Updated VM Ubuntu-20.04 to Ubuntu-latest
  • Updated VM Windows-20.04 to Windows-latest

Still deadlock on Windows/Linux

What works:

  • Remove all "async" tests from any of these classes
  • Add [Collection("Disable parallelism")] to both test classes to disable parallelism.

I hope it's a xUnit issue. I will send it to them.

@timcassell
Copy link
Collaborator

timcassell commented Sep 29, 2022

I'm curious why the deadlock only happens with your changes, and not in the existing tests that do the same blocking wait (just without reading the result). By deduction, it must be some other change causing it, no?

@YegorStepanov
Copy link
Contributor Author

YegorStepanov commented Sep 29, 2022

Currently, these validators don't await async methods. They simple call Invoke:

MethodInfo benchmarkMethod = ...;
benchmarkMethod.Invoke(null);

Yep, it's a xUnit bug, msTest and nUnit are completed successfully.

I'll send a bug report in a few hours.

@YegorStepanov
Copy link
Contributor Author

The bug is unlikely to be fixed anytime soon.

@YegorStepanov YegorStepanov marked this pull request as ready for review October 14, 2022 11:21
@timcassell
Copy link
Collaborator

You added validation here to make sure IterationSetup/Cleanup are not async. I actually added async support for them in #2111, and this seems likely to get merged first, so I'll have to remember to update the validator there.

@timcassell
Copy link
Collaborator

I wonder if that xUnit bug is the cause of the AllSetupAndCleanupTest flakiness. 🤔

@YegorStepanov
Copy link
Contributor Author

You added validation here to make sure IterationSetup/Cleanup are not async. I actually added async support for them in #2111, and this seems likely to get merged first, so I'll have to remember to update the validator there.

Okay, I'll keep an eye on it too.
Also you can ping me because I want to move more checks from BenchmarkConverter to the validators and remove duplicate checks from these PR's classes.

I hope your PRs will be accepted before the Unity moves to .NET Core. Just two years left to wait 😭

@YegorStepanov
Copy link
Contributor Author

I wonder if that xUnit bug is the cause of the AllSetupAndCleanupTest flakiness. 🤔

I didn't even think why some tests are flakiness.
These tests never fail locally, only on the VM?

Hmm, it is a good idea to check it, it not hard (I don't want to do it 😄):

  1. add new project
  2. copy flakiness tests
  3. update xUnit assertion to NUnit/MsUnit
  4. Start spamming in the fork

@timcassell
Copy link
Collaborator

I hope your PRs will be accepted before the Unity moves to .NET Core. Just two years left to wait 😭

Yeah it would be nice to unblock async engine support, but what does it have to do with Core in Unity?

@adamsitnik
Copy link
Member

I've observed similar CI issues in the past in other projects. What is unique about most CI machines is that they have only one or two cores, so getting into a deadlock is way easier. You could try to reproduce it locally by running the tests affinized to 1 or 2 cores.

@YegorStepanov
Copy link
Contributor Author

I hope your PRs will be accepted before the Unity moves to .NET Core. Just two years left to wait 😭

Yeah it would be nice to unblock async engine support, but what does it have to do with Core in Unity?

Since you are writing a library for Unity, I thought it was related to Unity.

Anyway, we need to support taskable object to support UniTask and UnityEngine.Awaitable/UnityTask without converting them to Task.

To do this, we need to slightly modify your AwaitHelper

This was referenced Oct 19, 2022
@snowfrogdev
Copy link

@YegorStepanov any plans for this?

@timcassell
Copy link
Collaborator

@snowfrogdev Yegor is MIA, feel free to take over here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants