ITestLoggerWithParameters | ITestLogger async issue

[nbalu]

Description

Using the following interface types there's no way that you can garatnee that a service can "finish" it's work if there's no sync alternative.

Event handlers were fine before Tasks but if you would write (async void) anything that will reach the first await will treat the event as returned.

Steps to reproduce

public class ShameOnYouLogger : ITestLoggerWithParameters
{
    public void Initialize(TestLoggerEvents events, Dictionary<string, string?> parameters)
    {
        events.DiscoveryStart += Events_DiscoveryStart;
    }

    private async void Events_DiscoveredTests(object? sender, DiscoveredTestsEventArgs e)
    {
        var message = JsonConvert.SerializeObject(e);

        await Task.Delay(10000);
        Console.WriteLine("This will never show up if your descovery not takes at least 10 sec");
    }
}

Expected behavior

Console should be reached.

Actual behavior

Won't be logging.

Diagnostic logs

⏹️

Environment

🌎

Proposal

There are couple of ways to solve it:

• Interface should be marked as IDisposable + IAsyncDisposable and the loggers should be disposed along with the EventHandler
• Introduce a new abstract class to introduce "calls" to both sync / async function instead of dispatching events (UI obsession) like:
  virtual void DiscoveredTests(DiscoveredTestsEventArgs e) { return; }
  virtual Task DiscoveredTests(DiscoveredTestsEventArgs e) { return Task.Completed; }

Or feel free to come up with any proper solution you might like. But this is not a very fortunate situation...

[nohwnd]

Is there a goal that you are trying to achieve? We cannot redesign the whole way vstest is done, and it was written in a time where async hardly existed as a construct in C#.

[nbalu]

I'd like use a lib that I'd like to trigger on messages. But the API surface only supports async methods.

I'm not saying that you should redesign it. Though I would recommend to introduce a new interface which can be used for extensibility that supports async methods. This should apply to Collectors / Adapters / Discoverer / Executer and whatever extensibility point exists within vstest.

For now I would say it would be sufficient to have a Listener upgraded for my case for now... But I would be happy to see that the lookup would look for additional new generation types that are capable to handle async workloads.

From implementation perspective I don't think it would be a huge deal to introduce these types.

[nohwnd]

Unfortunately that is not so easy. The design where commands (as method calls) flow one way and responses flow as events the other way is common in the whole codebase. For your approach we would have to switch to a message bus or something and then "emulate" the events. I would be up for it, but it would have to be used extensively throughout the whole codebase, and this big refactoring is not something we want to do right now.

[nbalu2]

I don't think it's something that it's required at least not for the logger scenario.

When getting the implementaion of logger in TestLoggerManager.cs why don't you check for another interface IDisposable() for discovery. If the logger implements it then call it once the testmanager disposes.

The implementation could use that to wait for the internal tasks to be completed (which is not nice) though could be leveraged to add a Task.Wait() on the collected async events that we need to wait for.

I know it's ugly but the implementations do not require synchronization context so deadlock can be avoided.

---

Or back to my suggestion. Within InternalTestManager you have full control of triggering the events like: LoggerManager.HandleDiscoveredTests(discoveredTestsEvent);
If you introduce a new type like IAsyncTestLogger with a method of:
Task HandleDiscoveredTests(DiscoveredTestsEventArgs e) { return Task.Completed; }
etc.

It can have a default implementation which can be used to wrap the older types to be "backward" compatible like:

/// Partial implementation
public AsyncProxyTestListener : IAsyncTestListener
{
    private readonly ITestListener original;
    
    public AsyncProxyTestListener(ITestListener original)
    {
        this.original = original;
    }

    Task HandleDiscoveredTests(DiscoveredTestsEventArgs e) {
        original.HandleDiscoveredTests(e);
        return Task.Completed; 
    }
}

Then instead of triggering the event call the async method which needs to be waited but it's fine to have LoggerManager.HandleDiscoveredTests(discoveredTestsEvent).ConfigureAwait(false).GetAwaiter().GetResult(); there IMHO.

What do you think?

PS.:
If you don't want to make things sync you can still collect the created tasks and use the DiscoveryRequest _discoveryCompleted manualreset event to only be triggered if the tasks are completed by counting outstanding tasks and on latest task completion trigger the event (or something like that 😆). The same should apply to the TestRunRequest with _runCompletionEvent.

[nohwnd]

Thanks for the suggestions. We can come up with a lot of workarounds, but I would like to avoid introducing a one off solutions that are not building on a solid foundation. The internal architecture is different and does not mesh well with async workloads.