From 055ec9e723513a26ab78e88e81994ba5592271de Mon Sep 17 00:00:00 2001
From: Mikko Rasa <tdb@tdb.fi>
Date: Wed, 19 Mar 2025 00:03:04 +0200
Subject: [PATCH] Prevent concurrent write access to unbuffered components

---
 source/game/system.h            | 36 ++++++++++++++++++++-------------
 source/game/systemscheduler.cpp |  8 ++++++--
 tests/scheduler.cpp             | 17 +++++++++-------
 3 files changed, 38 insertions(+), 23 deletions(-)

diff --git a/source/game/system.h b/source/game/system.h
index d4ba13e..2882e8c 100644
--- a/source/game/system.h
+++ b/source/game/system.h
@@ -22,9 +22,10 @@ public:
 		READ_OLD = 1,
 		READ_FRESH = 3,
 		WRITE = 4,
+		UNBUFFERED = 32,
 		UPDATE = READ_OLD | WRITE,
 		CHAINED_UPDATE = READ_FRESH | WRITE,
-		DATA_MASK = 7,
+		DATA_MASK = 39,
 		RUN_BEFORE = 8,
 		RUN_AFTER = 16,
 		ORDER_MASK = 24
@@ -96,6 +97,8 @@ protected:
 template<typename T>
 inline void System::declare_dependency(DependencyFlags flags)
 {
+	if(flags&UNBUFFERED)
+		throw std::invalid_argument("System::declare_dependency");
 	if((flags&DATA_MASK) && !std::is_base_of_v<Component, T>)
 		throw std::invalid_argument("System::declare_dependency");
 	if((flags&ORDER_MASK) && !std::is_base_of_v<System, T>)
@@ -111,23 +114,28 @@ inline void System::declare_dependency(DependencyFlags flags)
 
 	Dependency &dep = (i!=dependencies.end() ? *i : dependencies.emplace_back(type));
 	dep.flags = flags;
-	if constexpr(requires(T &c) { typename T::Data; c.prepare_tick(); c.commit_tick(); })
+	if constexpr(std::is_base_of_v<Component, T>)
 	{
+		if constexpr(requires(T &c) { typename T::Data; c.prepare_tick(); c.commit_tick(); })
+		{
 #ifdef DEBUG
-		dep.unblock = +[](DependencyFlags f){
-			if(f&READ_OLD) AccessGuard::get_instance().unblock<AccessGuard::Read<typename T::Data>>();
-			if(f&WRITE) AccessGuard::get_instance().unblock<AccessGuard::Write<typename T::Data>>();
-		};
-		dep.block = +[](DependencyFlags f){
-			if(f&READ_OLD) AccessGuard::get_instance().block<AccessGuard::Read<typename T::Data>>();
-			if(f&WRITE) AccessGuard::get_instance().block<AccessGuard::Write<typename T::Data>>();
-		};
+			dep.unblock = +[](DependencyFlags f){
+				if(f&READ_OLD) AccessGuard::get_instance().unblock<AccessGuard::Read<typename T::Data>>();
+				if(f&WRITE) AccessGuard::get_instance().unblock<AccessGuard::Write<typename T::Data>>();
+			};
+			dep.block = +[](DependencyFlags f){
+				if(f&READ_OLD) AccessGuard::get_instance().block<AccessGuard::Read<typename T::Data>>();
+				if(f&WRITE) AccessGuard::get_instance().block<AccessGuard::Write<typename T::Data>>();
+			};
 #endif
-		if(flags&WRITE)
-		{
-			dep.prepare = +[](Stage &s){ s.iterate_objects<T>([](T &c){ c.prepare_tick(); }); };
-			dep.commit = +[](Stage &s){ s.iterate_objects<T>([](T &c){ c.commit_tick(); }); };
+			if((flags&(WRITE|UNBUFFERED))==WRITE)
+			{
+				dep.prepare = +[](Stage &s){ s.iterate_objects<T>([](T &c){ c.prepare_tick(); }); };
+				dep.commit = +[](Stage &s){ s.iterate_objects<T>([](T &c){ c.commit_tick(); }); };
+			}
 		}
+		else
+			dep.flags = static_cast<DependencyFlags>(dep.flags|UNBUFFERED);
 	}
 }
 
diff --git a/source/game/systemscheduler.cpp b/source/game/systemscheduler.cpp
index 40a951e..1cc78a4 100644
--- a/source/game/systemscheduler.cpp
+++ b/source/game/systemscheduler.cpp
@@ -90,14 +90,18 @@ int SystemScheduler::get_data_order(const GraphNode &node1, const GraphNode &nod
 			else
 				continue;
 
+			bool unbuffered = (flags1|flags2)&System::UNBUFFERED;
+			flags1 &= ~System::UNBUFFERED;
+			flags2 &= ~System::UNBUFFERED;
+
 			int order = 0;
 			// Read-only access to fresh data comes after any writes
 			if(flags1==System::READ_FRESH && (flags2&System::WRITE))
 				order = 1;
 			else if(flags2==System::READ_FRESH && (flags1&System::WRITE))
 				order = -1;
-			// Other read-only access situations are not ordered
-			else if(!(flags1&System::WRITE) || !(flags2&System::WRITE))
+			// Reads can happen concurrently to other access with buffering
+			else if(!(flags1&flags2&System::WRITE) && !unbuffered)
 				order = 0;
 			// Chained updates always come after non-chained writes
 			else if(flags1==System::CHAINED_UPDATE && (flags2==System::WRITE || flags2==System::UPDATE))
diff --git a/tests/scheduler.cpp b/tests/scheduler.cpp
index 7b34ebf..24bbe1b 100644
--- a/tests/scheduler.cpp
+++ b/tests/scheduler.cpp
@@ -15,7 +15,7 @@ public:
 private:
 	void unrelated_components();
 	void chained_update();
-	void parallel_reads();
+	void parallel_access();
 	void ambiguous_data_order();
 };
 
@@ -35,9 +35,12 @@ struct Env
 	{ }
 };
 
-struct A: Game::Component { };
-struct B: Game::Component { };
-struct C: Game::Component { };
+struct AData { };
+struct A: Game::BufferedComponent<AData> { };
+struct BData { };
+struct B: Game::BufferedComponent<BData> { };
+struct CData { };
+struct C: Game::BufferedComponent<CData> { };
 
 template<typename T, Game::System::DependencyFlags F>
 struct Dep
@@ -75,7 +78,7 @@ SchedulerTests::SchedulerTests()
 {
 	add(&SchedulerTests::unrelated_components, "Unrelated components");
 	add(&SchedulerTests::chained_update, "Chained update");
-	add(&SchedulerTests::parallel_reads, "Parallel reads");
+	add(&SchedulerTests::parallel_access, "Parallel access");
 	add(&SchedulerTests::ambiguous_data_order, "Ambiguous data order").expect_throw<Game::scheduling_error>();
 }
 
@@ -120,11 +123,11 @@ void SchedulerTests::chained_update()
 	EXPECT_EQUAL(graph[2].prerequisites, 3);
 }
 
-void SchedulerTests::parallel_reads()
+void SchedulerTests::parallel_access()
 {
 	Env env;
 
-	auto &sys1 = env.stage.add_system<Sys<Dep<A, READ_OLD>>>();
+	auto &sys1 = env.stage.add_system<Sys<Dep<A, UPDATE>>>();
 	auto &sys2 = env.stage.add_system<Sys<Dep<A, READ_OLD>, Dep<B, UPDATE>>>();
 	auto &sys3 = env.stage.add_system<Sys<Dep<A, READ_OLD>, Dep<C, UPDATE>>>();
 	auto &sys4 = env.stage.add_system<Sys<Dep<B, READ_FRESH>, Dep<C, READ_FRESH>>>();
-- 
2.45.2