Task.Run inside a test will result in deadlock if a control was created previously

[costin-zaharia]

Hi,

I'm not sure whether an issue with NUnit but the following test will always result in a deadlock:

[Test]
public async Task Deadlock()
{
    new Control();

    await Task.Run(() => { int x = 42; });
}

Version used: 3.6.1

Thanks,
Costin

[jnm2]

Are you using the Apartment attribute to make sure you're running the GUI code on an STA thread?

[costin-zaharia]

I tried with ApartmentState.STA but I get the same result:

[Test]
[Apartment(ApartmentState.STA)]
public async Task Deadlock()
{
    new Control();

    await Task.Run(() => { int x = 42; });
}

[jnm2]

Okay, I see what's going on. This is not NUnit-related.

What's happening is that new Control() is equivalent to doing this:

public async Task Deadlock()
{
    SynchronizationContext.SetSynchronizationContext(new WindowsFormsSynchronizationContext());

    await Task.Run(() => { });
}

await works by taking the current synchronization context and posting to it (SynchronizationContext.Post) to continue the method. For WindowsFormsSynchronizationContext, this means it sends a windows message to the message loop. The problem is that you aren't running a message loop (Application.Run() / Application.Exit()):

public async Task Deadlock()
{
    SynchronizationContext.SetSynchronizationContext(new WindowsFormsSynchronizationContext());

    await Task.Run(() => { });
    // We never get here because the async/await system is waiting for the message loop
    // to run the continuation on the STA thread
}

[jnm2]

If you are not going to run any GUI code, which needs STA, this is one solution:

[Test]
[Apartment(ApartmentState.STA)]
public async Task Deadlock()
{
    new Control();

    await Task.Run(() => { }).ConfigureAwait(false); // Continue on a threadpool thread instead of the current synchronization context

    // No GUI code here!
}

Another solution:

[Test]
[Apartment(ApartmentState.STA)]
public async Task Deadlock()
{
    new Control();
    SynchronizationContext.SetSynchronizationContext(null);

    await Task.Run(() => { });

    // No GUI code here!
}

[costin-zaharia]

Hi jnm2

Awesome explanation :)

Thank you very much!

[jnm2]

No problem! Here's something I might do:

[Test]
[Apartment(ApartmentState.STA)]
public void NoDeadlock()
{
    Utils.RunMessageLoop(async () =>
    {
        using (var control = new Control())
        {
            await Task.Run(() => { });

            control.Text = "42";
        }
    });
}

public static class Utils
{
    public static void RunMessageLoop(Func<Task> action)
    {
        var originalContext = SynchronizationContext.Current;
        try
        {
            SynchronizationContext.SetSynchronizationContext(new WindowsFormsSynchronizationContext());

            action.Invoke().ContinueWith(t => Application.Exit(), TaskScheduler.FromCurrentSynchronizationContext());

            Application.Run();
        }
        finally
        {
            SynchronizationContext.SetSynchronizationContext(originalContext);
        }
    }
}

[jnm2]

@nunit/framework-team It would be cool if you could implement your own AsyncInvocationRegion. That would allow running the message pump via attribute:

[Test]
[WindowsFormsSTAMessageLoop]
public async Task NoDeadlock()
{
    using (var control = new Control())
    {
        await Task.Run(() => { });

        control.Text = "42";
    }
}

Right now implementation of this attribute is impossible because TestMethodCommand.Execute calls AsyncTaskInvocationRegion.WaitForPendingOperationsToComplete which blocks, so the message pump can't run.

[jnm2]

Well hey guess what! I learned that you can hook any blocking wait by overriding SynchronizationContext.Wait, so overriding AsyncTaskInvocationRegion.WaitForPendingOperationsToComplete is not necessary.

This works perfectly. It has no downsides that I am aware of:

[Test]
[WindowsFormsSTAMessageLoop]
public async Task NoDeadlock()
{
    using (var control = new Control())
    {
        await Task.Run(() => { });

        control.Text = "42";
    }
}


public sealed class WindowsFormsSTAMessageLoopAttribute : PropertyAttribute, IWrapSetUpTearDown
{
    public WindowsFormsSTAMessageLoopAttribute()
    {
        Properties.Add("ApartmentState", ApartmentState.STA);
    }

    public TestCommand Wrap(TestCommand command) => new RunOnMessageLoopCommand(command);

    private sealed class RunOnMessageLoopCommand : DelegatingTestCommand
    {
        public RunOnMessageLoopCommand(TestCommand innerCommand) : base(innerCommand)
        {
        }

        public override TestResult Execute(TestExecutionContext context)
        {
            var originalContext = SynchronizationContext.Current;
            try
            {
                SynchronizationContext.SetSynchronizationContext(new WindowsFormsMessagePumpingSynchronizationContext());
                return innerCommand.Execute(context);
            }
            finally
            {
                SynchronizationContext.SetSynchronizationContext(originalContext);
            }
        }

        private sealed class WindowsFormsMessagePumpingSynchronizationContext : SynchronizationContext
        {
            private readonly WindowsFormsSynchronizationContext windowsFormsContext = new WindowsFormsSynchronizationContext();

            public WindowsFormsMessagePumpingSynchronizationContext()
            {
                SetWaitNotificationRequired();
            }

            public override void Post(SendOrPostCallback d, object state)
            {
                windowsFormsContext.Post(d, state);
            }

            public override void Send(SendOrPostCallback d, object state)
            {
                windowsFormsContext.Send(d, state);
            }

            public override int Wait(IntPtr[] waitHandles, bool waitAll, int millisecondsTimeout)
            {
                var arrayHandle = GCHandle.Alloc(waitHandles, GCHandleType.Pinned);
                try
                {
                    var startTick = Environment.TickCount;
                    var messageAvailableResult = WAIT.OBJECT_0 + (uint)waitHandles.Length;

                    while (true)
                    {
                        var timeout = Timeout.Infinite == millisecondsTimeout ? Timeout.Infinite :
                            Math.Max(0, millisecondsTimeout + startTick - Environment.TickCount);

                        var result = MsgWaitForMultipleObjectsEx(waitHandles.Length, arrayHandle.AddrOfPinnedObject(), timeout, QS.ALLINPUT, MWMO.INPUTAVAILABLE & (waitAll ? MWMO.WAITALL : 0));
                        if (result == WAIT.FAILED) throw new Win32Exception();

                        if (result != messageAvailableResult) return (int)result;
                        // Message is available

                        // Prefer signal or abandonment (and do COM message processing)
                        var noWaitResult = base.Wait(waitHandles, waitAll, 0);
                        if (noWaitResult != (int)WAIT.TIMEOUT) return noWaitResult;

                        // Then prefer to time out
                        if (millisecondsTimeout != Timeout.Infinite && (uint)startTick + (uint)millisecondsTimeout <= (uint)Environment.TickCount)
                            return (int)WAIT.TIMEOUT;

                        // No signal, abandonment, or timeout, so process messages
                        Application.DoEvents();
                    }
                }
                finally
                {
                    arrayHandle.Free();
                }
            }

            #region Native methods
            // ReSharper disable InconsistentNaming

            [DllImport("user32.dll", SetLastError = true)]
            private static extern WAIT MsgWaitForMultipleObjectsEx(int nCount, IntPtr pHandles, int dwMilliseconds, QS dwWakeMask, MWMO dwFlags);

            private enum QS : uint
            {
                ALLINPUT = 0x04FF
            }

            [Flags]
            private enum MWMO : uint
            {
                WAITALL = 0x1,
                INPUTAVAILABLE = 0x4
            }

            private enum WAIT : uint
            {
                OBJECT_0 = 0,
                ABANDONED_0 = 0x80,
                TIMEOUT = 0x102,
                FAILED = uint.MaxValue
            }

            // ReSharper restore InconsistentNaming
            #endregion
        }
    }
}

[CharliePoole]

@jnm2 Just wanted to point out that "you" addressed to the framework team actually includes you!

If you feel like there is something we should do to facilitate the solution, you should re-open and perhaps re-title this issue, then do a PR. 😄

One detail... should this really wrap setup/teardown or should it wrap the test method only?

[jnm2]

Just wanted to point out that "you" addressed to the framework team actually includes you!

Well, it was everyone else I was talking to! 😆

As usual, I just wanted to elicit feedback. I'm not sure if this is a problem everyone thinks we should solve.

The drawback to the last solution above is that it relies on hooking waits which introduces overhead everywhere. Ideally we'd be able to implement our own AsyncInvocationRegion which the attribute would have to have some way of indicating, or perhaps expose a TestCommand.ExecuteAsync. But those both require some framework API design.

should this really wrap setup/teardown or should it wrap the test method only?

If the day ever comes when you support async setup and teardown methods, then we need it this way to prevent deadlocks.

[CharliePoole]

I know that the issue of running on the message loop has come up in the past. The overhead of your approach is limited to tests that use the attribute, so I don't see that as a big issue.

So yes, let's get some feedback from @nunit/framework-team and @nunit/core-team !