From 3f067f845796e592ffb5dff903034d79da900ec4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?kleines=20Filmr=C3=B6llchen?= <malu.bertsch@gmail.com>
Date: Thu, 2 Sep 2021 20:05:21 +0200
Subject: [PATCH] AudioServer: Fix deadlock when playing two audio streams

Previously, AudioServer would deadlock when trying to play another audio
stream, i.e. creating a queue. The m_pending_cond condition was used
improperly, and the condition wait now happens independently of querying
the pending queue for new clients if the mixer is running.

To make the mixer's concurrency-safety code more readable, the use of
raw POSIX mutex and condition syscalls is replaced with Threading::Mutex
and Threading::ConditionVariable.
---
 Userland/Services/AudioServer/Mixer.cpp | 26 +++++++++++++------------
 Userland/Services/AudioServer/Mixer.h   |  7 ++++---
 2 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/Userland/Services/AudioServer/Mixer.cpp b/Userland/Services/AudioServer/Mixer.cpp
index 519780b996b..bc57b25eda4 100644
--- a/Userland/Services/AudioServer/Mixer.cpp
+++ b/Userland/Services/AudioServer/Mixer.cpp
@@ -6,6 +6,7 @@
  */
 
 #include "Mixer.h"
+#include "AK/Format.h"
 #include <AK/Array.h>
 #include <AK/MemoryStream.h>
 #include <AK/NumericLimits.h>
@@ -36,9 +37,6 @@ Mixer::Mixer(NonnullRefPtr<Core::ConfigFile> config)
         return;
     }
 
-    pthread_mutex_init(&m_pending_mutex, nullptr);
-    pthread_cond_init(&m_pending_cond, nullptr);
-
     m_muted = m_config->read_bool_entry("Master", "Mute", false);
     m_main_volume = static_cast<double>(m_config->read_num_entry("Master", "Volume", 100)) / 100.0;
 
@@ -52,11 +50,14 @@ Mixer::~Mixer()
 NonnullRefPtr<ClientAudioStream> Mixer::create_queue(ClientConnection& client)
 {
     auto queue = adopt_ref(*new ClientAudioStream(client));
-    pthread_mutex_lock(&m_pending_mutex);
+    m_pending_mutex.lock();
+
     m_pending_mixing.append(*queue);
-    m_added_queue = true;
-    pthread_cond_signal(&m_pending_cond);
-    pthread_mutex_unlock(&m_pending_mutex);
+
+    m_pending_mutex.unlock();
+    // Signal the mixer thread to start back up, in case nobody was connected before.
+    m_mixing_necessary.signal();
+
     return queue;
 }
 
@@ -65,13 +66,14 @@ void Mixer::mix()
     decltype(m_pending_mixing) active_mix_queues;
 
     for (;;) {
-        if (active_mix_queues.is_empty() || m_added_queue) {
-            pthread_mutex_lock(&m_pending_mutex);
-            pthread_cond_wait(&m_pending_cond, &m_pending_mutex);
+        m_pending_mutex.lock();
+        // While we have nothing to mix, wait on the condition.
+        m_mixing_necessary.wait_while([this, &active_mix_queues]() { return m_pending_mixing.is_empty() && active_mix_queues.is_empty(); });
+        if (!m_pending_mixing.is_empty()) {
             active_mix_queues.extend(move(m_pending_mixing));
-            pthread_mutex_unlock(&m_pending_mutex);
-            m_added_queue = false;
+            m_pending_mixing.clear();
         }
+        m_pending_mutex.unlock();
 
         active_mix_queues.remove_all_matching([&](auto& entry) { return !entry->client(); });
 
diff --git a/Userland/Services/AudioServer/Mixer.h b/Userland/Services/AudioServer/Mixer.h
index cf47523382a..d56d97fef8f 100644
--- a/Userland/Services/AudioServer/Mixer.h
+++ b/Userland/Services/AudioServer/Mixer.h
@@ -19,8 +19,10 @@
 #include <LibAudio/Buffer.h>
 #include <LibCore/File.h>
 #include <LibCore/Timer.h>
+#include <LibThreading/ConditionVariable.h>
 #include <LibThreading/Mutex.h>
 #include <LibThreading/Thread.h>
+#include <sys/types.h>
 
 namespace AudioServer {
 
@@ -125,9 +127,8 @@ private:
     void request_setting_sync();
 
     Vector<NonnullRefPtr<ClientAudioStream>> m_pending_mixing;
-    Atomic<bool> m_added_queue { false };
-    pthread_mutex_t m_pending_mutex;
-    pthread_cond_t m_pending_cond;
+    Threading::Mutex m_pending_mutex;
+    Threading::ConditionVariable m_mixing_necessary { m_pending_mutex };
 
     RefPtr<Core::File> m_device;