pipewire: context hardening — combines #226 + #227, reconciled#229
pipewire: context hardening — combines #226 + #227, reconciled#229jcelerier wants to merge 7 commits into
Conversation
In thread-loop mode context::do_sync_locked() called pw_loop_iterate() on the shared pw_loop from the caller while the pw_thread_loop worker iterates the same loop. Two threads iterating one pw_loop corrupt the per-source dispatch bookkeeping (s->priv/s->rmask, stack-local ep[]) and double-fire socket callbacks, crashing in on_remote_data -> pw_protocol_native_connection_flush on a freed/NULL connection (segfault at process exit, during audio filter teardown which calls synchronize() amid protocol traffic). Use the idiomatic pattern instead: for the thread loop, issue pw_core_sync and block on pw_thread_loop_timed_wait, letting the single worker drive the loop; on_core_done/on_core_error wake the waiter via pw_thread_loop_signal. The main-loop path (no worker) keeps driving iteration itself. Require pw_thread_loop_timed_wait in the thread_available gate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
reconnect() wrapped tear_down()+build_connection() in invoke_sync(), running them on the pw_thread_loop worker. But tear_down() stops and destroys that same loop: pthread_join(self) returns EDEADLK (silent no-op) and pw_thread_loop_destroy() then unlocks a loop it is running inside, tripping 'this->recurse > 0' in do_unlock() and hanging the blocking invoke. score's PipeWireAudioFactory::make_engine() calls reconnect() directly when the shared context is broken, so this fires even with auto_reconnect off. Run the tear-down/rebuild/sync directly on the calling thread instead, and bail out if called from the loop thread. tear_down() then joins the worker from a foreign thread (correct) before recreating the loop and core. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two related recursive-lock bugs surfaced by using a MIDI device and audio
together on the shared context:
1. invoke_sync() (used by unsubscribe) called pw_loop_invoke(block=true)
WITHOUT holding the thread-loop lock. pipewire's blocking invoke wraps its
wait in the loop control hooks (do_unlock before / do_lock after), so it
requires the caller to hold the lock exactly once. Unlocked, do_unlock
underflows the recurse counter ('recurse > 0' failed at thread-loop.c:62)
and the stray do_lock leaves the recursive mutex owned by the wrong thread,
deadlocking the next pw_thread_loop_stop() join. Take the lock around the
blocking invoke.
2. synchronize() held pipewire's recursive loop lock across
pw_thread_loop_timed_wait(); pthread_cond_wait cannot fully release a
recursively-held mutex, so nested/concurrent syncs starved the worker and
corrupted the lock. Replace with a private std::condition_variable: take the
loop lock only briefly to issue pw_core_sync, then wait off the loop lock;
on_core_done/on_core_error notify it. Also guard is_in_loop_thread() (drive
iteration directly on the worker instead of waiting on ourselves).
pw_thread_loop_timed_wait is no longer used; drop it from the thread gate.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Three standalone programs under examples/backends/ that exercise the shared thread-loop context fixes: - pipewire_context_sync: many create/synchronize/destroy cycles plus a sustained synchronize() loop (foreign-thread sync must not iterate the loop concurrently with the worker). - pipewire_context_reconnect: repeated reconnect() from the calling thread (must not self-destruct the loop from the worker thread). - pipewire_context_subscriptions: subscribe/unsubscribe churn, MIDI-style filter teardown with live subscriptions, and synchronize() from a subscriber callback (recursive thread-loop lock must stay balanced). Each skips (exit 0) when no PipeWire daemon is reachable and arms a watchdog so a lock-corruption regression fails loudly instead of hanging. Built against the pre-fix headers, pipewire_context_reconnect and pipewire_context_subscriptions reproduce 'recurse > 0' + SIGSEGV. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…aram ranges
Two fixes found by hardware/daemon round-trip testing of ossia score's
PipeWire video path (PipeWire 1.3.0):
1. context::invoke_sync issued pw_loop_invoke(block=true) from foreign
threads WITHOUT holding the thread-loop lock. The blocking-invoke path
in spa/plugins/support/loop.c unconditionally fires the loop-control
hooks around its wait (pw_thread_loop's impl_before/impl_after =
unlock/lock), which exist precisely to release the caller's lock while
it blocks — i.e. the API contract is that the caller holds the lock.
Without it, do_unlock underflows the recurse counter ("'this->recurse
> 0' failed" on stderr) and is refused, and impl_after then ACQUIRES
the mutex on the way out — the calling thread exits owning the loop
mutex forever. The next loop iteration blocks in do_lock and any
subsequent pw_thread_loop_stop deadlocks in pthread_join. Proven with
an instrumented thread-loop.c (abort on underflow): the guilty stack
was invoke_sync <- context::unsubscribe <- subscription dtor.
Fix: wrap both blocking pw_loop_invoke calls in with_lock (the hooks
then release/retake the lock during the wait, as designed).
invoke_async (block=false) is unaffected.
2. format_negotiation::build_buffers_param advertised
SPA_PARAM_BUFFERS_size/stride as fixed Ints, so any producer with
padded rows (GPU-allocated buffers routinely pad strides) failed the
buffers-param intersection and the link died before a single frame.
Advertise CHOICE_RANGE(tight, tight, INT32_MAX) instead; consumers
must read chunk->stride/size per-frame anyway.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014rZgzE8JjWvHDtaVUhxpLE
Keep #226's single lock+guard in invoke_sync (drop #227's with_lock wrappers — stacking both holds the recursive mutex twice, and the loop control hooks release exactly one level, so the blocking invoke would never be dispatched: a new deadlock). Add a m_thread_loop null check to the early return so a mid-teardown call cannot lock a null loop. Retains #227's format.hpp buffers size/stride ranges and #226's synchronize/reconnect fixes + regression examples unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014rZgzE8JjWvHDtaVUhxpLE
The lock contract of pw_loop_invoke(block=true) from a foreign thread flip-flopped across PipeWire releases: every 1.2.x / 1.3.81+ / 1.4.x release fires the thread-loop control hooks around the wait (caller MUST hold the lock exactly once — calling unlocked underflows recurse and strands the mutex), but upstream removed and reverted those hooks three times, and the hook-less state shipped in the 1.3.0-1.3.80 and 1.5.0-1.5.80 dev series — where a held lock deadlocks instead. Since 1.5.81 the wait is self-managing and either pattern works. No blocking invoke is correct against all of them. So stop using one: queue a NON-blocking invoke (fires no hooks, needs no lock on any series) and wait on a private mutex/condition variable that the trampoline notifies, bounded by cfg.sync_deadline so teardown races return instead of hanging. The trampoline runs fn under the payload mutex and honors an `abandoned` flag flipped by a timed-out caller under that same mutex, so by-reference captures are either consumed before the timeout is observed or never touched; the payload is shared_ptr-owned from both sides. If the loop rejects the queue (teardown/OOM) fn runs inline — the item can never double-run. Validated: score PipewireRoundtrip 28-cell matrix (24 PASS + 4 PASS(fallback), zero recurse asserts) and the three watchdog-armed regression examples (sync: 320k round-trips; reconnect: 8 generations; subscriptions incl. on-loop-thread synchronize) on PipeWire 1.3-master. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014rZgzE8JjWvHDtaVUhxpLE
|
Follow-up commit 91ecbd5: Version archaeology (verified per tag in the pipewire sources,
No blocking invoke is correct against all three states — so the new Validation on this branch: ossia score's 28-cell PipeWire round-trip matrix (24 PASS + 4 honest-fallback, zero |
Combines #226 (@ogauthiersat) and #227 into one coherent series, with the two
invoke_syncfixes reconciled — merging both PRs as-is would auto-merge cleanly and deadlock (each adds its own lock around the blocking invoke; the recursive mutex would be held twice, and the loop-control hooks release exactly one level, so the worker could never dispatch the invoke).Contents:
invoke_synclock+guard, thesynchronize()rewrite (no more two threads iterating onepw_loop—pw_core_sync+ private CV notified fromon_core_done), thereconnect()self-destruct fix (no morepthread_join(self)via invoke_sync), and the three watchdog-armed regression examples.format.hppchange kept:SPA_PARAM_BUFFERS_size/strideasCHOICE_RANGE(tight, tight, INT32_MAX)so padded-stride producers (GPU allocations) can link.with_lockwrappers dropped in favor of pipewire: fix thread-loop lifetime & recursive-lock crashes in the shared context #226's single guard (same semantics — lock held exactly once at the invoke, per the hook contract inthread-loop.cimpl_before/impl_after); plus a defensivem_thread_loopnull check in the early return so a mid-teardowninvoke_synccan't lock a null loop.Validation: ossia score's
PipewireRoundtripharness against a live PipeWire 1.3.0 daemon — 28-cell format×transport matrix (SHM + DMA-BUF, OpenGL + Vulkan, 12 pixel formats incl. padded strides): 24 PASS + 4 PASS(honest-fallback), zero deadlocks/asserts across repeated stream create/destroy cycles. #226's regression examples compile and are included unchanged.Supersedes #227 (I'll close it); #226 can be closed in favor of this or merged first — this branch contains it verbatim either way.
🤖 Generated with Claude Code
https://claude.ai/code/session_014rZgzE8JjWvHDtaVUhxpLE
Update (91ecbd5):
invoke_syncis now rewritten to a non-blockingpw_loop_invoke+ private mutex/CV with async_deadlinebound, instead of a lock-held blocking invoke. Rationale: the blocking-invoke lock contract flip-flopped across PipeWire releases (hooks removed/reverted three times upstream; 1.3.0–1.3.80 and 1.5.0–1.5.80 dev clients deadlock with a held lock, all 1.2/1.4 releases require it, 1.5.81+ accepts either) — the non-blocking pattern is contract-stable on every series. Details and the per-series matrix in the comments below. Validated with score's 28-cell round-trip harness and the three regression examples from #226.