Skip to content

Commit 125122a

Browse files
kleinesfilmroellchenlinusg
authored andcommitted
LibAudio: Prevent racy eternal deadlock of the audio enqueue thread
The audio enqueuer thread goes to sleep when there is no more audio data present, and through normal Core::EventLoop events it can be woken up. However, that waking up only happens when the thread is not currently running, so that the wake-up events don't queue up and cause weirdness. The atomic variable responsible for keeping track of whether the thread is active can lead to a racy deadlock however, where the audio enqueuer thread will never wake up again despite there being audio data to enqueue. Consider this scenario: - Main thread calls into async_enqueue. It detects that according to the atomic variable, the other thread is still running, skipping the event queue wake. - Enqueuer thread has just finished playing the last chunk of audio and detects that there is no audio left. It enters the if block with the dbgln "Reached end of provided audio data..." - Main thread enqueues audio, making the user sample queue non-empty. - Enqueuer thread does not check this condition again, instead setting the atomic variable to indicate that it is not running. It exits into an event loop sleep. - Main thread exits async_enqueue. The calling audio enqueuing system (see e.g. Piano, but all of them function similarly) will wait until the enqueuer thread has played enough samples before async_enqueue is called again. However, since the enqueuer thread will never play any audio, this condition is never fulfilled and audio playback deadlocks This commit fixes that by allowing the event loop to not enqueue an event that already exists, therefore overloading the audio enqueuer event loop by at maximum one message in weird situations. We entirely get rid of the atomic variable and the race condition is prevented.
1 parent 763cda2 commit 125122a

File tree

4 files changed

+19
-6
lines changed

4 files changed

+19
-6
lines changed

Userland/Libraries/LibAudio/ConnectionToServer.cpp

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,7 @@ ErrorOr<void> ConnectionToServer::async_enqueue(FixedArray<Sample>&& samples)
6262
update_good_sleep_time();
6363
m_user_queue->append(move(samples));
6464
// Wake the background thread to make sure it starts enqueuing audio.
65-
if (!m_audio_enqueuer_active.load())
66-
m_enqueuer_loop->post_event(*this, make<Core::CustomEvent>(0), Core::EventLoop::ShouldWake::Yes);
65+
m_enqueuer_loop->wake_once(*this, 0);
6766
async_start_playback();
6867

6968
return {};
@@ -87,8 +86,6 @@ void ConnectionToServer::custom_event(Core::CustomEvent&)
8786
{
8887
Array<Sample, AUDIO_BUFFER_SIZE> next_chunk;
8988
while (true) {
90-
m_audio_enqueuer_active.store(true);
91-
9289
if (m_user_queue->is_empty()) {
9390
dbgln("Reached end of provided audio data, going to sleep");
9491
break;
@@ -107,7 +104,6 @@ void ConnectionToServer::custom_event(Core::CustomEvent&)
107104
if (result.is_error())
108105
dbgln("Error while writing samples to shared buffer: {}", result.error());
109106
}
110-
m_audio_enqueuer_active.store(false);
111107
}
112108

113109
ErrorOr<void, AudioQueue::QueueStatus> ConnectionToServer::realtime_enqueue(Array<Sample, AUDIO_BUFFER_SIZE> samples)

Userland/Libraries/LibAudio/ConnectionToServer.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,6 @@ class ConnectionToServer final
8383
NonnullRefPtr<Threading::Thread> m_background_audio_enqueuer;
8484
Core::EventLoop* m_enqueuer_loop;
8585
Threading::Mutex m_enqueuer_loop_destruction;
86-
Atomic<bool> m_audio_enqueuer_active { false };
8786

8887
// A good amount of time to sleep when the queue is full.
8988
// (Only used for non-realtime enqueues)

Userland/Libraries/LibCore/EventLoop.cpp

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -501,6 +501,23 @@ void EventLoop::post_event(Object& receiver, NonnullOwnPtr<Event>&& event, Shoul
501501
wake();
502502
}
503503

504+
void EventLoop::wake_once(Object& receiver, int custom_event_type)
505+
{
506+
Threading::MutexLocker lock(m_private->lock);
507+
dbgln_if(EVENTLOOP_DEBUG, "Core::EventLoop::wake_once: event type {}", custom_event_type);
508+
auto identical_events = m_queued_events.find_if([&](auto& queued_event) {
509+
if (queued_event.receiver.is_null())
510+
return false;
511+
auto const& event = queued_event.event;
512+
auto is_receiver_identical = queued_event.receiver.ptr() == &receiver;
513+
auto event_id_matches = event->type() == Event::Type::Custom && static_cast<CustomEvent const*>(event.ptr())->custom_type() == custom_event_type;
514+
return is_receiver_identical && event_id_matches;
515+
});
516+
// Event is not in the queue yet, so we want to wake.
517+
if (identical_events.is_end())
518+
post_event(receiver, make<CustomEvent>(custom_event_type), ShouldWake::Yes);
519+
}
520+
504521
SignalHandlers::SignalHandlers(int signo, void (*handle_signal)(int))
505522
: m_signo(signo)
506523
, m_original_handler(signal(signo, handle_signal))

Userland/Libraries/LibCore/EventLoop.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ class EventLoop {
5656
void spin_until(Function<bool()>);
5757

5858
void post_event(Object& receiver, NonnullOwnPtr<Event>&&, ShouldWake = ShouldWake::No);
59+
void wake_once(Object& receiver, int custom_event_type);
5960

6061
static EventLoop& current();
6162

0 commit comments

Comments
 (0)