Skip to content

Commit

Permalink
Fix race condition in sound_threaded_backend
Browse files Browse the repository at this point in the history
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
  • Loading branch information
edo9300 committed Mar 11, 2023
1 parent 855c4e3 commit acf5cbf
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 15 deletions.
35 changes: 22 additions & 13 deletions gframe/sound_threaded_backend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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;
}
Expand Down Expand Up @@ -99,29 +105,31 @@ 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);
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);
}

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() {
Expand All @@ -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() {
Expand Down
18 changes: 16 additions & 2 deletions gframe/sound_threaded_backend.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
Expand All @@ -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>
Expand Down

0 comments on commit acf5cbf

Please sign in to comment.