From 84bc4d8c657834983261c6dee37ab7f686972094 Mon Sep 17 00:00:00 2001
From: Vladimir Olteanu <vladimir.olteanu@cs.pub.ro>
Date: Sun, 18 Jul 2021 16:12:47 +0300
Subject: [PATCH] dthread: simplify synchronization logic (#2419)

---
 Source/dthread.cpp       | 62 +++++++++++++++++++++-------------------
 Source/utils/sdl_cond.h  | 49 +++++++++++++++++++++++++++++++
 Source/utils/sdl_mutex.h |  5 ++++
 3 files changed, 86 insertions(+), 30 deletions(-)
 create mode 100644 Source/utils/sdl_cond.h

diff --git a/Source/dthread.cpp b/Source/dthread.cpp
index 7ba8792a4..4c4d9f1da 100644
--- a/Source/dthread.cpp
+++ b/Source/dthread.cpp
@@ -4,12 +4,11 @@
  * Implementation of functions for updating game state from network commands.
  */
 
-#include <atomic>
 #include <list>
 #include <mutex>
 
 #include "nthread.h"
-#include "utils/sdl_mutex.h"
+#include "utils/sdl_cond.h"
 #include "utils/thread.h"
 
 namespace devilution {
@@ -31,32 +30,30 @@ struct DThreadPkt {
 
 namespace {
 
-SdlMutex DthreadMutex;
+std::optional<SdlMutex> DthreadMutex;
 SDL_threadID glpDThreadId;
 std::list<DThreadPkt> InfoList;
-std::atomic_bool dthread_running;
-event_emul *sghWorkToDoEvent;
+bool DthreadRunning;
+std::optional<SdlCond> WorkToDo;
 
 /* rdata */
 SDL_Thread *sghThread = nullptr;
 
 void DthreadHandler()
 {
-	while (dthread_running) {
-		if (InfoList.empty() && WaitForEvent(sghWorkToDoEvent) == -1)
-			app_fatal("dthread4:\n%s", SDL_GetError());
-
-		DthreadMutex.lock();
-		if (InfoList.empty()) {
-			ResetEvent(sghWorkToDoEvent);
-			DthreadMutex.unlock();
-			continue;
+	std::lock_guard<SdlMutex> lock(*DthreadMutex);
+	while (true) {
+		while (!InfoList.empty()) {
+			DThreadPkt pkt = std::move(InfoList.front());
+			InfoList.pop_front();
+
+			DthreadMutex->unlock();
+			multi_send_zero_packet(pkt.pnum, pkt.cmd, pkt.data.get(), pkt.len);
+			DthreadMutex->lock();
 		}
-		DThreadPkt pkt = std::move(InfoList.front());
-		InfoList.pop_front();
-		DthreadMutex.unlock();
-
-		multi_send_zero_packet(pkt.pnum, pkt.cmd, pkt.data.get(), pkt.len);
+		if (!DthreadRunning)
+			return;
+		WorkToDo->wait(*DthreadMutex);
 	}
 }
 
@@ -64,7 +61,7 @@ void DthreadHandler()
 
 void dthread_remove_player(uint8_t pnum)
 {
-	std::lock_guard<SdlMutex> lock(DthreadMutex);
+	std::lock_guard<SdlMutex> lock(*DthreadMutex);
 	InfoList.remove_if([&](auto &pkt) {
 		return pkt.pnum == pnum;
 	});
@@ -77,9 +74,9 @@ void dthread_send_delta(int pnum, _cmd_id cmd, std::unique_ptr<byte[]> data, uin
 
 	DThreadPkt pkt { pnum, cmd, std::move(data), len };
 
-	std::lock_guard<SdlMutex> lock(DthreadMutex);
+	std::lock_guard<SdlMutex> lock(*DthreadMutex);
 	InfoList.push_back(std::move(pkt));
-	SetEvent(sghWorkToDoEvent);
+	WorkToDo->signal();
 }
 
 void dthread_start()
@@ -87,26 +84,31 @@ void dthread_start()
 	if (!gbIsMultiplayer)
 		return;
 
-	sghWorkToDoEvent = StartEvent();
-	dthread_running = true;
+	DthreadRunning = true;
+	DthreadMutex.emplace();
+	WorkToDo.emplace();
 	sghThread = CreateThread(DthreadHandler, &glpDThreadId);
 }
 
 void DThreadCleanup()
 {
-	if (sghWorkToDoEvent == nullptr)
+	if (!DthreadRunning)
 		return;
 
-	dthread_running = false;
-	SetEvent(sghWorkToDoEvent);
+	{
+		std::lock_guard<SdlMutex> lock(*DthreadMutex);
+		DthreadRunning = false;
+		InfoList.clear();
+		WorkToDo->signal();
+	}
+
 	if (sghThread != nullptr && glpDThreadId != SDL_GetThreadID(nullptr)) {
 		SDL_WaitThread(sghThread, nullptr);
 		sghThread = nullptr;
 	}
-	EndEvent(sghWorkToDoEvent);
-	sghWorkToDoEvent = nullptr;
 
-	InfoList.clear();
+	DthreadMutex = std::nullopt;
+	WorkToDo = std::nullopt;
 }
 
 } // namespace devilution
diff --git a/Source/utils/sdl_cond.h b/Source/utils/sdl_cond.h
new file mode 100644
index 000000000..ecf14fe7f
--- /dev/null
+++ b/Source/utils/sdl_cond.h
@@ -0,0 +1,49 @@
+#pragma once
+
+#include <SDL.h>
+#include "sdl_mutex.h"
+
+namespace devilution {
+
+/*
+ * RAII wrapper for SDL_cond.
+ */
+class SdlCond final {
+public:
+	SdlCond()
+	    : cond(SDL_CreateCond())
+	{
+		if (cond == nullptr)
+			ErrSdl();
+	}
+
+	~SdlCond()
+	{
+		SDL_DestroyCond(cond);
+	}
+
+	SdlCond(const SdlCond &) = delete;
+	SdlCond(SdlCond &&) = delete;
+	SdlCond &operator=(const SdlCond &) = delete;
+	SdlCond &operator=(SdlCond &&) = delete;
+
+	void signal()
+	{
+		int err = SDL_CondSignal(cond);
+		if (err < 0)
+			ErrSdl();
+	}
+
+	void wait(SdlMutex &mutex)
+	{
+		int err = SDL_CondWait(cond, mutex.get());
+		if (err < 0)
+			ErrSdl();
+	}
+
+private:
+	SDL_cond *cond;
+};
+
+} // namespace devilution
+
diff --git a/Source/utils/sdl_mutex.h b/Source/utils/sdl_mutex.h
index 2f37c730c..95286e480 100644
--- a/Source/utils/sdl_mutex.h
+++ b/Source/utils/sdl_mutex.h
@@ -56,6 +56,11 @@ public:
 			ErrSdl();
 	}
 
+	SDL_mutex *get()
+	{
+		return mutex_;
+	}
+
 private:
 	SDL_mutex *mutex_;
 };