Never block the control thread indefinitely on a stalled render thread#635
Open
cmzy wants to merge 2 commits into
Open
Never block the control thread indefinitely on a stalled render thread#635cmzy wants to merge 2 commits into
cmzy wants to merge 2 commits into
Conversation
Two related hang surfaces, both triggered by a render thread that stops draining its channels - in practice: the output device was unplugged or the driver died, upon which cpal only invokes err_fn (which logs) and stops issuing callbacks. 1. send_control_msg() performed a blocking send() into the bounded(256) control channel. Once the render callback stops draining, the 257th message blocks the sending thread forever. Applications hosting a scripting engine send control messages from their main thread, which then hangs wholesale. 2. suspend_sync()/resume_sync()/close_sync() send a oneshot notify through the same machinery and then wait with a bare recv() for the render thread to acknowledge. With the render thread gone the notify never arrives. close() is exactly the API an application calls right after a device disappears, and it hung forever. Manual reproduction: play audio through a USB audio interface, unplug it, then keep using the context (schedule >256 param changes, or call ctx.close()). The control thread never returns. Fix: keep the channel bounded (the receiving side lives on the render thread; a bounded channel is allocation-free and realtime-safe) and keep blocking back-pressure semantics (single FIFO: no drops, no reordering), but replace the indefinite waits with waits on render-thread liveness. While the channel is full (or the ack is pending), poll every 50 ms and check whether frames_played advanced - the render thread bumps it by 128 per rendered quantum. Progress means plain back-pressure: keep waiting, exactly like before. Zero progress across a 2 s window (~700 missed quanta - not busyness) means the render side is gone: mark the context Closed, drop the in-flight message / stop waiting, and let the existing Closed short-circuit discard all subsequent messages. Dropping is safe at that point: a Closed context discards control messages anyway, and there is no render thread left to execute them. Liveness (playhead progress) is used instead of a fixed send timeout so that healthy-but-busy devices and device startup cannot misfire the detection. Offline contexts are unaffected (their control channel is unbounded, and their playhead only advances under explicit rendering calls on the control thread itself). A regression test guards the false-positive direction: a suspended context keeps draining its channel without advancing the playhead, so flooding it with thousands of messages must neither hang nor close it. The stall direction requires actual device removal and remains a manual test.
set_sink_id_sync had two failure paths that misbehaved when the audio device disappears mid-protocol (unplugged USB interface, dead driver - cpal only reports through err_fn and stops issuing callbacks): 1. After sending CloseAndRecycle it waited for the render thread to hand back the audio graph with a bare graph_recv.recv().unwrap(). With the render thread stalled the graph never arrives and the control thread blocks forever. "Device disappeared, application switches to another sink" is precisely the sequence that hits this. 2. If io::build_output for the new sink failed after the old backend had already been closed, the error was returned but the context kept claiming its previous state - typically Running - while the graph had been taken out of the render thread and dropped. Nothing can produce audio at that point; every later call quietly operates on a dead context. The graph wait now uses the same frames_played liveness rule introduced for send_control_msg and the suspend/resume/close acknowledgements: poll every 50 ms and only declare the render thread gone after 2 s without playhead progress. Progress means plain back-pressure and the wait continues exactly as before. On a stall (or a disconnected reply channel) the context is marked Closed and the call returns an error instead of hanging. A build_output failure after teardown also marks the context Closed so the observable state reflects reality. The stall direction requires physically removing a device and remains a manual test. A regression test covers the ordering guarantee that must not change: rejecting an invalid sinkId happens before any teardown and must leave the context running and usable.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR groups two commits that make the control thread resilient to a stalled render thread; the sink-change fix reuses the liveness rule introduced by the first commit, so they are kept together.
Never block the control thread indefinitely on a stalled render thread
Two related hang surfaces, both triggered by a render thread that stops
draining its channels - in practice: the output device was unplugged or
the driver died, upon which cpal only invokes err_fn (which logs) and
stops issuing callbacks.
send_control_msg() performed a blocking send() into the bounded(256)
control channel. Once the render callback stops draining, the 257th
message blocks the sending thread forever. Applications hosting a
scripting engine send control messages from their main thread, which
then hangs wholesale.
suspend_sync()/resume_sync()/close_sync() send a oneshot notify
through the same machinery and then wait with a bare recv() for the
render thread to acknowledge. With the render thread gone the notify
never arrives. close() is exactly the API an application calls right
after a device disappears, and it hung forever.
Manual reproduction: play audio through a USB audio interface, unplug
it, then keep using the context (schedule >256 param changes, or call
ctx.close()). The control thread never returns.
Fix: keep the channel bounded (the receiving side lives on the render
thread; a bounded channel is allocation-free and realtime-safe) and keep
blocking back-pressure semantics (single FIFO: no drops, no reordering),
but replace the indefinite waits with waits on render-thread liveness.
While the channel is full (or the ack is pending), poll every 50 ms and
check whether frames_played advanced - the render thread bumps it by 128
per rendered quantum. Progress means plain back-pressure: keep waiting,
exactly like before. Zero progress across a 2 s window (~700 missed
quanta - not busyness) means the render side is gone: mark the context
Closed, drop the in-flight message / stop waiting, and let the existing
Closed short-circuit discard all subsequent messages. Dropping is safe
at that point: a Closed context discards control messages anyway, and
there is no render thread left to execute them.
Liveness (playhead progress) is used instead of a fixed send timeout so
that healthy-but-busy devices and device startup cannot misfire the
detection. Offline contexts are unaffected (their control channel is
unbounded, and their playhead only advances under explicit rendering
calls on the control thread itself).
A regression test guards the false-positive direction: a suspended
context keeps draining its channel without advancing the playhead, so
flooding it with thousands of messages must neither hang nor close it.
The stall direction requires actual device removal and remains a manual
test.
Do not hang or silently kill the context when a sink change fails
set_sink_id_sync had two failure paths that misbehaved when the audio
device disappears mid-protocol (unplugged USB interface, dead driver -
cpal only reports through err_fn and stops issuing callbacks):
After sending CloseAndRecycle it waited for the render thread to
hand back the audio graph with a bare graph_recv.recv().unwrap().
With the render thread stalled the graph never arrives and the
control thread blocks forever. "Device disappeared, application
switches to another sink" is precisely the sequence that hits this.
If io::build_output for the new sink failed after the old backend
had already been closed, the error was returned but the context kept
claiming its previous state - typically Running - while the graph
had been taken out of the render thread and dropped. Nothing can
produce audio at that point; every later call quietly operates on a
dead context.
The graph wait now uses the same frames_played liveness rule introduced
for send_control_msg and the suspend/resume/close acknowledgements:
poll every 50 ms and only declare the render thread gone after 2 s
without playhead progress. Progress means plain back-pressure and the
wait continues exactly as before. On a stall (or a disconnected reply
channel) the context is marked Closed and the call returns an error
instead of hanging. A build_output failure after teardown also marks
the context Closed so the observable state reflects reality.
The stall direction requires physically removing a device and remains a
manual test. A regression test covers the ordering guarantee that must
not change: rejecting an invalid sinkId happens before any teardown and
must leave the context running and usable.