project store gc deadlock (#381)

* Trying to recursively take a shared RWLock while another thread is waiting for a exclusive lock results in a deadlock. Forward the shared lock so we don't have to grab it recursively.
* changelog

1 files changed, 42 insertions, 29 deletions

diff --git a/src/zenserver/projectstore/projectstore.cpp b/src/zenserver/projectstore/projectstore.cpp
index c37b8c8d4..e42afdb67 100644
--- a/src/zenserver/projectstore/projectstore.cpp
+++ b/src/zenserver/projectstore/projectstore.cpp
@@ -1320,24 +1320,24 @@ ProjectStore::Project::ScanForOplogs() const
 }
 
 void
-ProjectStore::Project::IterateOplogs(std::function<void(const Oplog&)>&& Fn) const
+ProjectStore::Project::IterateOplogs(std::function<void(const RwLock::SharedLockScope&, const Oplog&)>&& Fn) const
 {
-	RwLock::SharedLockScope _(m_ProjectLock);
+	RwLock::SharedLockScope Lock(m_ProjectLock);
 
 	for (auto& Kv : m_Oplogs)
 	{
-		Fn(*Kv.second);
+		Fn(Lock, *Kv.second);
 	}
 }
 
 void
-ProjectStore::Project::IterateOplogs(std::function<void(Oplog&)>&& Fn)
+ProjectStore::Project::IterateOplogs(std::function<void(const RwLock::SharedLockScope&, Oplog&)>&& Fn)
 {
-	RwLock::SharedLockScope _(m_ProjectLock);
+	RwLock::SharedLockScope Lock(m_ProjectLock);
 
 	for (auto& Kv : m_Oplogs)
 	{
-		Fn(*Kv.second);
+		Fn(Lock, *Kv.second);
 	}
 }
 
@@ -1345,7 +1345,7 @@ void
 ProjectStore::Project::Flush()
 {
 	// We only need to flush oplogs that we have already loaded
-	IterateOplogs([&](Oplog& Ops) { Ops.Flush(); });
+	IterateOplogs([&](const RwLock::SharedLockScope&, Oplog& Ops) { Ops.Flush(); });
 	WriteAccessTimes();
 }
 
@@ -1358,8 +1358,8 @@ ProjectStore::Project::ScrubStorage(ScrubContext& Ctx)
 	{
 		OpenOplog(OpLogId);
 	}
-	IterateOplogs([&](const Oplog& Ops) {
-		if (!IsExpired(GcClock::TimePoint::min(), Ops))
+	IterateOplogs([&](const RwLock::SharedLockScope& ProjectLock, const Oplog& Ops) {
+		if (!IsExpired(ProjectLock, GcClock::TimePoint::min(), Ops))
 		{
 			Ops.ScrubStorage(Ctx);
 		}
@@ -1395,8 +1395,8 @@ ProjectStore::Project::GatherReferences(GcContext& GcCtx)
 		}
 	}
 
-	IterateOplogs([&](Oplog& Ops) {
-		if (!IsExpired(GcCtx.ProjectStoreExpireTime(), Ops))
+	IterateOplogs([&](const RwLock::SharedLockScope& ProjectLock, Oplog& Ops) {
+		if (!IsExpired(ProjectLock, GcCtx.ProjectStoreExpireTime(), Ops))
 		{
 			Ops.GatherReferences(GcCtx);
 		}
@@ -1442,7 +1442,8 @@ ProjectStore::Project::PrepareForDelete(std::filesystem::path& OutDeletePath)
 }
 
 bool
-ProjectStore::Project::IsExpired(const std::string&			  EntryName,
+ProjectStore::Project::IsExpired(const RwLock::SharedLockScope&,
+								 const std::string&			  EntryName,
 								 const std::filesystem::path& MarkerPath,
 								 const GcClock::TimePoint	  ExpireTime) const
 {
@@ -1456,7 +1457,6 @@ ProjectStore::Project::IsExpired(const std::string&			  EntryName,
 
 	const GcClock::Tick ExpireTicks = ExpireTime.time_since_epoch().count();
 
-	RwLock::SharedLockScope _(m_ProjectLock);
 	if (auto It = m_LastAccessTimes.find(EntryName); It != m_LastAccessTimes.end())
 	{
 		if (It->second <= ExpireTicks)
@@ -1468,15 +1468,24 @@ ProjectStore::Project::IsExpired(const std::string&			  EntryName,
 }
 
 bool
-ProjectStore::Project::IsExpired(const GcClock::TimePoint ExpireTime) const
+ProjectStore::Project::IsExpired(const RwLock::SharedLockScope& ProjectLock, const GcClock::TimePoint ExpireTime) const
+{
+	return IsExpired(ProjectLock, std::string(), ProjectFilePath, ExpireTime);
+}
+
+bool
+ProjectStore::Project::IsExpired(const RwLock::SharedLockScope& ProjectLock,
+								 const GcClock::TimePoint		ExpireTime,
+								 const ProjectStore::Oplog&		Oplog) const
 {
-	return IsExpired(std::string(), ProjectFilePath, ExpireTime);
+	return IsExpired(ProjectLock, Oplog.OplogId(), Oplog.MarkerPath(), ExpireTime);
 }
 
 bool
 ProjectStore::Project::IsExpired(const GcClock::TimePoint ExpireTime, const ProjectStore::Oplog& Oplog) const
 {
-	return IsExpired(Oplog.OplogId(), Oplog.MarkerPath(), ExpireTime);
+	RwLock::SharedLockScope Lock(m_ProjectLock);
+	return IsExpired(Lock, Oplog.OplogId(), Oplog.MarkerPath(), ExpireTime);
 }
 
 void
@@ -1576,12 +1585,12 @@ ProjectStore::ScrubStorage(ScrubContext& Ctx)
 
 	std::vector<Ref<Project>> Projects;
 	{
-		RwLock::SharedLockScope _(m_ProjectsLock);
+		RwLock::SharedLockScope Lock(m_ProjectsLock);
 		Projects.reserve(m_Projects.size());
 
 		for (auto& Kv : m_Projects)
 		{
-			if (Kv.second->IsExpired(GcClock::TimePoint::min()))
+			if (Kv.second->IsExpired(Lock, GcClock::TimePoint::min()))
 			{
 				continue;
 			}
@@ -1614,12 +1623,12 @@ ProjectStore::GatherReferences(GcContext& GcCtx)
 
 	std::vector<Ref<Project>> Projects;
 	{
-		RwLock::SharedLockScope _(m_ProjectsLock);
+		RwLock::SharedLockScope Lock(m_ProjectsLock);
 		Projects.reserve(m_Projects.size());
 
 		for (auto& Kv : m_Projects)
 		{
-			if (Kv.second->IsExpired(GcCtx.ProjectStoreExpireTime()))
+			if (Kv.second->IsExpired(Lock, GcCtx.ProjectStoreExpireTime()))
 			{
 				ExpiredProjectCount++;
 				continue;
@@ -1654,10 +1663,10 @@ ProjectStore::CollectGarbage(GcContext& GcCtx)
 	std::vector<Ref<Project>> Projects;
 
 	{
-		RwLock::SharedLockScope _(m_ProjectsLock);
+		RwLock::SharedLockScope Lock(m_ProjectsLock);
 		for (auto& Kv : m_Projects)
 		{
-			if (Kv.second->IsExpired(GcCtx.ProjectStoreExpireTime()))
+			if (Kv.second->IsExpired(Lock, GcCtx.ProjectStoreExpireTime()))
 			{
 				ExpiredProjects.push_back(Kv.second);
 				ExpiredProjectCount++;
@@ -1679,8 +1688,8 @@ ProjectStore::CollectGarbage(GcContext& GcCtx)
 		std::vector<std::string> ExpiredOplogs;
 		{
 			RwLock::ExclusiveLockScope _(m_ProjectsLock);
-			Project->IterateOplogs([&GcCtx, &Project, &ExpiredOplogs](ProjectStore::Oplog& Oplog) {
-				if (Project->IsExpired(GcCtx.ProjectStoreExpireTime(), Oplog))
+			Project->IterateOplogs([&GcCtx, &Project, &ExpiredOplogs](const RwLock::SharedLockScope& Lock, ProjectStore::Oplog& Oplog) {
+				if (Project->IsExpired(Lock, GcCtx.ProjectStoreExpireTime(), Oplog))
 				{
 					ExpiredOplogs.push_back(Oplog.OplogId());
 				}
@@ -1707,13 +1716,17 @@ ProjectStore::CollectGarbage(GcContext& GcCtx)
 		std::filesystem::path PathToRemove;
 		std::string			  ProjectId;
 		{
-			RwLock::ExclusiveLockScope _(m_ProjectsLock);
-			if (!Project->IsExpired(GcCtx.ProjectStoreExpireTime()))
 			{
-				ZEN_DEBUG("ProjectStore::CollectGarbage skipped garbage collect of project '{}'. Project no longer expired.", ProjectId);
-				continue;
+				RwLock::SharedLockScope Lock(m_ProjectsLock);
+				if (!Project->IsExpired(Lock, GcCtx.ProjectStoreExpireTime()))
+				{
+					ZEN_DEBUG("ProjectStore::CollectGarbage skipped garbage collect of project '{}'. Project no longer expired.",
+							  ProjectId);
+					continue;
+				}
 			}
-			bool Success = Project->PrepareForDelete(PathToRemove);
+			RwLock::ExclusiveLockScope _(m_ProjectsLock);
+			bool					   Success = Project->PrepareForDelete(PathToRemove);
 			if (!Success)
 			{
 				ZEN_DEBUG("ProjectStore::CollectGarbage skipped garbage collect of project '{}'. Project folder is locked.", ProjectId);