Skip to content

raop: fix read-side deadlock in PatchedIceCastClient (#2849)#2850

Open
dantidote wants to merge 6 commits into
postlund:masterfrom
dantidote:fix-2849-read-deadlock
Open

raop: fix read-side deadlock in PatchedIceCastClient (#2849)#2850
dantidote wants to merge 6 commits into
postlund:masterfrom
dantidote:fix-2849-read-deadlock

Conversation

@dantidote

@dantidote dantidote commented May 15, 2026

Copy link
Copy Markdown

fixes #2849

When miniaudio.stream_any's decoder init issues a read larger than (BUFFER_SIZE - position) while position is still inside the seekable headroom, PatchedIceCastClient.read deadlocks against its downloader thread:

  • read() polls until len(buffer) >= num_bytes, where len(buffer) is size (post-position bytes).
  • The downloader's _buffer.fits(BLOCK_SIZE) guard checks the raw buffer length, not size, so once raw is at BUFFER_SIZE it refuses to add even though size has room to grow with consumption.
  • Reader waits for the buffer to grow; downloader cannot grow it until the reader consumes. Polling loop hits DEFAULT_TIMEOUT (10s) and miniaudio surfaces the resulting OperationTimeoutError as DecodeError('failed to init decoder', -1).

Small streams escape because the HTTP body completes within the 10s window: _stop_stream flips True and the loop's other exit fires.

Detect 'raw buffer cannot grow' in the polling loop and let self._buffer.get() return a short read. miniaudio's drmp3 handles short reads by re-calling; once subsequent reads advance position past HEADROOM_SIZE the buffer releases its headroom and becomes a normal sliding window, after which the downloader can refill.

Tests use pytest-httpserver to serve the existing static_3sec.ogg fixture (small, exercises the fast path) and a new audio_long.mp3 (~80 KiB silence, just over BUFFER_SIZE) to trigger the deadlock. On master the long test fails after ~10s with DecodeError(-1); with this fix both tests pass in under a second.

Also bump the miniaudio dev pin from 1.61 to 1.71 in requirements/requirements.txt. The deadlock fires on 1.71's decoder init call sequence (the new seek(0, END) plus extra round-trip leaves position at 2048 when the read(65536) fires); 1.61's init ends with seek(0, START) which resets position before the same read, hiding the bug. Real users pip install pyatv and resolve miniaudio>=1.45 to the newest available (1.71 today, and only 1.71 has Python 3.14 wheels), but the old dev pin meant pyatv's own CI didn't exercise the bug pattern users hit. Bumping aligns CI with what users actually run so the new regression test serves as a real guard.

When miniaudio.stream_any's decoder init issues a read larger than
(BUFFER_SIZE - position) while position is still inside the seekable
headroom, PatchedIceCastClient.read deadlocks against its downloader
thread:

- read() polls until len(buffer) >= num_bytes, where len(buffer) is
  size (post-position bytes).
- The downloader's _buffer.fits(BLOCK_SIZE) guard checks the *raw*
  buffer length, not size, so once raw is at BUFFER_SIZE it refuses
  to add even though size has room to grow with consumption.
- Reader waits for the buffer to grow; downloader cannot grow it
  until the reader consumes. Polling loop hits DEFAULT_TIMEOUT (10s)
  and miniaudio surfaces the resulting OperationTimeoutError as
  DecodeError('failed to init decoder', -1).

Small streams escape because the HTTP body completes within the 10s
window: _stop_stream flips True and the loop's other exit fires.

Detect 'raw buffer cannot grow' in the polling loop and let
self._buffer.get() return a short read. miniaudio's drmp3 handles
short reads by re-calling; once subsequent reads advance position
past HEADROOM_SIZE the buffer releases its headroom and becomes a
normal sliding window, after which the downloader can refill.

Tests use pytest-httpserver to serve the existing static_3sec.ogg
fixture (small, exercises the fast path) and a new audio_long.mp3
(~80 KiB silence, just over BUFFER_SIZE) to trigger the deadlock.
On master the long test fails after ~10s with DecodeError(-1); with
this fix both tests pass in under a second.

Also bump the miniaudio dev pin from 1.61 to 1.71 in
requirements/requirements.txt. The deadlock fires on 1.71's decoder
init call sequence (the new seek(0, END) plus extra round-trip
leaves position at 2048 when the read(65536) fires); 1.61's init
ends with seek(0, START) which resets position before the same
read, hiding the bug. Real users `pip install pyatv` and resolve
miniaudio>=1.45 to the newest available (1.71 today, and only 1.71
has Python 3.14 wheels), but the old dev pin meant pyatv's own CI
didn't exercise the bug pattern users hit. Bumping aligns CI with
what users actually run so the new regression test serves as a real
guard.
@Quentame

Quentame commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

You can add "fixes #2849" in the description so it links the issue with the PR and will auto-close it when the PR got merged 😉

@postlund

Copy link
Copy Markdown
Owner

@dantidote Not really sure what is wrong here, can you take a look? I can merge once tests pass.

werkzeug's server_bind calls socket.getfqdn() on the resolved bind
address, which performs a reverse-DNS lookup. On the macOS and Windows
CI runners that lookup blocks past the 30s pytest-timeout, so the
session-scoped make_httpserver fixture stalls and every httpserver-based
test on that xdist worker is reported as an ERROR at setup (both new
postlund#2849 regression tests, plus the pre-existing data_webserver path).

Override the plugin's make_httpserver fixture to short-circuit
socket.getfqdn while the server binds. The reverse lookup only feeds
werkzeug's cosmetic server_name (WSGI SERVER_NAME), which the loopback
test server never meaningfully uses, so nothing real is lost and startup
becomes instant and deterministic across all platforms.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@dantidote

Copy link
Copy Markdown
Author

I think not all OSes are doing the reverse dns lookup right for localhost. Hoping this fix gets past it!

@postlund

Copy link
Copy Markdown
Owner

I think not all OSes are doing the reverse dns lookup right for localhost. Hoping this fix gets past it!

I forgot, there's already a way to serve files via http in tests:

def data_webserver_fixture(httpserver: HTTPServer, files: typing.Sequence[str]):

Can you switch to rust instead, so as don't re-invent things?

dantidote and others added 2 commits June 20, 2026 11:27
…2849)

The previous fix broke PatchedIceCastClient.read's wait loop on
`not fits(1)`, which only fires at exactly BUFFER_SIZE. But the
downloader stops adding earlier -- once a full BLOCK_SIZE no longer
fits -- so a decoder-init read of BUFFER_SIZE can wait forever in the
gap (BUFFER_SIZE - BLOCK_SIZE, BUFFER_SIZE) and time out (postlund#2849).

That gap is only avoided when the producer's chunks land the buffer on
exactly BUFFER_SIZE (a Content-Length body read in full 8192-byte
chunks). Short-read streams -- icy-metaint, chunked transfer-encoding,
gzip, slow sockets -- settle inside the gap and re-deadlock.

Break on the same threshold the producer uses (`not fits(BLOCK_SIZE)`)
so the reader short-reads when the downloader is blocked; consuming
bytes then frees space and the downloader resumes.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Switch the InternetSource HTTP tests to the existing data_webserver
fixture instead of re-implementing httpserver wiring, per review
feedback.

Add test_read_short_reads_when_producer_blocked, which drives
PatchedIceCastClient with a transport that always returns short reads.
The Content-Length HTTP tests can't reproduce the gap (they read full
8192-byte chunks and land on exactly BUFFER_SIZE), so this guards the
fits(BLOCK_SIZE) fix for icy-metaint / chunked / short-read streams.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@dantidote dantidote force-pushed the fix-2849-read-deadlock branch from f61a1ca to 488933e Compare June 20, 2026 16:18
@dantidote

Copy link
Copy Markdown
Author

OK switched to data_webserver. The tests all passed on my run now: https://github.com/dantidote/pyatv/actions/runs/27881135812

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

miniaudio.DecodeError: failed to init decoder when streaming long MP3 files via RAOP (HomePod) — regression from 0.16.x

3 participants