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

3 files changed, 57 insertions, 40 deletions

diff --git a/src/zenserver/projectstore/httpprojectstore.cpp b/src/zenserver/projectstore/httpprojectstore.cpp
index 9a5894c56..d28d49d1f 100644
--- a/src/zenserver/projectstore/httpprojectstore.cpp
+++ b/src/zenserver/projectstore/httpprojectstore.cpp
@@ -1626,7 +1626,7 @@ HttpProjectService::HandleDetailsRequest(HttpRouterRequest& Req)
 		CSVHeader(Details, AttachmentDetails, CSVWriter);
 
 		m_ProjectStore->IterateProjects([&](ProjectStore::Project& Project) {
-			Project.IterateOplogs([&](ProjectStore::Oplog& Oplog) {
+			Project.IterateOplogs([&](const RwLock::SharedLockScope&, ProjectStore::Oplog& Oplog) {
 				Oplog.IterateOplogWithKey(
 					[this, &Project, &Oplog, &CSVWriter, Details, AttachmentDetails](int LSN, const Oid& Key, CbObject Op) {
 						CSVWriteOp(m_CidStore, Project.Identifier, Oplog.OplogId(), Details, AttachmentDetails, LSN, Key, Op, CSVWriter);
@@ -1681,7 +1681,7 @@ HttpProjectService::HandleProjectDetailsRequest(HttpRouterRequest& Req)
 		ExtendableStringBuilder<4096> CSVWriter;
 		CSVHeader(Details, AttachmentDetails, CSVWriter);
 
-		FoundProject->IterateOplogs([&](ProjectStore::Oplog& Oplog) {
+		FoundProject->IterateOplogs([&](const RwLock::SharedLockScope&, ProjectStore::Oplog& Oplog) {
 			Oplog.IterateOplogWithKey(
 				[this, &Project, &Oplog, &CSVWriter, Details, AttachmentDetails](int LSN, const Oid& Key, CbObject Op) {
 					CSVWriteOp(m_CidStore, Project.Identifier, Oplog.OplogId(), Details, AttachmentDetails, LSN, Key, Op, CSVWriter);
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);
diff --git a/src/zenserver/projectstore/projectstore.h b/src/zenserver/projectstore/projectstore.h
index dcbe8cdab..a2f92fb25 100644
--- a/src/zenserver/projectstore/projectstore.h
+++ b/src/zenserver/projectstore/projectstore.h
@@ -213,13 +213,14 @@ public:
 		Oplog*					 NewOplog(std::string_view OplogId, const std::filesystem::path& MarkerPath);
 		Oplog*					 OpenOplog(std::string_view OplogId);
 		void					 DeleteOplog(std::string_view OplogId);
-		void					 IterateOplogs(std::function<void(const Oplog&)>&& Fn) const;
-		void					 IterateOplogs(std::function<void(Oplog&)>&& Fn);
+		void					 IterateOplogs(std::function<void(const RwLock::SharedLockScope&, const Oplog&)>&& Fn) const;
+		void					 IterateOplogs(std::function<void(const RwLock::SharedLockScope&, Oplog&)>&& Fn);
 		std::vector<std::string> ScanForOplogs() const;
-		bool					 IsExpired(const GcClock::TimePoint ExpireTime) const;
-		bool					 IsExpired(const GcClock::TimePoint ExpireTime, const ProjectStore::Oplog& Oplog) const;
-		void					 TouchProject() const;
-		void					 TouchOplog(std::string_view Oplog) const;
+		bool					 IsExpired(const RwLock::SharedLockScope&, const GcClock::TimePoint ExpireTime) const;
+		bool IsExpired(const RwLock::SharedLockScope&, const GcClock::TimePoint ExpireTime, const ProjectStore::Oplog& Oplog) const;
+		bool IsExpired(const GcClock::TimePoint ExpireTime, const ProjectStore::Oplog& Oplog) const;
+		void TouchProject() const;
+		void TouchOplog(std::string_view Oplog) const;
 
 		Project(ProjectStore* PrjStore, CidStore& Store, std::filesystem::path BasePath);
 		virtual ~Project();
@@ -244,9 +245,12 @@ public:
 		mutable tsl::robin_map<std::string, GcClock::Tick> m_LastAccessTimes;
 
 		std::filesystem::path BasePathForOplog(std::string_view OplogId);
-		bool IsExpired(const std::string& EntryName, const std::filesystem::path& MarkerPath, const GcClock::TimePoint ExpireTime) const;
-		void WriteAccessTimes();
-		void ReadAccessTimes();
+		bool				  IsExpired(const RwLock::SharedLockScope&,
+										const std::string&			 EntryName,
+										const std::filesystem::path& MarkerPath,
+										const GcClock::TimePoint	 ExpireTime) const;
+		void				  WriteAccessTimes();
+		void				  ReadAccessTimes();
 	};
 
 	//	Oplog* OpenProjectOplog(std::string_view ProjectId, std::string_view OplogId);