From 6b43ce066294c058de4b1c2908f8f60f8d59fd04 Mon Sep 17 00:00:00 2001
From: elsid <elsid.mail@gmail.com>
Date: Wed, 24 Nov 2021 19:10:02 +0100
Subject: [PATCH] Fix deadlock in physics system

1. Reorder unlock and notify_all calls to avoid notifying when not all worker
   threads are waiting.
2. Make sure main thread does not attempt to exclusively lock mSimulationMutex
   while not all workers are done with previous frame.
3. Replace mNewFrame flag by counter to avoid modification from multiple
   threads.
---
 apps/openmw/mwphysics/mtphysics.cpp | 39 ++++++++++++++++++++++++-----
 apps/openmw/mwphysics/mtphysics.hpp |  7 +++++-
 2 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/apps/openmw/mwphysics/mtphysics.cpp b/apps/openmw/mwphysics/mtphysics.cpp
index 5c49dee297..2f7e3b5995 100644
--- a/apps/openmw/mwphysics/mtphysics.cpp
+++ b/apps/openmw/mwphysics/mtphysics.cpp
@@ -267,7 +267,7 @@ namespace MWPhysics
           , mNumJobs(0)
           , mRemainingSteps(0)
           , mLOSCacheExpiry(Settings::Manager::getInt("lineofsight keep inactive cache", "Physics"))
-          , mNewFrame(false)
+          , mFrameCounter(0)
           , mAdvanceSimulation(false)
           , mQuit(false)
           , mNextJob(0)
@@ -302,13 +302,14 @@ namespace MWPhysics
 
     PhysicsTaskScheduler::~PhysicsTaskScheduler()
     {
+        waitForWorkers();
         {
             MaybeExclusiveLock lock(mSimulationMutex, mNumThreads);
             mQuit = true;
             mNumJobs = 0;
             mRemainingSteps = 0;
+            mHasJob.notify_all();
         }
-        mHasJob.notify_all();
         for (auto& thread : mThreads)
             thread.join();
     }
@@ -361,6 +362,8 @@ namespace MWPhysics
 
     void PhysicsTaskScheduler::applyQueuedMovements(float & timeAccum, std::vector<Simulation>&& simulations, osg::Timer_t frameStart, unsigned int frameNumber, osg::Stats& stats)
     {
+        waitForWorkers();
+
         // This function run in the main thread.
         // While the mSimulationMutex is held, background physics threads can't run.
 
@@ -393,7 +396,7 @@ namespace MWPhysics
         mPhysicsDt = newDelta;
         mSimulations = std::move(simulations);
         mAdvanceSimulation = (mRemainingSteps != 0);
-        mNewFrame = true;
+        ++mFrameCounter;
         mNumJobs = mSimulations.size();
         mNextLOS.store(0, std::memory_order_relaxed);
         mNextJob.store(0, std::memory_order_release);
@@ -421,6 +424,7 @@ namespace MWPhysics
 
     void PhysicsTaskScheduler::resetSimulation(const ActorMap& actors)
     {
+        waitForWorkers();
         MaybeExclusiveLock lock(mSimulationMutex, mNumThreads);
         mBudget.reset(mDefaultPhysicsDt);
         mAsyncBudget.reset(0.0f);
@@ -577,11 +581,15 @@ namespace MWPhysics
 
     void PhysicsTaskScheduler::worker()
     {
+        std::size_t lastFrame = 0;
         std::shared_lock lock(mSimulationMutex);
         while (!mQuit)
         {
-            if (!mNewFrame)
-                mHasJob.wait(lock, [&]() { return mQuit || mNewFrame; });
+            if (lastFrame == mFrameCounter)
+            {
+                mHasJob.wait(lock, [&] { return mQuit || lastFrame != mFrameCounter; });
+                lastFrame = mFrameCounter;
+            }
 
             doSimulation();
         }
@@ -663,6 +671,7 @@ namespace MWPhysics
 
     void PhysicsTaskScheduler::releaseSharedStates()
     {
+        waitForWorkers();
         std::scoped_lock lock(mSimulationMutex, mUpdateAabbMutex);
         mSimulations.clear();
         mUpdateAabb.clear();
@@ -693,7 +702,6 @@ namespace MWPhysics
 
     void PhysicsTaskScheduler::afterPostSim()
     {
-        mNewFrame = false;
         {
             MaybeExclusiveLock lock(mLOSCacheMutex, mNumThreads);
             mLOSCache.erase(
@@ -702,6 +710,10 @@ namespace MWPhysics
                     mLOSCache.end());
         }
         mTimeEnd = mTimer->tick();
+
+        std::unique_lock lock(mWorkersDoneMutex);
+        ++mWorkersFrameCounter;
+        mWorkersDone.notify_all();
     }
 
     void PhysicsTaskScheduler::syncWithMainThread()
@@ -710,4 +722,19 @@ namespace MWPhysics
         for (auto& sim : mSimulations)
             std::visit(vis, sim);
     }
+
+    // Attempt to acquire unique lock on mSimulationMutex while not all worker
+    // threads are holding shared lock but will have to may lead to a deadlock because
+    // C++ standard does not guarantee priority for exclusive and shared locks
+    // for std::shared_mutex. For example microsoft STL implementation points out
+    // for the absence of such priority:
+    // https://docs.microsoft.com/en-us/windows/win32/sync/slim-reader-writer--srw--locks
+    void PhysicsTaskScheduler::waitForWorkers()
+    {
+        if (mNumThreads == 0)
+            return;
+        std::unique_lock lock(mWorkersDoneMutex);
+        if (mFrameCounter != mWorkersFrameCounter)
+            mWorkersDone.wait(lock);
+    }
 }
diff --git a/apps/openmw/mwphysics/mtphysics.hpp b/apps/openmw/mwphysics/mtphysics.hpp
index 44330b2cc6..9ded1262d6 100644
--- a/apps/openmw/mwphysics/mtphysics.hpp
+++ b/apps/openmw/mwphysics/mtphysics.hpp
@@ -74,6 +74,7 @@ namespace MWPhysics
             void afterPostStep();
             void afterPostSim();
             void syncWithMainThread();
+            void waitForWorkers();
 
             std::unique_ptr<WorldFrameData> mWorldFrameData;
             std::vector<Simulation> mSimulations;
@@ -95,13 +96,17 @@ namespace MWPhysics
             int mNumJobs;
             int mRemainingSteps;
             int mLOSCacheExpiry;
-            bool mNewFrame;
+            std::size_t mFrameCounter;
             bool mAdvanceSimulation;
             bool mQuit;
             std::atomic<int> mNextJob;
             std::atomic<int> mNextLOS;
             std::vector<std::thread> mThreads;
 
+            std::size_t mWorkersFrameCounter = 0;
+            std::condition_variable mWorkersDone;
+            std::mutex mWorkersDoneMutex;
+
             mutable std::shared_mutex mSimulationMutex;
             mutable std::shared_mutex mCollisionWorldMutex;
             mutable std::shared_mutex mLOSCacheMutex;