UnitySynchronizationContext deadlocking?

[StephenHodgson]

Seems Unity uses a lock() a delegate/event is invoked which can result in more nightmare bug hunts so it needs to be addressed.

It looks like the lock() is just used for having thread-safe collection access. Use the appropriate System.Collections.Concurrent collection (Queue?) type instead of a lock() statement while still having thread-safe collections.

If exclusive access is required, they might be able to use InterlockedExchange with a Boolean flag that is checked and set thread-safe with the Interlocked helper methods.

The task is to make the WaitForUpdate thread-safe to avoid deadlock situations.

A coworker wrote:

One issue I noticed: await WaitForUpdate() posts to the Unity synchronization context. This shouldn't be a problem, but UnitySynchronizationContext has a major problem in its implementation: it locks the message queue while it's executing messages. This means that if we await WaitForUpdate(), then lock() waiting for some other thread, and that other thread also does await WaitForUpdate(), it'll deadlock because the event queue is locked while the sync context is executing. Bad Unity!

        private void Exec()
        {
            lock (m_AsyncWorkQueue)
            {
                var workCount = m_AsyncWorkQueue.Count;
                for (int i = 0; i < workCount; i++)
                {
                    var work = m_AsyncWorkQueue.Dequeue();
                    work.Invoke();
                }
            }
        }

See the problem there? Rule of thumb: Never call external events or delegates inside of a lock()

[StephenHodgson]

Here's a bug report for unity about this issue

[StephenHodgson]

I was able to fix this issue by keeping my own queue in the coroutine runniner:

    /// <summary>
    /// This Async Coroutine Runner is just a helper object to
    /// ensure that coroutines run properly with async/await.
    /// </summary>
    [ExecuteInEditMode]
    internal class AsyncCoroutineRunner : MonoBehaviour
    {
        private static AsyncCoroutineRunner instance;
        private static Queue<Action> _Actions = new Queue<Action>();

        public event Action OnUpdate;

        internal static AsyncCoroutineRunner Instance
        {
            get
            {
                if (instance == null)
                {
                    instance = FindObjectOfType<AsyncCoroutineRunner>();
                }

                if (instance == null)
                {
                    var instanceGameObject = GameObject.Find("AsyncCoroutineRunner");
                    if (instanceGameObject != null)
                    {
                        instance = instanceGameObject.GetComponent<AsyncCoroutineRunner>();
                        if (instance == null)
                        {
                            Debug.Log("Found object but didn't have component");
                            Destroy(instanceGameObject);
                        }
                    }

                    instance = new GameObject("AsyncCoroutineRunner").AddComponent<AsyncCoroutineRunner>();
                    instance.gameObject.hideFlags = HideFlags.HideInHierarchy;
#if !UNITY_EDITOR
                    DontDestroyOnLoad(instance);
#endif
                }

                return instance;
            }
        }

        internal static void Post(Action task)
        {
            lock (_Actions)
            {
                _Actions.Enqueue(task);
            }
        }

        private void Update()
        {
            Debug.Assert(Instance != null);

            OnUpdate?.Invoke();

            int actionCount;

            lock (_Actions)
            {
                actionCount = _Actions.Count;
            }

            for (int i = 0; i < actionCount; i++)
            {
                Action next;

                lock (_Actions)
                {
                    next = _Actions.Dequeue();
                }

                next();
            }
        }
    }

Here's a repro script:

using System;
using System.Threading;
using System.Threading.Tasks;
using UnityAsyncUtils.Internal;
using UnityEngine;

public class DeadlockRepro : MonoBehaviour
{
    /// <summary>
    /// Ordinarily you'd use a lock(), but in our case the lock is a native critical section.
    /// The compiler doesn't realize there's anything wrong with awaiting inside of this lock.
    /// </summary>
    private readonly Mutex mutex = new Mutex();

    private void Start()
    {
        Debug.Log("Start");
        Task.Run((Action)ProcedureA);
        Task.Run((Action)ProcedureB);
    }

    private void DoBusywork()
    {
        // Simulates some amount of meaningful processing taking place.
        string x = string.Empty;

        for (int i = 0; i < 10; i++)
        {
            x += i.ToString();
            Thread.Sleep(100);
        }

        Debug.Log(x);
    }

    private void ProcedureA()
    {
        if (SynchronizationContext.Current == SyncContextUtility.UnitySynchronizationContext)
        {
            Debug.LogError("Task got executed on the main thread!");
        }

        Debug.Log("Running procedure A on a background thread. Awaiting Update()");
        SyncContextUtility.UnitySynchronizationContext.Post(state => ProcedureAMainThread(), null);
    }

    private void ProcedureAMainThread()
    {
        Debug.Log("Continuing procedure A on the main thread, within the sync context's lock. Doing busywork.");

        // Pass some time doing a little busywork
        DoBusywork();

        Debug.Log("Procedure A busywork complete. Entering lock(b)... (should deadlock)");

        lock (mutex)
        {
            Debug.Log("Procedure A never gets this far, because it deadlocks waiting for lock(b)");
        }
    }

    private void ProcedureB()
    {
        if (SynchronizationContext.Current == SyncContextUtility.UnitySynchronizationContext)
        {
            Debug.LogError("Task got executed on the main thread!");
        }

        lock (mutex)
        {
            Debug.Log("Running Procedure B on a background thread within lock(b)");

            // Do some busywork to pass the time.
            DoBusywork();

            Debug.Log("Procedure B busywork complete. Posting to sync context... (should deadlock)");

            // Post to the Unity sync context
            SyncContextUtility.UnitySynchronizationContext.Post(state =>
            {
                Debug.Log("Procedure B never gets this far, because the call to UnitySynchronizationContext.Post() deadlocks");
            }, null);
        }
    }
}

[StephenHodgson]

Just wanna give an update on this.

Unity is working on a fix.

[StephenHodgson]

Going to close this. It was fixed in 2018.3.3f1 and 2019