Conversation
Previously the wireplumber module connected to PipeWire once in its constructor and had no handling for the connection being lost. When PipeWire or the wireplumber service restarted (or crashed), the module went stale/blank and never recovered until Waybar itself was restarted. Connect to the WpCore "disconnected" signal and, on disconnect, schedule a bounded main-loop retry (Glib::signal_timeout) that tears down the now invalid core/object-manager/mixer-api references and rebuilds the whole connection from scratch, re-running the async API and object-manager setup. Connection setup/teardown is factored into setupConnection() and teardownConnection() so startup and reconnect share one code path. The reconnect timer is cancelled in the destructor and the existing isModuleAlive() registry guard still protects in-flight async callbacks, so teardown during a pending reconnect stays safe. Fixes #2882.
| spdlog::error("[{}]: Could not connect to PipeWire: '{}'", name_, type_); | ||
| throw std::runtime_error("Could not connect to PipeWire\n"); | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (non-blocking): onMixerApiLoaded (unchanged by this diff, but now reachable repeatedly) ends with:
Waybar/src/modules/wireplumber.cpp
Lines 574 to 575 in 0b88b4e
Before this PR that ran once, from the constructor. Now setupConnection() re-runs the same async chain (asyncLoadRequiredApiModules → ... → onMixerApiLoaded) on every successful reconnect, and sigc::signal::connect doesn't dedupe — event_box_ isn't recreated by setupConnection()/teardownConnection(), so a duplicate scroll handler is added per reconnect.
Behavior stays correct: GTK's scroll-event uses the boolean handled accumulator, so emission stops at the first handler returning TRUE and the duplicates never run — they just accumulate as dead connections (plus a redundant add_events call) for the life of the module.
Since this wiring only needs to happen once per module instance, move it here and delete it from onMixerApiLoaded (linked lines above). handleScroll already no-ops safely while mixer_api_ is null, so wiring it before the first successful connect is safe.
| } | |
| event_box_.add_events(Gdk::SCROLL_MASK | Gdk::SMOOTH_SCROLL_MASK); | |
| event_box_.signal_scroll_event().connect(sigc::mem_fun(*this, &Wireplumber::handleScroll)); | |
| } |
| bool waybar::modules::Wireplumber::onReconnectTimeout() { | ||
| teardownConnection(); | ||
| spdlog::info("[{}]: attempting to reconnect to PipeWire...", name_); | ||
| if (setupConnection()) { | ||
| spdlog::info("[{}]: reconnected to PipeWire", name_); | ||
| return false; | ||
| } | ||
| teardownConnection(); | ||
| spdlog::debug("[{}]: reconnect failed; retrying in {} ms", name_, kReconnectIntervalMs); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
issue (blocking): stale async completions from a superseded connection can corrupt the new generation's state
onReconnectTimeout rebuilds wp_core_/om_/apis_/pending_plugins_ in place on the same self, but wp_core_load_component() / wp_object_activate() are called with no GCancellable, and isModuleAlive() only checks that the C++ object still exists — not which connection generation a callback belongs to. If PipeWire drops again while the async chain (onDefaultNodesApiLoaded → onMixerApiLoaded → onPluginActivated) is still in flight — likely with a crash-looping service, the exact case this PR targets — the stale callback fires against the rebuilt connection's fields:
Waybar/src/modules/wireplumber.cpp
Lines 488 to 490 in 0b88b4e
A stray decrement corrupts the new generation's pending_plugins_ (never reaches 0, or hits it early), and wp_core_install_object_manager can run on the new om_/wp_core_ out of sequence — reproducing the stale/blank symptom of #2882 via a second disconnect during the reconnect window.
Fix: tag each generation (epoch counter bumped in setupConnection(), captured and compared in the callbacks), or pass a per-generation GCancellable cancelled in teardownConnection(), so completions from a torn-down generation are no-ops.
…e scroll once Addresses review on #5168: - Generational aliasing (blocking): setupConnection()/onReconnectTimeout() rebuild wp_core_/om_/pending_plugins_ in place on the same self, but the async load/activate callbacks carried no generation, and isModuleAlive() only proves self still exists. If PipeWire dropped again while a previous connection's async chain was still in flight, a stale completion would run against the rebuilt connection (a stray --pending_plugins_, an out-of-order install_object_manager), re-creating #2882's stale/blank state. Each async call now carries an AsyncCall{self, generation}; connection_generation_ is bumped in setupConnection(), and every callback drops out when its generation no longer matches (checked after isModuleAlive short-circuits). - Duplicate scroll handlers: onMixerApiLoaded re-runs on every reconnect and connected a new scroll handler each time (dead but accumulating). Moved the one-time wiring to the constructor; handleScroll no-ops while mixer_api_ is null, so wiring it before the first connect is safe.
|
Thanks for the thorough review — both are addressed in the latest push. Generational aliasing (blocking): you're right that Duplicate scroll handler (non-blocking): moved the |
…OB write) g_variant_lookup with the "b" format writes a gboolean (gint, 4 bytes), but muted_ and source_muted_ are C++ bool members (1 byte). Passing their addresses caused a 3-byte out-of-bounds write past the member (undefined behavior). Read into a gboolean temporary and assign back to the bool, preserving the prior value when "mute" is absent.
2 participants
Please review carefully — this refactors the wireplumber module's connection lifecycle, and I couldn't runtime-test it.
wireplumber: no reconnect when PipeWire/WirePlumber restarts (#2882). The module's connection was one-shot: the constructor called
wp_core_connect()once, connected only to the object manager'sinstalledsignal, and had noWpCoredisconnectedhandler or connect-failure recovery. So when PipeWire/WirePlumber restarted or crashed, the core silently disconnected,om_/mixer_api_/def_nodes_api_went dead, and the module stayed stale/blank until Waybar was restarted.This splits the one-shot init into a reusable
setupConnection()/teardownConnection()pair (used by both the constructor and reconnect), connects theWpCoredisconnectedsignal, and adds a boundedGlib::signal_timeout(2s, no busy loop) that tears down the dead connection and rebuilds it, retrying until PipeWire is back and re-arming on a later disconnect. It preserves the recent async-callback use-after-free guard (isModuleAlive()/registry) and cancels the reconnect timer + tears down cleanly in the destructor; it also clears the cached default node-name out-params on teardown to avoid a per-reconnect leak.handleScrollalready no-ops whilemixer_api_is null, so the reconnect window is safe.