fix(pulseaudio): release server-side stream on Stream::drop#1189
fix(pulseaudio): release server-side stream on Stream::drop#1189knz wants to merge 4 commits intoRustAudio:masterfrom
Conversation
|
NB: cleanup is asynchronous (the delete is queued, not awaited), so a |
|
Thank you for the analysis and follow-up fix! This seems the right approach. One concern: the driver thread is never joined. Fixing this requires storing the worker thread's if handle.thread().id() != std::thread::current().id() {
let _ = handle.join();
}Would you take that along with this PR? |
|
Done; added a commit, see if you like it. |
|
hm I sense there's something wrong. let me investigate further. |
|
Okay this patch is actually fine! (while testing it I ran into the unrelated issue #1190. I've been able to work around that issue in my own code, but it may be worth investigating further.) |
knz
force-pushed
the
fix/pulseaudio-stream-leak-1188
branch
from
2c8009c to
1149632
Compare
|
There's a new failure in CI, but it appears unrelated to my change. |
roderickvd
left a comment
roderickvd
left a comment
There was a problem hiding this comment.
Thanks for the PR. Please take care to keep the documentation style consistent with the rest and ensure what seems to be your LLM follows that.
Don't worry about the unrelated Rust nightly failure in the CI.
knz
force-pushed
the
fix/pulseaudio-stream-leak-1188
branch
from
1149632 to
5597cb7
Compare
|
I'll handle the rebase; hold on |
I believe this entry was dropped accidentally while reordering the file in dccd6b4.
The PulseAudio backend spawns a driver thread that calls `pulseaudio::PlaybackStream::play_all`, which awaits source EOF before draining and exiting. EOF for a `PlaybackSource` is signalled by `poll_read` returning `0`, but the data-callback wrapper used by `new_playback` always reports the full buffer length to satisfy cpal's no-short-write contract. The driver thread therefore parks in `source_eof` for the lifetime of the process. Because that thread holds a `PlaybackStream` clone (an `Arc<InnerPlaybackStream>`), the inner Arc count never drops to zero, `InnerPlaybackStream::drop` never fires, and the `DeletePlaybackStream` command it would otherwise queue is never sent. Each `Stream` created and dropped by the user thus leaks one server-side stream and one OS thread; long-running clients accumulate `pactl list short sink-inputs` entries until PulseAudio runs out of channels. Queue a `DeletePlaybackStream` from `Stream::drop` so the reactor removes the stream state, drops the source's `eof_tx` channel, and wakes the driver thread with a cancellation error. `now_or_never` mirrors `InnerPlaybackStream::drop` and avoids blocking in `Drop`. Fixes RustAudio#1188.
The previous fix queued `DeletePlaybackStream` from `Stream::drop` but returned immediately. Because the delete is processed asynchronously on the reactor and the play_all driver thread only exits *after* the reactor wakes it, a process that exits shortly after dropping the Stream can race the worker threads — the delete may never reach the server, leaving exactly the leak this PR set out to fix. Store the spawned threads' JoinHandles on `Stream` and join them in `Drop` after signalling cancellation. Skip handles whose thread id matches the current thread, since the user's error_callback runs on those workers and may itself drop the Stream; joining ourselves would deadlock. Mirrors the pattern roderickvd is introducing for PipeWire in RustAudio#1187.
The play_all driver thread surfaces ClientError::Disconnected when the reactor drops the source's eof_tx — which happens not only on real server disconnect but also as a *direct consequence* of Stream::drop queueing DeletePlaybackStream and the server processing it. The error callback then fires "PulseAudio client disconnected" on every clean teardown, despite the connection being healthy. Check the cancel flag in the play_all spawn before emit_error, mirroring the latency thread's existing pre-poll cancel check. After a clean Stream::drop the cancel flag is already set when play_all unblocks, so the spurious notification is silenced; real disconnects (reactor exits, socket closed) leave the flag clear and still surface through the user's error_callback.
knz
force-pushed
the
fix/pulseaudio-stream-leak-1188
branch
from
5597cb7 to
fb8131a
Compare
|
rebase completed |
2 participants
The PulseAudio backend spawns a driver thread that calls
pulseaudio::PlaybackStream::play_all, which awaits source EOF before draining and exiting. EOF for aPlaybackSourceis signalled bypoll_readreturning0, but the data-callback wrapper used bynew_playbackalways reports the full buffer length to satisfy cpal's no-short-write contract. The driver thread therefore parks insource_eoffor the lifetime of the process.Because that thread holds a
PlaybackStreamclone (anArc<InnerPlaybackStream>), the inner Arc count never drops to zero,InnerPlaybackStream::dropnever fires, and theDeletePlaybackStreamcommand it would otherwise queue is never sent. EachStreamcreated and dropped by the user thus leaks one server-side stream and one OS thread; long-running clients accumulatepactl list short sink-inputsentries until PulseAudio runs out of channels.Queue a
DeletePlaybackStreamfromStream::dropso the reactor removes the stream state, drops the source'seof_txchannel, and wakes the driver thread with a cancellation error.now_or_nevermirrorsInnerPlaybackStream::dropand avoids blocking inDrop.Fixes #1188.