Merge pull request #2240 from elsid/fix_navigator_deadlock

Fix navigator deadlock (bug #4777)

apps/openmw/mwrender/renderingmanager.cpp

@@ -1446,7 +1446,7 @@ namespace MWRender
         {
             try
             {
-                const auto locked = it->second.lockConst();
+                const auto locked = it->second->lockConst();
                 mNavMesh->update(locked->getValue(), mNavMeshNumber, locked->getGeneration(),
                                  locked->getNavMeshRevision(), mNavigator.getSettings());
             }

apps/openmw_test_suite/detournavigator/navigator.cpp

@@ -80,14 +80,6 @@ namespace
         EXPECT_THROW(mNavigator->findPath(mAgentHalfExtents, mStepSize, mStart, mEnd, Flag_walk, mOut), NavigatorException);
     }
 
-    TEST_F(DetourNavigatorNavigatorTest, find_path_for_removed_agent_should_return_empty)
-    {
-        mNavigator->addAgent(mAgentHalfExtents);
-        mNavigator->removeAgent(mAgentHalfExtents);
-        mNavigator->findPath(mAgentHalfExtents, mStepSize, mStart, mEnd, Flag_walk, mOut);
-        EXPECT_EQ(mPath, std::deque<osg::Vec3f>());
-    }
-
     TEST_F(DetourNavigatorNavigatorTest, add_agent_should_count_each_agent)
     {
         mNavigator->addAgent(mAgentHalfExtents);

components/detournavigator/asyncnavmeshupdater.cpp

@@ -129,12 +129,17 @@ namespace DetourNavigator
 
         const auto firstStart = setFirstStart(start);
 
+        const auto navMeshCacheItem = job.mNavMeshCacheItem.lock();
+
+        if (!navMeshCacheItem)
+            return true;
+
         const auto recastMesh = mRecastMeshManager.get().getMesh(job.mChangedTile);
         const auto playerTile = *mPlayerTile.lockConst();
         const auto offMeshConnections = mOffMeshConnectionsManager.get().get(job.mChangedTile);
 
         const auto status = updateNavMesh(job.mAgentHalfExtents, recastMesh.get(), job.mChangedTile, playerTile,
-            offMeshConnections, mSettings, job.mNavMeshCacheItem, mNavMeshTilesCache);
+            offMeshConnections, mSettings, navMeshCacheItem, mNavMeshTilesCache);
 
         const auto finish = std::chrono::steady_clock::now();
 
@@ -143,7 +148,7 @@ namespace DetourNavigator
         using FloatMs = std::chrono::duration<float, std::milli>;
 
         {
-            const auto locked = job.mNavMeshCacheItem.lockConst();
+            const auto locked = navMeshCacheItem->lockConst();
             log("cache updated for agent=", job.mAgentHalfExtents, " status=", status,
                 " generation=", locked->getGeneration(),
                 " revision=", locked->getNavMeshRevision(),
@@ -194,7 +199,8 @@ namespace DetourNavigator
             writeToFile(*recastMesh, mSettings.get().mRecastMeshPathPrefix + std::to_string(job.mChangedTile.x())
                         + "_" + std::to_string(job.mChangedTile.y()) + "_", recastMeshRevision);
         if (mSettings.get().mEnableWriteNavMeshToFile)
-            writeToFile(job.mNavMeshCacheItem.lockConst()->getValue(), mSettings.get().mNavMeshPathPrefix, navMeshRevision);
+            if (const auto shared = job.mNavMeshCacheItem.lock())
+                writeToFile(shared->lockConst()->getValue(), mSettings.get().mNavMeshPathPrefix, navMeshRevision);
     }
 
     std::chrono::steady_clock::time_point AsyncNavMeshUpdater::setFirstStart(const std::chrono::steady_clock::time_point& value)

components/detournavigator/asyncnavmeshupdater.hpp

@@ -48,7 +48,7 @@ namespace DetourNavigator
         struct Job
         {
             osg::Vec3f mAgentHalfExtents;
-            SharedNavMeshCacheItem mNavMeshCacheItem;
+            std::weak_ptr<GuardedNavMeshCacheItem> mNavMeshCacheItem;
             TilePosition mChangedTile;
             unsigned mTryNumber;
             ChangeType mChangeType;

components/detournavigator/makenavmesh.cpp

@@ -521,7 +521,7 @@ namespace
     UpdateNavMeshStatus replaceTile(const SharedNavMeshCacheItem& navMeshCacheItem,
         const TilePosition& changedTile, T&& navMeshData)
     {
-        const auto locked = navMeshCacheItem.lock();
+        const auto locked = navMeshCacheItem->lock();
         auto& navMesh = locked->getValue();
         const int layer = 0;
         const auto tileRef = navMesh.getTileRefAt(changedTile.x(), changedTile.y(), layer);
@@ -591,14 +591,14 @@ namespace DetourNavigator
             " playerTile=", playerTile,
             " changedTileDistance=", getDistance(changedTile, playerTile));
 
-        const auto params = *navMeshCacheItem.lockConst()->getValue().getParams();
+        const auto params = *navMeshCacheItem->lockConst()->getValue().getParams();
         const osg::Vec3f origin(params.orig[0], params.orig[1], params.orig[2]);
 
         const auto x = changedTile.x();
         const auto y = changedTile.y();
 
         const auto removeTile = [&] {
-            const auto locked = navMeshCacheItem.lock();
+            const auto locked = navMeshCacheItem->lock();
             auto& navMesh = locked->getValue();
             const auto tileRef = navMesh.getTileRefAt(x, y, 0);
             const auto removed = dtStatusSucceed(navMesh.removeTile(tileRef, nullptr, nullptr));

components/detournavigator/navigator.hpp

@@ -174,7 +174,7 @@ namespace DetourNavigator
             if (!navMesh)
                 return out;
             const auto settings = getSettings();
-            return findSmoothPath(navMesh.lock()->getValue(), toNavMeshCoordinates(settings, agentHalfExtents),
+            return findSmoothPath(navMesh->lockConst()->getValue(), toNavMeshCoordinates(settings, agentHalfExtents),
                 toNavMeshCoordinates(settings, stepSize), toNavMeshCoordinates(settings, start),
                 toNavMeshCoordinates(settings, end), includeFlags, settings, out);
         }

components/detournavigator/navigatorimpl.cpp

@@ -21,10 +21,10 @@ namespace DetourNavigator
     void NavigatorImpl::removeAgent(const osg::Vec3f& agentHalfExtents)
     {
         const auto it = mAgents.find(agentHalfExtents);
-        if (it == mAgents.end() || --it->second)
+        if (it == mAgents.end())
             return;
-        mAgents.erase(it);
+        if (it->second > 0)
-        mNavMeshManager.reset(agentHalfExtents);
+            --it->second;
     }
 
     bool NavigatorImpl::addObject(const ObjectId id, const btCollisionShape& shape, const btTransform& transform)
@@ -113,6 +113,7 @@ namespace DetourNavigator
 
     void NavigatorImpl::update(const osg::Vec3f& playerPosition)
     {
+        removeUnusedNavMeshes();
         for (const auto& v : mAgents)
             mNavMeshManager.update(playerPosition, v.first);
     }
@@ -156,4 +157,15 @@ namespace DetourNavigator
             inserted.first->second = updateId;
         }
     }
+
+    void NavigatorImpl::removeUnusedNavMeshes()
+    {
+        for (auto it = mAgents.begin(); it != mAgents.end();)
+        {
+            if (it->second == 0 && mNavMeshManager.reset(it->first))
+                it = mAgents.erase(it);
+            else
+                ++it;
+        }
+    }
 }

components/detournavigator/navigatorimpl.hpp

@@ -58,6 +58,7 @@ namespace DetourNavigator
         void updateAvoidShapeId(const ObjectId id, const ObjectId avoidId);
         void updateWaterShapeId(const ObjectId id, const ObjectId waterId);
         void updateId(const ObjectId id, const ObjectId waterId, std::unordered_map<ObjectId, ObjectId>& ids);
+        void removeUnusedNavMeshes();
     };
 }
 

components/detournavigator/navmeshcacheitem.hpp

@@ -64,7 +64,8 @@ namespace DetourNavigator
         std::map<TilePosition, std::pair<NavMeshTilesCache::Value, NavMeshData>> mUsedTiles;
     };
 
-    using SharedNavMeshCacheItem = Misc::SharedGuarded<NavMeshCacheItem>;
+    using GuardedNavMeshCacheItem = Misc::ScopeGuarded<NavMeshCacheItem>;
+    using SharedNavMeshCacheItem = std::shared_ptr<GuardedNavMeshCacheItem>;
 }
 
 #endif

components/detournavigator/navmeshmanager.cpp

@@ -16,6 +16,20 @@ namespace
     {
         return current == add ? current : ChangeType::mixed;
     }
+
+    /// Safely reset shared_ptr with definite underlying object destrutor call.
+    /// Assuming there is another thread holding copy of this shared_ptr or weak_ptr to this shared_ptr.
+    template <class T>
+    bool resetIfUnique(std::shared_ptr<T>& ptr)
+    {
+        const std::weak_ptr<T> weak = std::move(ptr);
+        if (auto shared = weak.lock())
+        {
+            ptr = std::move(shared);
+            return false;
+        }
+        return true;
+    }
 }
 
 namespace DetourNavigator
@@ -79,13 +93,19 @@ namespace DetourNavigator
         if (cached != mCache.end())
             return;
         mCache.insert(std::make_pair(agentHalfExtents,
-            std::make_shared<NavMeshCacheItem>(makeEmptyNavMesh(mSettings), ++mGenerationCounter)));
+            std::make_shared<GuardedNavMeshCacheItem>(makeEmptyNavMesh(mSettings), ++mGenerationCounter)));
         log("cache add for agent=", agentHalfExtents);
     }
 
-    void NavMeshManager::reset(const osg::Vec3f& agentHalfExtents)
+    bool NavMeshManager::reset(const osg::Vec3f& agentHalfExtents)
     {
+        const auto it = mCache.find(agentHalfExtents);
+        if (it == mCache.end())
+            return true;
+        if (!resetIfUnique(it->second))
+            return false;
         mCache.erase(agentHalfExtents);
+        return true;
     }
 
     void NavMeshManager::addOffMeshConnection(const ObjectId id, const osg::Vec3f& start, const osg::Vec3f& end)
@@ -139,7 +159,7 @@ namespace DetourNavigator
         }
         const auto changedTiles = mChangedTiles.find(agentHalfExtents);
         {
-            const auto locked = cached.lock();
+            const auto locked = cached->lockConst();
             const auto& navMesh = locked->getValue();
             if (changedTiles != mChangedTiles.end())
             {

components/detournavigator/navmeshmanager.hpp

@@ -36,7 +36,7 @@ namespace DetourNavigator
 
         bool removeWater(const osg::Vec2i& cellPosition);
 
-        void reset(const osg::Vec3f& agentHalfExtents);
+        bool reset(const osg::Vec3f& agentHalfExtents);
 
         void addOffMeshConnection(const ObjectId id, const osg::Vec3f& start, const osg::Vec3f& end);
 

components/misc/guarded.hpp

@@ -83,38 +83,6 @@ namespace Misc
             std::mutex mMutex;
             T mValue;
     };
-
-    template <class T>
-    class SharedGuarded
-    {
-        public:
-            SharedGuarded()
-                : mMutex(std::make_shared<std::mutex>()), mValue()
-            {}
-
-            SharedGuarded(std::shared_ptr<T> value)
-                : mMutex(std::make_shared<std::mutex>()), mValue(std::move(value))
-            {}
-
-            Locked<T> lock() const
-            {
-                return Locked<T>(*mMutex, *mValue);
-            }
-
-            Locked<const T> lockConst() const
-            {
-                return Locked<const T>(*mMutex, *mValue);
-            }
-
-            operator bool() const
-            {
-                return static_cast<bool>(mValue);
-            }
-
-        private:
-            std::shared_ptr<std::mutex> mMutex;
-            std::shared_ptr<T> mValue;
-    };
 }
 
 #endif