From acf5cbffd7d5eb779738785057930dc2bd0364fc Mon Sep 17 00:00:00 2001
From: Edoardo Lolletti <edoardo762@gmail.com>
Date: Sat, 11 Mar 2023 22:19:44 +0100
Subject: [PATCH] Fix race condition in sound_threaded_backend

Actions that required a response could incur in a race condition if multiple threads queued an action requiring an answer, thus making them unblock on that same response.
Rework those actions to pass a pointer to a response variable on the stack that will then be populated with the repsonse and checked accordingly
---
 gframe/sound_threaded_backend.cpp | 35 +++++++++++++++++++------------
 gframe/sound_threaded_backend.h   | 18 ++++++++++++++--
 2 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/gframe/sound_threaded_backend.cpp b/gframe/sound_threaded_backend.cpp
index c48f9c602..c98c5c6e8 100644
--- a/gframe/sound_threaded_backend.cpp
+++ b/gframe/sound_threaded_backend.cpp
@@ -26,16 +26,20 @@ void SoundThreadedBackend::BaseLoop() {
 			break;
 		}
 		case ActionType::PLAY_MUSIC: {
-			auto res = m_BaseBackend->PlayMusic(*action.arg.play_music.name, action.arg.play_music.loop);
+			auto& argument = action.arg.play_music;
+			auto& response = *argument.response;
+			response.answer = m_BaseBackend->PlayMusic(*argument.name, argument.loop);
 			std::lock_guard<epro::mutex> lckres(m_ResponseMutex);
-			response = res;
+			response.answered = true;
 			m_ResponseCondVar.notify_all();
 			break;
 		}
 		case ActionType::PLAY_SOUND: {
-			auto res = m_BaseBackend->PlaySound(*action.arg.play_sound);
+			auto& argument = action.arg.play_sound;
+			auto& response = *argument.response;
+			response.answer = m_BaseBackend->PlaySound(*argument.name);
 			std::lock_guard<epro::mutex> lckres(m_ResponseMutex);
-			response = res;
+			response.answered = true;
 			m_ResponseCondVar.notify_all();
 			break;
 		}
@@ -52,9 +56,11 @@ void SoundThreadedBackend::BaseLoop() {
 			break;
 		}
 		case ActionType::MUSIC_PLAYING: {
-			auto res = m_BaseBackend->MusicPlaying();
+			auto& argument = action.arg.is_playing;
+			auto& response = *argument.response;
+			response.answer = m_BaseBackend->MusicPlaying();
 			std::lock_guard<epro::mutex> lckres(m_ResponseMutex);
-			response = res;
+			response.answered = true;
 			m_ResponseCondVar.notify_all();
 			break;
 		}
@@ -99,8 +105,10 @@ void SoundThreadedBackend::SetMusicVolume(double volume) {
 }
 
 bool SoundThreadedBackend::PlayMusic(const std::string& name, bool loop) {
+	Response res{};
 	Action action{ ActionType::PLAY_MUSIC };
 	auto& args = action.arg.play_music;
+	args.response = &res;
 	args.name = &name;
 	args.loop = loop;
 	std::unique_lock<epro::mutex> lck(m_ActionMutex);
@@ -108,20 +116,20 @@ bool SoundThreadedBackend::PlayMusic(const std::string& name, bool loop) {
 	m_Actions.emplace(std::move(action));
 	m_ActionCondVar.notify_all();
 	lck.unlock();
-	m_ResponseCondVar.wait(lckres);
-	return response;
+	return WaitForResponse(lckres, res);
 }
 
 bool SoundThreadedBackend::PlaySound(const std::string& name) {
+	Response res{};
 	Action action{ ActionType::PLAY_SOUND };
-	action.arg.play_sound = &name;
+	action.arg.play_sound.name = &name;
+	action.arg.play_sound.response = &res;
 	std::unique_lock<epro::mutex> lck(m_ActionMutex);
 	std::unique_lock<epro::mutex> lckres(m_ResponseMutex);
 	m_Actions.emplace(std::move(action));
 	m_ActionCondVar.notify_all();
 	lck.unlock();
-	m_ResponseCondVar.wait(lckres);
-	return response;
+	return WaitForResponse(lckres, res);
 }
 
 void SoundThreadedBackend::StopSounds() {
@@ -147,14 +155,15 @@ void SoundThreadedBackend::PauseMusic(bool pause) {
 }
 
 bool SoundThreadedBackend::MusicPlaying() {
+	Response res{};
 	Action action{ ActionType::MUSIC_PLAYING };
+	action.arg.is_playing.response = &res;
 	std::unique_lock<epro::mutex> lck(m_ActionMutex);
 	std::unique_lock<epro::mutex> lckres(m_ResponseMutex);
 	m_Actions.emplace(std::move(action));
 	m_ActionCondVar.notify_all();
 	lck.unlock();
-	m_ResponseCondVar.wait(lckres);
-	return response;
+	return WaitForResponse(lckres, res);
 }
 
 void SoundThreadedBackend::Tick() {
diff --git a/gframe/sound_threaded_backend.h b/gframe/sound_threaded_backend.h
index 226f34b02..0540d8ee3 100644
--- a/gframe/sound_threaded_backend.h
+++ b/gframe/sound_threaded_backend.h
@@ -44,12 +44,23 @@ class SoundThreadedBackend : public SoundBackend {
 		PLAY_SOUND,
 		MUSIC_PLAYING
 	};
+	struct Response {
+		bool answer;
+		bool answered;
+	};
 	union Argument {
 		struct {
+			Response* response;
 			const std::string* name;
 			bool loop;
 		} play_music;
-		const std::string* play_sound;
+		struct {
+			Response* response;
+			const std::string* name;
+		} play_sound;
+		struct {
+			Response* response;
+		} is_playing;
 		double volume;
 		bool pause;
 	};
@@ -64,8 +75,11 @@ class SoundThreadedBackend : public SoundBackend {
 	epro::mutex m_ResponseMutex;
 	epro::condition_variable m_ResponseCondVar;
 	std::queue<Action> m_Actions;
-	bool response;
 	epro::thread m_BaseThread;
+	bool WaitForResponse(std::unique_lock<epro::mutex>& lock, Response& res) {
+		m_ResponseCondVar.wait(lock, [&res] {return res.answered; });
+		return res.answer;
+	}
 };
 
 template<typename T>