Merge branch 'fix_physics_deadlock' into 'master'

Fix deadlock in physics system (#6414)

Closes #6414

See merge request OpenMW/openmw!1440

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)
+            if (lastFrame == mFrameCounter)
-                mHasJob.wait(lock, [&]() { return mQuit || mNewFrame; });
+            {
+                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);
+    }
 }

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;