Rework SafeTimespanGuarantor and fix issue where exiting XR can get stuck for random amounts of time

Core/Graphics/Source/SafeTimespanGuarantor.cpp

@@ -3,36 +3,68 @@
 namespace Babylon
 {
     SafeTimespanGuarantor::SafeTimespanGuarantor()
-        : m_lock{m_mutex}
+        : m_affinity{ std::this_thread::get_id() }
+        , m_lock{ m_mutex }
     {
     }
 
     void SafeTimespanGuarantor::BeginSafeTimespan()
     {
-        m_postCount = 0;
-        m_lock.reset();
+        if (!m_affinity.check())
+        {
+            throw std::runtime_error{ "BeginSafeTimespan must be called from the thread on which the SafeTimespanGuarantor was constructed." };
+        }
+
+        if (!m_lock.owns_lock())
+        {
+            throw std::runtime_error{ "EndSafeTimespan must be called before BeginSafeTimespan can be called again." };
+        }
+
+        // First unlock the underlying mutex, which allows calls to GetSafetyGuarentee to acquire a SafetyGuarentee.
+        m_lock.unlock();
+
+        // Then yield to ensure calls to GetSafetyGuarentee get a chance to lock on the mutex before EndSafeTimeSpan takes the lock (e.g. prevent starvation).
+        std::this_thread::yield();
     }
 
     void SafeTimespanGuarantor::EndSafeTimespan()
     {
-        bool wait{false};
-        do
+        if (!m_affinity.check())
         {
-            m_lock.emplace(m_mutex);
-            wait = m_postCount > 0;
-            --m_postCount;
+            throw std::runtime_error{ "EndSafeTimespan must be called from the thread on which the SafeTimespanGuarantor was constructed." };
+        }
 
-            if (wait)
-            {
-                m_lock.reset();
-            }
-        } while (wait && m_semaphore.wait());
+        if (m_lock.owns_lock())
+        {
+            throw std::runtime_error{ "BeginSafeTimespan must be called before EndSafeTimespan can be called." };
+        }
+
+        // First lock on the underlying mutex.
+        m_lock.lock();
+
+        // Then wait for the count of outstanding SafeteyGuarentees to reach zero.
+        // If the condition is not met, the underlying mutex is unlocked, but we still block on the condition variable, waiting to be signated to recheck the condition.
+        // Once the condition is met, then the condition variable unblocks, but the lock on the underlying mutex (re-acquired when checking the condition) is retained.
+        m_condition.wait(m_lock, [this]{ return m_count == 0; });
     }
 
     SafeTimespanGuarantor::SafetyGuarantee SafeTimespanGuarantor::GetSafetyGuarantee()
     {
-        std::scoped_lock lock{m_mutex};
-        ++m_postCount;
-        return m_semaphore.GetPostFinalAction();
+        // First lock on the underlying mutex and increment the outstanding SafeteyGuarantee count.
+        std::lock_guard<std::mutex> guard(m_mutex);
+        m_count++;
+
+        // Then return a SafeteyGuarantee that should be held until caller operations are complete.
+        return gsl::finally(std::function<void()>{ [this]
+        {
+            // First lock the underlying mutex and decrement the outstanding SafeteyGuarantee count.
+            {
+                std::lock_guard<std::mutex> guard(m_mutex);
+                m_count--;
+            }
+
+            // Then signal the condition variable to recheck the condition.
+            m_condition.notify_one();
+        }});
     }
 }

Core/Graphics/Source/SafeTimespanGuarantor.h

@@ -1,25 +1,13 @@
 #pragma once
 
-#include <bx/semaphore.h>
-
 #include <gsl/gsl>
 
 #include <mutex>
-#include <optional>
+
+#include <arcana/threading/affinity.h>
 
 namespace Babylon
 {
-    class Semaphore : public bx::Semaphore
-    {
-    public:
-        auto GetPostFinalAction()
-        {
-            return gsl::finally([this]() {
-                post();
-            });
-        }
-    };
-
     class SafeTimespanGuarantor
     {
     public:
@@ -28,13 +16,14 @@ namespace Babylon
         void BeginSafeTimespan();
         void EndSafeTimespan();
 
-        using SafetyGuarantee = decltype(std::declval<Semaphore>().GetPostFinalAction());
+        using SafetyGuarantee = gsl::final_action<std::function<void()>>;
         SafetyGuarantee GetSafetyGuarantee();
 
     private:
-        Semaphore m_semaphore{};
-        size_t m_postCount{};
+        arcana::affinity m_affinity{};
+        uint32_t m_count{};
         std::mutex m_mutex{};
-        std::optional<std::scoped_lock<std::mutex>> m_lock{};
+        std::unique_lock<std::mutex> m_lock{};
+        std::condition_variable m_condition{};
     };
 }

Plugins/NativeXr/Source/NativeXr.cpp

@@ -434,7 +434,7 @@ namespace Babylon
         m_textureToFrameBufferMap.clear();
         m_activeTextures.clear();
 
-        return m_frameTask.then(m_graphicsImpl.AfterRenderScheduler(), arcana::cancellation::none(), [this]() {
+        return m_frameTask.then(m_graphicsImpl.AfterRenderScheduler(), arcana::cancellation::none(), [this, thisRef{shared_from_this()}](const arcana::expected<void, std::exception_ptr>&) {
             assert(m_session != nullptr);
             assert(m_frame == nullptr);
 
@@ -479,7 +479,7 @@ namespace Babylon
                 }
 
                 EndUpdate();
-            }).then(m_graphicsImpl.AfterRenderScheduler(), arcana::cancellation::none(), [this, thisRef{shared_from_this()}](arcana::expected<void, std::exception_ptr>) {
+            }).then(m_graphicsImpl.AfterRenderScheduler(), arcana::cancellation::none(), [this, thisRef{shared_from_this()}](const arcana::expected<void, std::exception_ptr>&) {
                 EndFrame();
             });
         });