From d9c1eb860f103320b784a64752a2c5219cdee543 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?kleines=20Filmr=C3=B6llchen?= Date: Wed, 23 Nov 2022 12:50:26 +0100 Subject: [PATCH] AudioServer: Detect improperly detached audio clients Because IPC is used very little in audio server communication, a ping-pong method like WindowServer is neither a good nor a reliable way of detecting detached audio clients. AudioServer was previously doing nothing to detect the kinds of clients that never closed their connection properly, which happens e.g. when a program is force-closed. Due to reference-counting cycles, the associated client connection queues were being kept alive. However, the is_open method of local sockets reliably detects all kinds of disconnected sockets and can easily be adapted for this use case. With this fix, we no longer get "Audio client can't keep up" spam on improperly disconnected clients, and the client queues don't fill up indefinitely, reducing processing and memory usage in AudioServer. --- Userland/Services/AudioServer/Mixer.cpp | 2 +- Userland/Services/AudioServer/Mixer.h | 14 ++++++++++---- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/Userland/Services/AudioServer/Mixer.cpp b/Userland/Services/AudioServer/Mixer.cpp index a6030b5923..7227392996 100644 --- a/Userland/Services/AudioServer/Mixer.cpp +++ b/Userland/Services/AudioServer/Mixer.cpp @@ -70,7 +70,7 @@ void Mixer::mix() } } - active_mix_queues.remove_all_matching([&](auto& entry) { return !entry->client(); }); + active_mix_queues.remove_all_matching([&](auto& entry) { return !entry->is_connected(); }); Array mixed_buffer; diff --git a/Userland/Services/AudioServer/Mixer.h b/Userland/Services/AudioServer/Mixer.h index fcd94b7319..de613ec0d1 100644 --- a/Userland/Services/AudioServer/Mixer.h +++ b/Userland/Services/AudioServer/Mixer.h @@ -47,12 +47,16 @@ public: return false; if (m_in_chunk_location >= m_current_audio_chunk.size()) { - // FIXME: We should send a did_misbehave to the client if the queue is empty, - // but the lifetimes involved mean that we segfault if we try to do that. auto result = m_buffer->try_dequeue(); if (result.is_error()) { - if (result.error() == Audio::AudioQueue::QueueStatus::Empty) - dbgln("Audio client can't keep up!"); + if (result.error() == Audio::AudioQueue::QueueStatus::Empty) { + dbgln("Audio client {} can't keep up!", m_client->client_id()); + // Note: Even though we only check client state here, we will probably close the client much earlier. + if (!m_client->is_open()) { + dbgln("Client socket {} has closed, closing audio server connection.", m_client->client_id()); + m_client->shutdown(); + } + } return false; } @@ -65,6 +69,8 @@ public: return true; } + bool is_connected() const { return m_client && m_client->is_open(); } + ConnectionFromClient* client() { return m_client.ptr(); } void set_buffer(OwnPtr buffer) { m_buffer = move(buffer); }