From 909c7634ef459225fdcc2c87ec3942f52f7899ce Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Thu, 10 Sep 2026 11:39:39 -0400 Subject: [PATCH 01/11] Add a synchronous start/stop/restart lifecycle to SendspinClient start() starts the role threads and arms the WebSocket server, rolling back the roles that did start if one fails. stop() goodbyes every peer, waits up to GOODBYE_FLUSH_TIMEOUT_MS for the sends to complete, tears the server, connections, and role threads down regardless, resets every role, and delivers the clear callbacks before returning. is_started() reports the state; loop() is a no-op and connect_to() is refused while stopped. start_server() stays as a deprecated alias. A stopping_ guard refuses start() and ignores stop() from a listener callback fired inside the teardown. ConnectionManager::stop() closes admission first (a peer delivered during the wait is rejected with a goodbye), counts goodbye completions in a shared GoodbyeWait record so a late completion on a transport thread touches nothing stop() owns, and drops every queued lifecycle event outside the locks. Restart correctness: the player role now re-creates its sync thread on every start() (the init guard used to swallow the thread start too), SyncTask::stop() clears TASK_RUNNING and resets the encoded ring after the join, the sync thread returns a held codec header on its idle-state exits, and the artwork and visualizer roles discard their queue/ring content after their joins and clear stale command flags before spawning. The client destructor performs the transport half of stop() only, so a consumer that destroyed its listeners first is never called into. --- docs/integration-guide.md | 33 +- docs/internals.md | 41 ++- examples/basic_client/main.cpp | 4 +- examples/tui_client/main.cpp | 4 +- include/sendspin/client.h | 81 ++++- include/sendspin/config.h | 2 +- src/artwork_role.cpp | 9 + src/client.cpp | 133 ++++++-- src/connection_manager.cpp | 88 ++++- src/connection_manager.h | 86 ++++- src/player_role.cpp | 27 +- src/player_role_impl.h | 2 + src/sync_task.cpp | 16 + src/sync_task.h | 8 +- src/visualizer_role.cpp | 11 + tests/CMakeLists.txt | 1 + tests/test_client_lifecycle.cpp | 489 ++++++++++++++++++++++++++++ tests/test_client_teardown.cpp | 2 +- tests/test_connection_lifecycle.cpp | 14 +- 19 files changed, 963 insertions(+), 88 deletions(-) create mode 100644 tests/test_client_lifecycle.cpp diff --git a/docs/integration-guide.md b/docs/integration-guide.md index a7b57148..23a08055 100644 --- a/docs/integration-guide.md +++ b/docs/integration-guide.md @@ -51,7 +51,7 @@ SendspinClient client(std::move(config)); ## Step 2: Add Roles -Add only the roles your application needs. All roles must be added before calling `start_server()`. +Add only the roles your application needs. All roles must be added before calling `start()`. ### Player Role (Audio Playback) @@ -499,7 +499,7 @@ struct MyClientListener : SendspinClientListener { ## Step 5: Wire Everything Together -Listeners and providers are set as raw pointers. They must outlive the client. +Listeners and providers are set as raw pointers. They must stay alive for as long as the client can call them: until `stop()` returns, or until the client is destroyed if `stop()` is never called. The destructor itself never invokes a listener (see [Stopping and Restarting](#stopping-and-restarting)). ```cpp MyPlayerListener player_listener; @@ -520,9 +520,10 @@ client.set_persistence_provider(&persistence_provider); // Optional ## Step 6: Start and Run ```cpp -// Start the WebSocket server and sync task. +// Start the role threads and arm the WebSocket server (it comes up on the first loop() tick +// after the network provider reports ready). // Task priorities and PSRAM settings are taken from SendspinClientConfig. -if (!client.start_server()) { +if (!client.start()) { // Handle failure return 1; } @@ -537,10 +538,28 @@ while (running) { std::this_thread::sleep_for(std::chrono::milliseconds(10)); } -// Clean shutdown -client.disconnect(SendspinGoodbyeReason::SHUTDOWN); +// Clean shutdown: goodbye every peer, tear everything down, deliver the clear callbacks. +client.stop(); ``` +`start_server()` is a deprecated alias of `start()`. + +## Stopping and Restarting + +`stop()` is synchronous: when it returns the client is fully stopped. It sends a `client/goodbye` (reason `shutdown`) to every peer, waits up to a short bound (50 ms) for those sends to complete, then closes the server and every connection regardless, joins the role threads, resets every role, and delivers the roles' clear callbacks (`on_stream_end()`, `on_image_clear()`, `on_visualizer_stream_end()`, `on_metadata_clear()`, `on_controller_state_clear()`, `on_color_clear()`) before returning. It is a no-op on a stopped client. `is_started()` reports the state, and `loop()` is a no-op while stopped. + +Restarting is `start()` again; start, stop, and start again can be repeated indefinitely, and a restarted client begins with no connection, no group state, and no role state from before the stop. + +`stop()` may block, but the wait is bounded. Besides the goodbye bound it includes: + +- The transports' own close. The host server waits up to 300 ms per connection for the WebSocket close handshake. The ESP server waits for the httpd task to exit, which polls at 100 ms and first finishes any queued send, which can take up to httpd's send timeout for a peer that has stopped reading. +- An outbound `connect_to()` connection's transport stop, which is synchronous (`esp_websocket_client_stop()` / `ix::WebSocket::stop()`). +- A listener callback already running on a role thread: the join cannot interrupt it. `on_audio_write()` is bounded by its `timeout_ms`; `on_image_decode()` has no bound. + +Listener callbacks fire from inside `stop()`. One that calls `start()` gets `false` and starts nothing; one that calls `stop()` or `connect_to()` is ignored. Call `stop()` only from the main loop thread: from a role-thread callback it would join the calling thread. + +Destroying a running client performs the transport half of `stop()` (goodbye, bounded wait, close, join) but delivers no listener callback, so a consumer that destroys its listeners before the client is never called into. Call `stop()` first when the clear callbacks matter. + ## Sending Commands If you added the controller role, use it to send playback commands. `send_command` takes a `ClientCommandControllerObject`, built with designated initializers - set only the field the command uses: @@ -700,7 +719,7 @@ int main() { player.set_listener(&player_listener); client.set_network_provider(&network); - client.start_server(); + client.start(); while (true) { client.loop(); diff --git a/docs/internals.md b/docs/internals.md index 29e0a9e4..e63cf209 100644 --- a/docs/internals.md +++ b/docs/internals.md @@ -48,20 +48,21 @@ On host builds, `platform_configure_thread()` is a no-op; threads use OS default 1. `SyncTask::start()` configures the thread and spawns it. 2. The caller blocks until the thread reaches IDLE state (`TASK_IDLE` event flag) or exits early due to an allocation failure (`TASK_STOPPED`). 3. The thread runs a persistent outer loop for the lifetime of the client. -4. `SyncTask::stop()` sets `COMMAND_STOP`, wakes the ring buffer receive via `wake_receiver()`, and joins the thread. Called from `SyncTask`'s destructor, which is triggered by `sync_task_.reset()` in `PlayerRole::Impl`'s destructor. +4. `SyncTask::stop()` sets `COMMAND_STOP`, wakes the ring buffer receive via `wake_receiver()`, and joins the thread; after the join it clears `TASK_RUNNING` (a stop mid-stream leaves it set, and the player's sync-idle gate must read a stopped task as idle) and resets the encoded ring buffer, so a later `start()` begins with an empty ring. Called from `PlayerRole::Impl::stop()` (`SendspinClient::stop()` and the client destructor) and from `SyncTask`'s destructor, which is triggered by `sync_task_.reset()` in `PlayerRole::Impl`'s destructor. +5. `SyncTask::start()` clears every command and state flag before spawning, so a restart after `stop()` inherits nothing from the previous thread. **Visualizer drain** (`src/visualizer_role.cpp`): 1. `VisualizerRole::Impl::start()` spawns the drain thread. 2. The thread blocks on ring buffer receives; commands interrupt the receive immediately via `wake_receiver()`. The 5 s receive timeout is only a fallback against a missed wake. -3. `VisualizerRole::Impl` destructor sets `COMMAND_STOP`, wakes the ring buffer receive, and joins. +3. `VisualizerRole::Impl::stop()` (from `SendspinClient::stop()`, the client destructor, and the `Impl` destructor) sets `COMMAND_STOP`, wakes the ring buffer receive, joins, and then flushes the ring buffer: with the thread joined it is the ring's only consumer, and a restart must not deliver the previous session's frames. `start()` clears `COMMAND_STOP`, `COMMAND_FLUSH`, and `COMMAND_CLEAR` before spawning, since `cleanup()` on a stopped role leaves a flush flagged. **Artwork decode** (`src/artwork_role.cpp`): 1. `ArtworkRole::Impl::start()` spawns the decode thread. 2. The thread blocks on notification queue receives; commands interrupt the receive immediately via `wake_receiver()`. The 5 s receive timeout is only a fallback against a missed wake. 3. On notification: calls `on_image_decode()`, then merges an `ArtworkDisplayUpdate` (the slot's server display timestamp plus the `stream_epoch` it was decoded under) into the `ArtworkRole::Impl::EventState::display_slot` `InboxSlot` via `merge_artwork_display_update`. The main loop's `ArtworkRole::Impl::drain_events()` folds the taken update into its main-thread-only `held_display_*` state and fires `on_image_display()` once the timestamp is reached. Latest-wins per slot: if a newer frame's timestamp overwrites the pending one before the main loop takes it, only the newer display fires; the per-slot epoch lets the deadline sweep drop a display whose stream was replaced after the hand-off. -4. `ArtworkRole::Impl` destructor sets `COMMAND_STOP`, wakes the queue receive, and joins. +4. `ArtworkRole::Impl::stop()` (from `SendspinClient::stop()`, the client destructor, and the `Impl` destructor) sets `COMMAND_STOP`, wakes the queue receive, joins, and then resets the notification queue so a restart does not decode the previous session's images. `start()` clears `COMMAND_STOP` before spawning. **Destruction order** matters because external audio callbacks may still reference the sync task. `PlayerRole::Impl`'s destructor resets the sync task first (`sync_task_.reset()`) before tearing down anything else, so the thread is fully joined before any shared state is destroyed. @@ -202,7 +203,7 @@ The bump arena suits ArduinoJson's allocation pattern: during a parse the varian ### Main Loop Processing -`SendspinClient::loop()` (`src/client.cpp`) runs the following steps **in order** on each tick: +`SendspinClient::loop()` (`src/client.cpp`) is a no-op while the client is stopped (a stopped client has no connections or threads, and the manager loop must not restart the WebSocket server). While started it runs the following steps **in order** on each tick; steps 3 onward are `SendspinClient::drain_inbox()`, which `stop()` also calls once so the clear callbacks are delivered synchronously: ```api 1. connection_manager_->loop() (sections gated on lock-free atomic hints - see below) @@ -451,6 +452,38 @@ When a connection is lost (`on_connection_lost`): `disable_message_dispatch()` is the first step because it's an atomic flag that the network thread checks before invoking any callback. This prevents stale messages from a dead connection from racing into freshly-reset role queues. +### Client Start and Stop + +`SendspinClient::start()` loads persisted state, starts the threaded roles (player sync task, visualizer drain, artwork decode; a failure part-way stops the ones that did start), and calls `ConnectionManager::start()`, which opens admission (`accepting_`) and creates the `SendspinWsServer` on first use. The server itself is started by the manager's `loop()` once the network provider reports ready, so `is_started()` means "running", not "listening". + +`SendspinClient::stop()` is synchronous and ordered so that every producer is gone before any state is reset: + +```api +1. ConnectionManager::stop(SHUTDOWN) + ├─ Under conn_ptr_mutex_: accepting_ = false; disable_message_dispatch() on every managed + │ connection; move the current slot, the nursery, and the deferred releases out; clear the + │ hello retries + ├─ Outside the lock: conn->disconnect(SHUTDOWN, completion) on each, completion counted by a + │ shared GoodbyeWait; wait up to GOODBYE_FLUSH_TIMEOUT_MS (50 ms) for the count to reach zero + ├─ ws_server_->stop() regardless (host: joins every connection thread, each bounded by + │ IXWebSocket's 300 ms close handshake; ESP: httpd_stop(), which runs queued sends first, + │ then every session's close_fn and ctx free_fn, polling at 100 ms) + └─ Move the pending connected/disconnect event queues out under conn_mutex_; every moved-out + shared_ptr is released outside the locks (an outbound connection's destructor stops its + transport synchronously) +2. Role threads: PlayerRole/VisualizerRole/ArtworkRole::Impl::stop() join, then each discards + its ring/queue content (sole consumer after the join) +3. started_ = false (loop() is a no-op and connect_to() is refused from here on) +4. cleanup_connection_state() (the same reset a lost connection triggers), then drain_inbox() + delivers the CLEARED / STREAM_END callbacks it queued; group_state_ and state_ are reset +``` + +The goodbye completion is best-effort: on ESP a session that closes before its queued worker runs, or whose `weak_ptr` no longer resolves, never reports, which is why the wait is bounded rather than exact. The `GoodbyeWait` record is held by `shared_ptr` and captured by value in each completion, so a completion that runs late on a transport thread touches nothing `stop()` owns. A peer delivered by the ws_server while admission is closed is rejected in `on_new_connection()` with a shutdown goodbye, the same shape as the nursery-full rejection. + +`stopping_` guards re-entrancy: a listener callback fired from inside `stop()` that calls `start()` gets `false`, and one that calls `stop()` returns immediately. The reset in step 4 runs with no manager lock held, so it does not reach the pre-existing lock re-entry in which `drop_connection()` holds `conn_ptr_mutex_` through `cleanup_connection_state()` and a listener's `on_release_high_performance()` calls `disconnect()`. + +The client destructor performs steps 1 and 2 only, so a consumer that destroyed its listeners first is never called into; the roles' own destructors then run as before. + ### Graceful Disconnect `disconnect_and_release()` calls `conn->disconnect(reason, nullptr)` and lets the local `shared_ptr` go out of scope. diff --git a/examples/basic_client/main.cpp b/examples/basic_client/main.cpp index 9f6889de..0ae5f2c7 100644 --- a/examples/basic_client/main.cpp +++ b/examples/basic_client/main.cpp @@ -325,7 +325,7 @@ int main(int argc, char* argv[]) { // Start the server fprintf(stderr, "Starting Sendspin basic client on port %u...\n", server_port); - if (!client.start_server()) { + if (!client.start()) { fprintf(stderr, "Failed to start server\n"); return 1; } @@ -373,7 +373,7 @@ int main(int argc, char* argv[]) { #ifdef SENDSPIN_HAS_MDNS mdns.stop(); #endif - client.disconnect(SendspinGoodbyeReason::SHUTDOWN); + client.stop(); #ifndef SENDSPIN_HAS_PORTAUDIO fprintf(stderr, "Total audio bytes received: %zu\n", null_audio_total_bytes); diff --git a/examples/tui_client/main.cpp b/examples/tui_client/main.cpp index 87fd5279..dee11746 100644 --- a/examples/tui_client/main.cpp +++ b/examples/tui_client/main.cpp @@ -763,7 +763,7 @@ int main(int argc, char* argv[]) { #endif // Start the server - if (!client.start_server()) { + if (!client.start()) { fprintf(stderr, "Failed to start server\n"); return 1; } @@ -951,7 +951,7 @@ int main(int argc, char* argv[]) { mdns_browser.stop(); mdns.stop(); #endif - client.disconnect(SendspinGoodbyeReason::SHUTDOWN); + client.stop(); return 0; } diff --git a/include/sendspin/client.h b/include/sendspin/client.h index 5f5fdf27..5e7df32f 100644 --- a/include/sendspin/client.h +++ b/include/sendspin/client.h @@ -75,7 +75,7 @@ class SendspinClientListener { }; /// @brief Platform hook for network readiness -/// Must be set before start_server() +/// Must be set before start() class SendspinNetworkProvider { public: virtual ~SendspinNetworkProvider() = default; @@ -147,8 +147,9 @@ class SendspinTimeBurst; * 2. Construct a SendspinClient with that config * 3. Add roles via add_player(), add_controller(), add_metadata(), etc. * 4. Set listeners on each role and set the network provider on the client - * 5. Call start_server() to start the WebSocket server and background tasks + * 5. Call start() to start the role threads and the WebSocket server * 6. Call loop() periodically from the platform main loop + * 7. Call stop() to goodbye every peer and tear everything down; start() again to restart * * @code * struct MyPlayerListener : PlayerRoleListener { @@ -175,11 +176,12 @@ class SendspinTimeBurst; * player.set_listener(&player_listener); * client.add_controller(); * client.set_network_provider(&network_provider); - * client.start_server(); + * client.start(); * - * while (true) { + * while (running) { * client.loop(); * } + * client.stop(); * @endcode */ class SendspinClient { @@ -201,15 +203,58 @@ class SendspinClient { // Lifecycle // ======================================== - /// @brief Starts the WebSocket server and initializes the sync task (if audio is configured) - /// @return true on success, false on failure - bool start_server(); + /// @brief Starts the role threads and arms the WebSocket server + /// + /// The server itself comes up on the first loop() tick after the network provider reports + /// ready. If a role fails to start, the roles that did start are stopped again so a corrected + /// retry begins from the stopped state. Main-loop thread only. + /// @return true if the client is running (including when it already was), false on failure + bool start(); + + /// @brief Stops the client and returns only once it is fully stopped + /// + /// Sends a client/goodbye (reason shutdown) to every peer, waits up to a short bound for + /// those sends to complete, then closes the server and every connection regardless, joins + /// the role threads, resets every role, and delivers the roles' clear callbacks + /// (on_stream_end(), on_image_clear(), on_metadata_clear(), ...) before returning. No-op + /// when stopped. Calling start() afterwards restarts the client; start, stop, and start + /// again can be repeated indefinitely. + /// + /// Blocking is bounded, but not by the goodbye bound alone. It also includes: the + /// transports' own close (the host server waits up to 300 ms per connection for the close + /// handshake; the ESP server waits for the httpd task to exit, which polls at 100 ms and + /// first finishes any queued send, up to its send timeout for a peer that stops reading); an + /// outbound connect_to() transport's synchronous stop; and any listener callback already + /// running on a role thread, which the join cannot interrupt (on_audio_write() is bounded by + /// its timeout_ms, on_image_decode() is not). + /// + /// Listener callbacks fire from inside this call. One that calls start() has no effect and + /// returns false; one that calls stop() or connect_to() is ignored. Main-loop thread only: + /// calling it from a role-thread callback would join the calling thread. + void stop(); + + /// @brief Returns true between a successful start() and stop() + /// + /// Running means the role threads are up and the server is armed, not that the server is + /// listening yet (that waits for the network provider). + bool is_started() const { + return this->started_; + } + + /// @brief Starts the client + /// @deprecated Use start(). Kept as an alias for existing consumers; removal is planned for + /// v0.8.0. + /// @return See start(). + [[deprecated("Use start()")]] bool start_server() { + return this->start(); + } /// @brief Initiates a client connection to a Sendspin server at the given URL /// - /// Must be called from the main loop thread: it tears down and replaces connection state - /// (time filter, dispatch, client state) directly rather than deferring to loop(), so calling - /// it concurrently with loop() would race those mutations. + /// Ignored (with a warning) while the client is not started. Must be called from the main + /// loop thread: it tears down and replaces connection state (time filter, dispatch, client + /// state) directly rather than deferring to loop(), so calling it concurrently with loop() + /// would race those mutations. /// @param url WebSocket server URL (e.g., "ws://server.local:8927/sendspin") void connect_to(const std::string& url); @@ -222,10 +267,11 @@ class SendspinClient { void disconnect(SendspinGoodbyeReason reason); /// @brief Processes events, drives time sync, checks network. Call from main loop + /// A no-op while the client is stopped. void loop(); // ======================================== - // Role registration (call before start_server) + // Role registration (call before start()) // ======================================== #ifdef SENDSPIN_ENABLE_PLAYER @@ -379,7 +425,7 @@ class SendspinClient { this->listener_ = listener; } - /// @brief Sets the network provider (required before start_server()) + /// @brief Sets the network provider (required before start()) /// The provider must outlive this client void set_network_provider(SendspinNetworkProvider* provider) { this->network_provider_ = provider; @@ -411,6 +457,13 @@ class SendspinClient { /// @brief Cleans up playback state when the active streaming connection is removed void cleanup_connection_state(); + /// @brief Drains the inbox: lifecycle events, role slots, and group updates, dispatching + /// listener callbacks on the calling (main-loop) thread. Shared by loop() and stop(). + void drain_inbox(); + + /// @brief Stops and joins every threaded role; each is a no-op if not running + void stop_role_threads(); + /// @brief Builds the formatted client hello message from config std::string build_hello_message(); @@ -502,7 +555,11 @@ class SendspinClient { // 8-bit fields bool high_performance_held_for_time_{false}; std::atomic high_performance_ref_count_{0}; + /// True between a successful start() and stop(); see is_started(). bool started_{false}; + /// True for the duration of stop(). Refuses a start() and ignores a stop() issued by a + /// listener callback fired from inside the teardown, which would otherwise recurse into it. + bool stopping_{false}; }; } // namespace sendspin diff --git a/include/sendspin/config.h b/include/sendspin/config.h index c3675cdf..c77bf605 100644 --- a/include/sendspin/config.h +++ b/include/sendspin/config.h @@ -32,7 +32,7 @@ namespace sendspin { // ============================================================================ /// @brief Configuration for a SendspinClient instance -/// Filled in by the platform (e.g., ESPHome) before calling start_server() +/// Filled in by the platform (e.g., ESPHome) before calling start() struct SendspinClientConfig { /// Unique client identifier. When left empty, the library falls back to the detected local /// network interface MAC address (the same value used for device_info.mac_address). diff --git a/src/artwork_role.cpp b/src/artwork_role.cpp index a7a0465e..eb12d467 100644 --- a/src/artwork_role.cpp +++ b/src/artwork_role.cpp @@ -101,6 +101,11 @@ bool ArtworkRole::Impl::start() { return false; } + // The flags survive a stop()/start() cycle. The exiting thread's own wait() normally clears + // COMMAND_STOP, but clear it here too so the new thread's first wait() can never see a stale + // stop and exit immediately. + this->drain_task->event_flags.clear(COMMAND_STOP); + platform_configure_thread("SsArt", 4096, static_cast(this->config.priority), this->config.psram_stack); this->drain_task->drain_thread = std::thread(drain_thread_func, this); @@ -117,6 +122,10 @@ void ArtworkRole::Impl::stop() const { this->drain_task->event_flags.set(COMMAND_STOP); this->drain_task->notify_queue.wake_receiver(); this->drain_task->drain_thread.join(); + + // Joined, so this is the queue's only consumer: discard notifications the old thread never + // took, so a restart does not decode the previous session's images. + this->drain_task->notify_queue.reset(); } void ArtworkRole::Impl::build_hello_fields(ClientHelloMessage& msg) const { diff --git a/src/client.cpp b/src/client.cpp index 0f2c27c8..fd6d3dba 100644 --- a/src/client.cpp +++ b/src/client.cpp @@ -79,6 +79,15 @@ SendspinClient::SendspinClient(SendspinClientConfig config) } SendspinClient::~SendspinClient() { + // Transport-only teardown: goodbye and close every peer and join every thread, exactly as + // stop() does, but deliver no listener callback. A consumer that destroys its listeners + // before the client (the natural declaration order when a listener needs a role reference) + // is never called into from here. + if (this->started_) { + this->connection_manager_->stop(SendspinGoodbyeReason::SHUTDOWN); + this->stop_role_threads(); + } + // Stop background threads before tearing down connections. Every role is reset explicitly // (not just the threaded ones): role InboxSlots release their topic-bit claims against // event_state_'s Inbox on destruction, so all roles must be gone before the alphabetized @@ -116,43 +125,102 @@ LogLevel SendspinClient::get_log_level() { // Lifecycle // ============================================================================ -bool SendspinClient::start_server() { - this->started_ = true; +bool SendspinClient::start() { + if (this->started_) { + return true; + } + if (this->stopping_) { + SS_LOGW(TAG, "start() ignored: called from a callback while stop() is in progress"); + return false; + } // Load persisted state this->load_last_played_server(); + // Start the role threads. A failure part-way stops the roles that did start, so the client + // is back in the stopped state and a corrected retry begins clean. + bool roles_started = true; #ifdef SENDSPIN_ENABLE_PLAYER - if (this->player_) { - if (!this->player_->impl_->start()) { - return false; - } + if (roles_started && this->player_) { + roles_started = this->player_->impl_->start(); } #endif +#ifdef SENDSPIN_ENABLE_VISUALIZER + if (roles_started && this->visualizer_) { + roles_started = this->visualizer_->impl_->start(); + } +#endif +#ifdef SENDSPIN_ENABLE_ARTWORK + if (roles_started && this->artwork_) { + roles_started = this->artwork_->impl_->start(); + } +#endif + if (!roles_started) { + this->stop_role_threads(); + return false; + } + // Open admission and create the WebSocket server (started by loop() once the network is + // ready). + this->connection_manager_->start(); + this->started_ = true; + return true; +} + +void SendspinClient::stop() { + if (!this->started_ || this->stopping_) { + return; + } + this->stopping_ = true; + + // 1. Transports first: goodbye every peer, wait up to the flush bound, then close the server + // and every connection. This joins every network thread, so nothing reaches a role or the + // inbox from the network after it returns, and a network thread blocked on ring space + // (write_audio_chunk) resolves while its consumer is still alive. + this->connection_manager_->stop(SendspinGoodbyeReason::SHUTDOWN); + + // 2. Role threads. Each role discards its ring/queue content after its own join. + this->stop_role_threads(); + + // From here the client reads as stopped: loop() is a no-op and connect_to() is refused, so a + // listener callback below cannot restart the server or admit a connection. + this->started_ = false; + + // 3. Reset per-connection and role state exactly as a lost connection does, then deliver + // the clear callbacks it queued now rather than on a loop() tick that is not coming. With + // every producer thread joined, the state the drain leaves behind is the state a restart + // begins from. + this->cleanup_connection_state(); + this->drain_inbox(); + this->group_state_ = GroupUpdateObject{}; + this->state_ = SendspinClientState::SYNCHRONIZED; + + this->stopping_ = false; +} + +void SendspinClient::stop_role_threads() { +#ifdef SENDSPIN_ENABLE_PLAYER + if (this->player_) { + this->player_->impl_->stop(); + } +#endif #ifdef SENDSPIN_ENABLE_VISUALIZER if (this->visualizer_) { - if (!this->visualizer_->impl_->start()) { - return false; - } + this->visualizer_->impl_->stop(); } #endif - #ifdef SENDSPIN_ENABLE_ARTWORK if (this->artwork_) { - if (!this->artwork_->impl_->start()) { - return false; - } + this->artwork_->impl_->stop(); } #endif - - // Create and configure the WebSocket server (started later when network is ready) - this->connection_manager_->init_server(this); - - return true; } void SendspinClient::connect_to(const std::string& url) { + if (!this->started_) { + SS_LOGW(TAG, "connect_to() ignored: client is not started"); + return; + } this->connection_manager_->connect_to(url); } @@ -161,6 +229,12 @@ void SendspinClient::disconnect(SendspinGoodbyeReason reason) { } void SendspinClient::loop() { + // A stopped client is quiescent: no connections, no threads, and the manager loop must not + // restart the WebSocket server the moment the network reads ready. + if (!this->started_) { + return; + } + // Process connection lifecycle events (close, disconnect, hello, handoff, retry) this->connection_manager_->loop(); @@ -183,6 +257,10 @@ void SendspinClient::loop() { } } + this->drain_inbox(); +} + +void SendspinClient::drain_inbox() { // Process deferred events: all state mutations and user callbacks happen here, on the main // loop thread, to avoid cross-thread data races. Two poll() snapshots gate the work below: // inbox_bits (here) gates only the event-ring drain immediately following it; slot_bits @@ -253,8 +331,9 @@ void SendspinClient::loop() { // CLEARED per role is ever pending when this drain runs: cleanup() is called // only from cleanup_connection_state(), which first calls inbox.reset_events() // (wiping the whole ring) before any role re-pushes its CLEARED, and that path - // runs only under conn_ptr_mutex_ (ConnectionManager::drop_connection), so it - // cannot interleave with itself. So even a back-to-back disconnect/reconnect + // runs only on the main loop (under conn_ptr_mutex_ from + // ConnectionManager::drop_connection, or directly from stop()), so it cannot + // interleave with itself. So even a back-to-back disconnect/reconnect // coalesces to a single CLEARED -- the reset_events() ordering is what // guarantees it, not clear-callback idempotency. (Callbacks are idempotent by // contract anyway; see on_controller_state_clear() / on_metadata_clear() / @@ -391,13 +470,13 @@ void SendspinClient::loop() { } // ============================================================================ -// Role registration (call before start_server) +// Role registration (call before start()) // ============================================================================ #ifdef SENDSPIN_ENABLE_PLAYER PlayerRole& SendspinClient::add_player(PlayerRoleConfig config) { if (this->started_) { - SS_LOGW(TAG, "add_player() called after start_server(); role may not initialize correctly"); + SS_LOGW(TAG, "add_player() called while started; role may not initialize correctly"); } this->player_ = std::make_unique(std::move(config), this, this->persistence_provider_); @@ -409,7 +488,7 @@ PlayerRole& SendspinClient::add_player(PlayerRoleConfig config) { #ifdef SENDSPIN_ENABLE_CONTROLLER ControllerRole& SendspinClient::add_controller() { if (this->started_) { - SS_LOGW(TAG, "add_controller() called after start_server()"); + SS_LOGW(TAG, "add_controller() called while started"); } this->controller_ = std::make_unique(this); this->controller_->impl_->attach_inbox(this->event_state_->inbox); @@ -420,7 +499,7 @@ ControllerRole& SendspinClient::add_controller() { #ifdef SENDSPIN_ENABLE_METADATA MetadataRole& SendspinClient::add_metadata() { if (this->started_) { - SS_LOGW(TAG, "add_metadata() called after start_server()"); + SS_LOGW(TAG, "add_metadata() called while started"); } this->metadata_ = std::make_unique(this); this->metadata_->impl_->attach_inbox(this->event_state_->inbox); @@ -431,7 +510,7 @@ MetadataRole& SendspinClient::add_metadata() { #ifdef SENDSPIN_ENABLE_COLOR ColorRole& SendspinClient::add_color() { if (this->started_) { - SS_LOGW(TAG, "add_color() called after start_server()"); + SS_LOGW(TAG, "add_color() called while started"); } this->color_ = std::make_unique(this); this->color_->impl_->attach_inbox(this->event_state_->inbox); @@ -442,7 +521,7 @@ ColorRole& SendspinClient::add_color() { #ifdef SENDSPIN_ENABLE_ARTWORK ArtworkRole& SendspinClient::add_artwork(ArtworkRoleConfig config) { if (this->started_) { - SS_LOGW(TAG, "add_artwork() called after start_server()"); + SS_LOGW(TAG, "add_artwork() called while started"); } this->artwork_ = std::make_unique(std::move(config), this); this->artwork_->impl_->attach_inbox(this->event_state_->inbox); @@ -453,7 +532,7 @@ ArtworkRole& SendspinClient::add_artwork(ArtworkRoleConfig config) { #ifdef SENDSPIN_ENABLE_VISUALIZER VisualizerRole& SendspinClient::add_visualizer(VisualizerRoleConfig config) { if (this->started_) { - SS_LOGW(TAG, "add_visualizer() called after start_server()"); + SS_LOGW(TAG, "add_visualizer() called while started"); } this->visualizer_ = std::make_unique(std::move(config), this); this->visualizer_->impl_->attach_inbox(this->event_state_->inbox); diff --git a/src/connection_manager.cpp b/src/connection_manager.cpp index b3473e6b..36dbeb14 100644 --- a/src/connection_manager.cpp +++ b/src/connection_manager.cpp @@ -222,8 +222,18 @@ void ConnectionManager::disconnect(SendspinGoodbyeReason reason) { // Server lifecycle // ============================================================================ -void ConnectionManager::init_server(SendspinClient* client) { - this->client_ = client; +void ConnectionManager::start() { + { + std::lock_guard lock(this->conn_ptr_mutex_); + this->accepting_.store(true, std::memory_order_release); + } + // A restart reuses the server object: stop() only stopped it, and loop() starts it again + // once the network is ready. Retry immediately rather than honoring a backoff from before + // the stop. + this->ws_server_start_retry_time_us_ = 0; + if (this->ws_server_ != nullptr) { + return; + } this->ws_server_ = std::make_unique(); this->ws_server_->set_port(this->client_->config_.server_port); @@ -279,6 +289,71 @@ void ConnectionManager::init_server(SendspinClient* client) { }); } +void ConnectionManager::stop(SendspinGoodbyeReason reason) { + // Close admission and detach every managed connection under the lock. Nothing is sent or + // released here (see DeferredRelease): the goodbyes below run outside the lock, and a + // rejection for a peer delivered during the wait can take the lock meanwhile. + std::vector> to_goodbye; + std::vector> to_drop; + { + std::lock_guard lock(this->conn_ptr_mutex_); + this->accepting_.store(false, std::memory_order_release); + if (this->current_connection_ != nullptr) { + this->current_connection_->disable_message_dispatch(); + to_goodbye.push_back(std::move(this->current_connection_)); + this->set_current_connection(nullptr); + } + for (auto& entry : this->nursery_) { + entry.conn->disable_message_dispatch(); + to_goodbye.push_back(std::move(entry.conn)); + } + this->nursery_.clear(); + this->nursery_size_.store(0, std::memory_order_release); + this->hello_retries_.clear(); + // Releases already queued (a handoff loser, a reaped entry) had their dispatch disabled + // when they were queued; the shutdown goodbye replaces whatever reason they carried. + for (auto& release : this->deferred_releases_) { + (release.goodbye.has_value() ? to_goodbye : to_drop).push_back(std::move(release.conn)); + } + this->deferred_releases_.clear(); + this->deferred_size_.store(0, std::memory_order_release); + } + + // Goodbye every connection and wait, bounded, for the sends to complete. Every count is + // registered before the wait starts, so a completion that runs inline (host, and any + // not-connected transport) cannot satisfy the wait early. A disconnected connection completes + // immediately (see SendspinConnection::disconnect), so none needs a pre-check. + auto wait = std::make_shared(); + for (auto& conn : to_goodbye) { + wait->add_pending(); + conn->disconnect(reason, [wait] { wait->complete_one(); }); + } + if (!wait->wait(GOODBYE_FLUSH_TIMEOUT_MS)) { + SS_LOGD(TAG, "Goodbye flush bound (%u ms) elapsed; closing regardless", + static_cast(GOODBYE_FLUSH_TIMEOUT_MS)); + } + + // Tear the server down regardless. This joins every network thread on host and waits for + // the httpd task on ESP, so no callback of any kind arrives after it returns. Close + // callbacks fired during it queue disconnect events under conn_mutex_, which is not held. + if (this->ws_server_ != nullptr) { + this->ws_server_->stop(); + } + + // Drop the lifecycle events those closes queued: the connections they name are gone. Moved + // out under the lock and destroyed after it, since a destructor can join a transport thread. + std::vector> pending_connected; + std::vector> pending_disconnects; + { + std::lock_guard lock(this->conn_mutex_); + pending_connected = std::move(this->pending_connected_events_); + pending_disconnects = std::move(this->pending_disconnect_events_); + this->has_pending_events_.store(false, std::memory_order_release); + } + // Locals release here, outside every lock. An outbound connection's destructor stops its + // transport synchronously; deferring that is not an option (see DeferredRelease). +} + void ConnectionManager::loop() { // Start WS server when network becomes ready. A persistent failure (e.g. the server port is // already in use) is retried with backoff instead of on every tick, which would spam the log. @@ -585,7 +660,14 @@ void ConnectionManager::on_new_connection(std::shared_ptr= NURSERY_CAPACITY) { + if (!this->accepting_.load(std::memory_order_acquire)) { + // Delivered while stop() is tearing down (or before start()): the nursery is being + // emptied, so the newcomer gets a goodbye and a close instead of a slot. Same shape + // as the nursery-full rejection below. + SS_LOGD(TAG, "Not accepting connections, rejecting new connection"); + conn->disable_message_dispatch(); + this->queue_deferred_release(std::move(conn), SendspinGoodbyeReason::SHUTDOWN); + } else if (inbound_count >= NURSERY_CAPACITY) { SS_LOGW(TAG, "Nursery full of live connections, rejecting new connection"); // Never managed, but its callbacks are already wired: block dispatch so it cannot // inject messages during the goodbye window. diff --git a/src/connection_manager.h b/src/connection_manager.h index 2fc1de09..aa5cacee 100644 --- a/src/connection_manager.h +++ b/src/connection_manager.h @@ -23,6 +23,7 @@ #include "sendspin/client.h" #include +#include #include #include #include @@ -65,6 +66,53 @@ static constexpr int64_t LIVENESS_TOLERATED_MISSES = 2; /// @return Timeout in milliseconds; 0 or negative disables the check. int64_t resolve_liveness_timeout_ms(const SendspinClientConfig& config); +/// @brief Bound (milliseconds) on waiting for stop()'s goodbyes to be sent before the transports +/// are torn down +/// +/// The host transports send synchronously, so on host the wait resolves before it starts. On the +/// ESP server path the goodbye is queued to the httpd worker, and this is a few scheduler quanta +/// for the worker to dequeue the frame and hand it to lwIP. Send completion is best-effort (see +/// SendspinConnection::send_text_message): a session that closes first never reports, so this is +/// a cap on how long stop() blocks for its peers' sake, never a guarantee the goodbye arrived. +static constexpr uint32_t GOODBYE_FLUSH_TIMEOUT_MS = 50; + +/// @brief Counts the goodbye sends stop() is waiting on +/// +/// Shared by stop() and each connection's completion callback through a shared_ptr captured by +/// value, so a completion that runs on a transport thread after stop() has given up (an ESP httpd +/// worker draining late) touches only this record, never stop()'s stack or the manager. +struct GoodbyeWait { + /// @brief Registers one goodbye whose completion is awaited + void add_pending() { + std::lock_guard lock(this->mutex); + ++this->pending; + } + + /// @brief Records one completion; wakes wait() when none remain + void complete_one() { + { + std::lock_guard lock(this->mutex); + if (this->pending > 0) { + --this->pending; + } + } + this->cv.notify_all(); + } + + /// @brief Blocks until every registered goodbye has completed or the bound elapses + /// @param timeout_ms Maximum time to wait. + /// @return true if every goodbye completed, false if the bound elapsed first. + bool wait(uint32_t timeout_ms) { + std::unique_lock lock(this->mutex); + return this->cv.wait_for(lock, std::chrono::milliseconds(timeout_ms), + [this] { return this->pending == 0; }); + } + + std::mutex mutex; + std::condition_variable cv; + size_t pending{0}; +}; + /// @brief A connection that has not completed the hello handshake /// /// Unproven connections never occupy the current-connection slot; they wait in the bounded nursery @@ -113,21 +161,22 @@ struct HelloRetryState { * * Typical usage: * 1. Construct with a `SendspinClient*`. - * 2. Call `init_server()` once to create and configure the WebSocket server. + * 2. Call `start()` to open admission and create the WebSocket server. * 3. Call `loop()` periodically to drive connection state, process deferred events, and retry * hellos. * 4. Call `connect_to()` to initiate an outgoing client connection when needed. - * 5. Call `disconnect()` to gracefully close the active connection. + * 5. Call `disconnect()` to gracefully close the active connection, or `stop()` to tear + * every connection and the server down synchronously. * * @code * ConnectionManager manager(client); - * manager.init_server(client, use_psram, priority); + * manager.start(); * * while (running) { * manager.loop(); * } * - * manager.disconnect(SendspinGoodbyeReason::SHUTDOWN); + * manager.stop(SendspinGoodbyeReason::SHUTDOWN); * @endcode */ class ConnectionManager { @@ -156,10 +205,24 @@ class ConnectionManager { // Server lifecycle // ======================================== - /// @brief Creates the WebSocket server and configures callbacks. Call once from start_server(). - /// Server configuration is read from client->config_. - /// @param client The SendspinClient that owns this manager. - void init_server(SendspinClient* client); + /// @brief Opens admission and creates the WebSocket server on first use + /// + /// Server configuration is read from the client's config. loop() starts the server once the + /// network provider reports ready. Main-loop thread only. + void start(); + + /// @brief Synchronous teardown: goodbyes every managed connection, waits up to + /// GOODBYE_FLUSH_TIMEOUT_MS for the sends to complete, then stops the WebSocket server and + /// releases every connection regardless + /// + /// Closes admission first, so a peer delivered during the wait is rejected with a goodbye. + /// Blocks on the transports' own teardown as well as the flush bound: the host server joins + /// its connection threads (each waits up to IXWebSocket's 300 ms close handshake), the ESP + /// server waits for the httpd task to exit, and an outbound connection's transport stop is + /// synchronous (esp_websocket_client_stop() / ix::WebSocket::stop()). Client-state cleanup is + /// the caller's job: this only detaches connections. Main-loop thread only. + /// @param reason The goodbye reason sent to every connected peer. + void stop(SendspinGoodbyeReason reason); /// @brief Drives connection state: starts server when network ready, processes lifecycle /// events, retries hello, calls loop() on active connections. @@ -345,7 +408,7 @@ class ConnectionManager { /// Socket-budget invariant: gracefully rejecting a surplus inbound peer requires the transport /// to accept NURSERY_CAPACITY + 2 sockets (1 established + the nursery + the surplus peer, /// which must be connected to receive its goodbye). The default server_max_connections - /// satisfies this; init_server warns when a configured value does not. + /// satisfies this; start() warns when a configured value does not. static constexpr size_t NURSERY_CAPACITY = 2; // Struct fields @@ -401,6 +464,11 @@ class ConnectionManager { /// queue_deferred_release()) and after the drain swap in flush_deferred_releases(). Lets /// flush_deferred_releases() early-return without locking when nothing is queued. std::atomic deferred_size_{0}; + + /// True between start() and stop(). Written under conn_ptr_mutex_ and read under it by + /// on_new_connection() (network thread), so a peer delivered after stop() closed admission is + /// rejected rather than admitted into a nursery stop() has already emptied. + std::atomic accepting_{false}; }; } // namespace sendspin diff --git a/src/player_role.cpp b/src/player_role.cpp index 8254a2e8..8c03b718 100644 --- a/src/player_role.cpp +++ b/src/player_role.cpp @@ -175,20 +175,27 @@ void PlayerRole::Impl::attach_inbox(Inbox& inbox) { bool PlayerRole::Impl::start() { this->load_static_delay(); - if (!this->config.audio_formats.empty() && this->listener && - !this->sync_task->is_initialized()) { - if (!this->sync_task->init(this, this->client, this->config.audio_buffer_capacity)) { - SS_LOGE(TAG, "Failed to initialize sync task"); - return false; - } - if (!this->sync_task->start(this->config.psram_stack, this->config.priority)) { - SS_LOGE(TAG, "Failed to start sync task thread"); - return false; - } + if (this->config.audio_formats.empty() || !this->listener) { + return true; + } + // Init once (event flags, ring buffer); the thread is created on every start(), including a + // restart after stop(), which joined the previous one. + if (!this->sync_task->is_initialized() && + !this->sync_task->init(this, this->client, this->config.audio_buffer_capacity)) { + SS_LOGE(TAG, "Failed to initialize sync task"); + return false; + } + if (!this->sync_task->start(this->config.psram_stack, this->config.priority)) { + SS_LOGE(TAG, "Failed to start sync task thread"); + return false; } return true; } +void PlayerRole::Impl::stop() const { + this->sync_task->stop(); +} + void PlayerRole::Impl::build_hello_fields(ClientHelloMessage& msg) { if (this->config.audio_formats.empty()) { return; diff --git a/src/player_role_impl.h b/src/player_role_impl.h index 494d7053..d4df6cc6 100644 --- a/src/player_role_impl.h +++ b/src/player_role_impl.h @@ -86,6 +86,8 @@ struct PlayerRole::Impl { } void drain_events(); void cleanup(); + /// @brief Joins the sync task thread and discards its buffered audio; no-op if not started. + void stop() const; // ======================================== // Consumer-facing method implementations diff --git a/src/sync_task.cpp b/src/sync_task.cpp index 8aa088bf..7ab9f5b8 100644 --- a/src/sync_task.cpp +++ b/src/sync_task.cpp @@ -802,6 +802,15 @@ void SyncTask::stop() { this->event_flags_.set(EventGroupBits::COMMAND_STOP); this->encoded_ring_buffer_->wake_receiver(); this->sync_thread_.join(); + + // A stop mid-stream leaves TASK_RUNNING set (only the idle transition clears it). The player's + // sync-idle gate reads is_running() to decide when a STREAM_END may fire, so a stopped task + // must read as idle or the stop-time on_stream_end() would wait for a thread that is gone. + this->event_flags_.clear(EventGroupBits::TASK_RUNNING); + + // The thread is joined, so this is the ring's only consumer (the single-consumer contract + // reset() requires). Discard buffered audio so a restart does not replay the old stream. + this->encoded_ring_buffer_->reset(); } // ============================================================================ @@ -941,6 +950,13 @@ void SyncTask::thread_entry(void* params) { // a codec header that arrived during a rapid seek (STREAM_END → STREAM_START). } + // The idle-state exits above break out while still holding the codec header they received; + // hand it back so stop()'s ring reset sees no borrowed entry. + if (sync_context.encoded_entry != nullptr) { + this_task->encoded_ring_buffer_->return_chunk(sync_context.encoded_entry); + sync_context.encoded_entry = nullptr; + } + this_task->event_flags_.set(EventGroupBits::TASK_STOPPED); } diff --git a/src/sync_task.h b/src/sync_task.h index df3de379..9a7af335 100644 --- a/src/sync_task.h +++ b/src/sync_task.h @@ -130,6 +130,11 @@ class SyncTask { /// @return true if thread started successfully, false otherwise. bool start(bool task_stack_in_psram, unsigned priority); + /// @brief Signals the task to stop, joins the thread, and discards buffered audio + /// A later start() creates a fresh thread on the same (still initialized) queues. No-op when + /// the thread is not running. Main-loop thread only: joins the sync thread. + void stop(); + /// @brief Returns true if init() has been called successfully /// @return true if the sync task has been initialized, false otherwise. bool is_initialized() const { @@ -264,9 +269,6 @@ class SyncTask { /// playtime. void process_playback_progress(SyncContext& sync_context); - /// @brief Signals the task to stop and waits for the thread to finish - void stop(); - // Struct fields EventFlags event_flags_; // Latest-wins slot that merges (sum frames, keep latest finish_timestamp) diff --git a/src/visualizer_role.cpp b/src/visualizer_role.cpp index 399e5dbc..d552d194 100644 --- a/src/visualizer_role.cpp +++ b/src/visualizer_role.cpp @@ -176,6 +176,12 @@ bool VisualizerRole::Impl::start() { return false; } + // The flags survive a stop()/start() cycle. The exiting thread's own wait() normally clears + // COMMAND_STOP, but a flush or clear signalled after the join (cleanup() on a stopped role) + // is still set; clear all three so the new thread starts from a clean command state (stop() + // already emptied the ring). + this->drain_task->event_flags.clear(COMMAND_STOP | COMMAND_FLUSH | COMMAND_CLEAR); + platform_configure_thread("SsVis", 4096, static_cast(this->config.priority), this->config.psram_stack); this->drain_task->drain_thread = std::thread(drain_thread_func, this); @@ -192,6 +198,11 @@ void VisualizerRole::Impl::stop() const { this->drain_task->event_flags.set(COMMAND_STOP); this->drain_task->ring_buffer.wake_receiver(); this->drain_task->drain_thread.join(); + + // Joined, so this is the ring's only consumer (the single-consumer contract the ring + // requires): discard entries the old thread never took, so a restart does not deliver the + // previous session's frames against the new session's format. + this->flush_ring_buffer(); } void VisualizerRole::Impl::build_hello_fields(ClientHelloMessage& msg) { diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index a429a078..d4189a7e 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -33,6 +33,7 @@ add_executable(sendspin_tests test_visualizer_role.cpp test_artwork_role.cpp test_client_teardown.cpp + test_client_lifecycle.cpp ) # Reach the library's private headers (protocol_messages.h, time_filter.h, ...). diff --git a/tests/test_client_lifecycle.cpp b/tests/test_client_lifecycle.cpp new file mode 100644 index 00000000..4bfc1d4a --- /dev/null +++ b/tests/test_client_lifecycle.cpp @@ -0,0 +1,489 @@ +// Copyright 2026 Sendspin Contributors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +/// @file test_client_lifecycle.cpp +/// @brief start() / stop() / restart of a SendspinClient: peers are goodbyed, role state is +/// reset and its clear callbacks delivered before stop() returns, a restarted client is live +/// again, and a callback fired from inside stop() cannot recurse into the lifecycle. +/// +/// The client is driven on loopback ports like test_connection_lifecycle.cpp: an IXWebSocket +/// endpoint plays the Sendspin server and the test thread pumps client.loop(). + +#include "connection_manager.h" // GoodbyeWait, GOODBYE_FLUSH_TIMEOUT_MS +#include "platform/time.h" +#include "sendspin/client.h" +#include "sendspin/config.h" +#include "sendspin/metadata_role.h" +#include "sendspin/player_role.h" + +#include +#include + +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +using namespace sendspin; // NOLINT(google-build-using-namespace): test-local convenience + +namespace { + +// Distinct ports per test so a lingering socket from one scenario cannot bleed into the next +// (and into test_connection_lifecycle.cpp, which uses 18941-18982). +constexpr uint16_t RESTART_TEST_PORT = 18991; +constexpr uint16_t NURSERY_GOODBYE_TEST_PORT = 18992; +constexpr uint16_t STREAM_TEST_PORT = 18993; +constexpr uint16_t CALLBACK_TEST_PORT = 18994; +constexpr uint16_t DESTRUCTOR_TEST_PORT = 18995; + +std::string server_url(uint16_t port) { + return "ws://127.0.0.1:" + std::to_string(port) + "/sendspin"; +} + +SendspinClientConfig make_config(uint16_t port) { + SendspinClientConfig config; + config.client_id = "lifecycle-test-client"; + config.name = "Lifecycle Test Client"; + config.server_port = port; + return config; +} + +class TestNetworkProvider : public SendspinNetworkProvider { +public: + bool is_network_ready() override { + return true; + } +}; + +// Pumps client.loop() until pred() is true. No timeout: a regression hangs here and the suite +// watchdog reports it. +void pump_until(SendspinClient& client, const std::function& pred) { + for (;;) { + client.loop(); + if (pred()) { + return; + } + std::this_thread::sleep_for(std::chrono::milliseconds(2)); + } +} + +// Pumps client.loop() for a fixed window. Only for "must not happen" checks: a window that is +// too short can miss a regression, never fail a correct run. +void pump_for(SendspinClient& client, int duration_ms) { + const auto deadline = std::chrono::steady_clock::now() + std::chrono::milliseconds(duration_ms); + while (std::chrono::steady_clock::now() < deadline) { + client.loop(); + std::this_thread::sleep_for(std::chrono::milliseconds(2)); + } +} + +/// Reports whether anything is listening on the loopback port. +bool port_accepts(uint16_t port) { + int fd = ::socket(AF_INET, SOCK_STREAM, 0); + if (fd < 0) { + return false; + } + sockaddr_in addr{}; + addr.sin_family = AF_INET; + addr.sin_addr.s_addr = htonl(INADDR_LOOPBACK); + addr.sin_port = htons(port); + const bool connected = ::connect(fd, reinterpret_cast(&addr), sizeof(addr)) == 0; + ::close(fd); + return connected; +} + +/// Behavior knobs for FakeServer. +struct FakeServerOptions { + bool answer_hello{true}; ///< Reply to client/hello with server/hello (false: a peer that + ///< upgrades and then never establishes, so it stays in the nursery) +}; + +/// A minimal Sendspin server: an IXWebSocket client that connects to the SendspinClient's WS +/// server, answers client/hello with server/hello, answers client/time with a server/time whose +/// clock is the client's own (both sides read platform_time_us(), so the offset is ~0 and audio +/// timestamps mean what they say), and records the goodbye and close. +class FakeServer { +public: + FakeServer(const std::string& url, std::string server_id, FakeServerOptions options = {}) + : server_id_(std::move(server_id)) { + this->ws_.setUrl(url); + this->ws_.disableAutomaticReconnection(); + this->ws_.setOnMessageCallback([this, options](const ix::WebSocketMessagePtr& msg) { + if (msg->type == ix::WebSocketMessageType::Message) { + const std::string& text = msg->str; + if (text.find("client/hello") != std::string::npos) { + this->got_client_hello_.store(true); + if (options.answer_hello) { + this->ws_.send( + std::string(R"({"type":"server/hello","payload":{"server_id":")") + + this->server_id_ + + R"(","name":"Fake Server","version":1,"active_roles":["player"],)" + + R"("connection_reason":"discovery"}})"); + } + } else if (text.find("client/time") != std::string::npos) { + const auto pos = text.find("\"client_transmitted\":"); + if (pos != std::string::npos) { + const long long client_transmitted = + std::strtoll(text.c_str() + pos + 21, nullptr, 10); + const int64_t now = platform_time_us(); + this->ws_.send(std::string(R"({"type":"server/time","payload":{)") + + "\"client_transmitted\":" + + std::to_string(client_transmitted) + + ",\"server_received\":" + std::to_string(now) + + ",\"server_transmitted\":" + std::to_string(now) + "}}"); + } + } else if (text.find("client/goodbye") != std::string::npos) { + this->got_goodbye_.store(true); + } + } else if (msg->type == ix::WebSocketMessageType::Close || + msg->type == ix::WebSocketMessageType::Error) { + this->closed_.store(true); + } + }); + this->ws_.start(); + } + + ~FakeServer() { + this->ws_.stop(); + } + + void send_text(const std::string& text) { + this->ws_.send(text); + } + + /// Sends one player audio chunk: binary type 4, big-endian server timestamp, PCM payload. + void send_audio(int64_t timestamp_us, size_t payload_bytes) { + std::string frame; + frame.push_back(static_cast(4)); + for (int shift = 56; shift >= 0; shift -= 8) { + frame.push_back(static_cast((timestamp_us >> shift) & 0xFF)); + } + frame.append(payload_bytes, '\0'); + this->ws_.sendBinary(frame); + } + + bool closed() const { + return this->closed_.load(); + } + + bool got_client_hello() const { + return this->got_client_hello_.load(); + } + + bool got_goodbye() const { + return this->got_goodbye_.load(); + } + +private: + ix::WebSocket ws_; + std::string server_id_; + std::atomic closed_{false}; + std::atomic got_client_hello_{false}; + std::atomic got_goodbye_{false}; +}; + +/// Blocks until pred() is true without pumping the client (for checks on a stopped client). +void wait_until(const std::function& pred) { + while (!pred()) { + std::this_thread::sleep_for(std::chrono::milliseconds(2)); + } +} + +/// Counts the player lifecycle callbacks and audio writes; the write itself is a sink. +class CountingPlayerListener : public PlayerRoleListener { +public: + size_t on_audio_write(uint8_t* /*data*/, size_t length, uint32_t /*timeout_ms*/) override { + this->audio_writes.fetch_add(1); + return length; + } + void on_stream_start() override { + ++this->stream_starts; + } + void on_stream_end() override { + ++this->stream_ends; + } + + std::atomic audio_writes{0}; + int stream_starts{0}; + int stream_ends{0}; +}; + +/// Records on_metadata_clear() and, from inside it, tries to drive the lifecycle re-entrantly. +class ReentrantMetadataListener : public MetadataRoleListener { +public: + explicit ReentrantMetadataListener(SendspinClient& client) : client_(client) {} + + void on_metadata_clear() override { + ++this->clears; + this->started_during_clear = this->client_.is_started(); + this->start_result_during_clear = this->client_.start(); + this->client_.stop(); // Must be ignored, not recurse + } + + int clears{0}; + bool started_during_clear{true}; + bool start_result_during_clear{true}; + +private: + SendspinClient& client_; +}; + +/// A metadata listener that must never be called; every callback aborts the test. +class ForbiddenMetadataListener : public MetadataRoleListener { +public: + void on_metadata(const ServerMetadataStateObject& /*metadata*/) override { + ADD_FAILURE() << "on_metadata() fired on a listener the consumer already released"; + } + void on_metadata_clear() override { + ADD_FAILURE() << "on_metadata_clear() fired on a listener the consumer already released"; + } +}; + +std::string stream_start_pcm_json() { + return R"({"type":"stream/start","payload":{"player":{"codec":"pcm","sample_rate":48000,)" + R"("channels":2,"bit_depth":16}}})"; +} + +PlayerRoleConfig make_player_config() { + PlayerRoleConfig player_cfg; + player_cfg.audio_formats.push_back({SendspinCodecFormat::PCM, 2, 48000, 16}); + player_cfg.audio_buffer_capacity = 64 * 1024; + return player_cfg; +} + +// Pumps until the peer has written at least `target` audio callbacks, feeding 20 ms PCM chunks +// stamped a little ahead of now so the sync task has something to schedule. +void stream_audio_until(SendspinClient& client, FakeServer& server, CountingPlayerListener& listener, + size_t target) { + constexpr size_t PCM_20MS_BYTES = 48000 / 50 * 2 * 2; + int64_t next_ts = platform_time_us() + 50 * 1000; + pump_until(client, [&] { + if (listener.audio_writes.load() >= target) { + return true; + } + server.send_audio(next_ts, PCM_20MS_BYTES); + next_ts += 20 * 1000; + std::this_thread::sleep_for(std::chrono::milliseconds(20)); // real-time pacing + return false; + }); +} + +// ============================================================================ +// GoodbyeWait: the bound stop() relies on +// ============================================================================ + +// A goodbye whose completion never arrives (an ESP session that closes before its worker runs +// reports nothing) must not hold stop() open: wait() returns false once the bound elapses. +// Deleting the bound turns this into a hang the suite watchdog reports. +TEST(GoodbyeWait, BoundElapsesWhenACompletionNeverArrives) { + GoodbyeWait wait; + wait.add_pending(); + EXPECT_FALSE(wait.wait(GOODBYE_FLUSH_TIMEOUT_MS)); +} + +// Control: with every registered goodbye completed (from another thread, as a transport worker +// would) wait() reports success, and with nothing registered it never blocks. +TEST(GoodbyeWait, CompletionsSatisfyTheWait) { + GoodbyeWait idle; + EXPECT_TRUE(idle.wait(GOODBYE_FLUSH_TIMEOUT_MS)); + + GoodbyeWait wait; + wait.add_pending(); + wait.add_pending(); + std::thread worker([&] { + wait.complete_one(); + wait.complete_one(); + }); + // No bound: a lost completion hangs here and the watchdog reports it, rather than the + // elapsed time deciding the verdict. + EXPECT_TRUE(wait.wait(UINT32_MAX)); + worker.join(); +} + +// ============================================================================ +// SendspinClient lifecycle +// ============================================================================ + +// start -> stop -> start, twice over: every stop goodbyes and closes the established peer, resets +// the group state, and leaves nothing listening; every restart accepts a new peer and completes +// its handshake. Also pins start() as idempotent while running and stop() as a no-op when +// stopped. +TEST(ClientLifecycle, RestartYieldsALiveClient) { + TestNetworkProvider network; + SendspinClient client(make_config(RESTART_TEST_PORT)); + client.set_network_provider(&network); + + EXPECT_FALSE(client.is_started()); + client.stop(); // No-op when stopped + EXPECT_FALSE(client.is_started()); + + for (int cycle = 0; cycle < 3; ++cycle) { + ASSERT_TRUE(client.start()); + EXPECT_TRUE(client.start()); // Already running: reports true, starts nothing twice + EXPECT_TRUE(client.is_started()); + + const std::string server_id = "server-" + std::to_string(cycle); + FakeServer server(server_url(RESTART_TEST_PORT), server_id); + pump_until(client, [&] { return client.is_connected(); }); + auto info = client.get_server_information(); + ASSERT_TRUE(info.has_value()); + EXPECT_EQ(info->server_id, server_id); + + // Some group state for stop() to reset. + server.send_text(R"({"type":"group/update","payload":{"playback_state":"playing"}})"); + pump_until(client, [&] { + return client.get_group_state().playback_state.has_value(); + }); + + client.stop(); + + EXPECT_FALSE(client.is_started()); + EXPECT_FALSE(client.is_connected()); + EXPECT_FALSE(client.get_server_information().has_value()); + EXPECT_FALSE(client.get_group_state().playback_state.has_value()); + // The peer received its goodbye and the close, in that order. + wait_until([&] { return server.closed(); }); + EXPECT_TRUE(server.got_goodbye()); + + // Stopped means quiescent: pumping loop() must not bring the server back up. + pump_for(client, 100); + EXPECT_FALSE(port_accepts(RESTART_TEST_PORT)); + } +} + +// A peer still in the nursery (it upgraded but never answered the hello) gets the same goodbye and +// close as the established one, so no peer is left to discover the shutdown by timeout. +TEST(ClientLifecycle, StopGoodbyesNurseryPeersToo) { + TestNetworkProvider network; + SendspinClient client(make_config(NURSERY_GOODBYE_TEST_PORT)); + client.set_network_provider(&network); + ASSERT_TRUE(client.start()); + + FakeServer established(server_url(NURSERY_GOODBYE_TEST_PORT), "server-established"); + pump_until(client, [&] { return client.is_connected(); }); + + FakeServer mute(server_url(NURSERY_GOODBYE_TEST_PORT), "server-mute", + FakeServerOptions{.answer_hello = false}); + pump_until(client, [&] { return mute.got_client_hello(); }); + + client.stop(); + + wait_until([&] { return established.closed() && mute.closed(); }); + EXPECT_TRUE(established.got_goodbye()); + EXPECT_TRUE(mute.got_goodbye()); +} + +// With a stream playing, stop() ends it (on_stream_end() fires before stop() returns, paired with +// the earlier on_stream_start()) and a restarted client plays a new stream: audio reaches the +// listener again, which needs the sync task thread to have been re-created, not just the server. +TEST(ClientLifecycle, StopEndsTheStreamAndRestartPlaysAgain) { + TestNetworkProvider network; + CountingPlayerListener listener; + auto config = make_config(STREAM_TEST_PORT); + config.time_burst_interval_ms = 100; // Sync promptly after each (re)connect + SendspinClient client(std::move(config)); + client.set_network_provider(&network); + client.add_player(make_player_config()).set_listener(&listener); + + for (int cycle = 0; cycle < 2; ++cycle) { + ASSERT_TRUE(client.start()); + FakeServer server(server_url(STREAM_TEST_PORT), "server-" + std::to_string(cycle)); + pump_until(client, [&] { return client.is_connected(); }); + + server.send_text(stream_start_pcm_json()); + pump_until(client, [&] { return listener.stream_starts == cycle + 1; }); + EXPECT_EQ(listener.stream_ends, cycle); + + // Audio flowing proves the sync task thread is alive in this cycle. + const size_t writes_before = listener.audio_writes.load(); + stream_audio_until(client, server, listener, writes_before + 1); + + client.stop(); + + // The clear callback was delivered inside stop(), not left for a loop() tick. + EXPECT_EQ(listener.stream_ends, cycle + 1); + EXPECT_EQ(listener.stream_starts, cycle + 1); + wait_until([&] { return server.closed(); }); + EXPECT_TRUE(server.got_goodbye()); + } +} + +// A listener callback fired from inside stop() cannot re-enter the lifecycle: start() reports +// failure and starts nothing, stop() is ignored rather than recursing, and the client reads as +// stopped. Afterwards the client restarts normally. +TEST(ClientLifecycle, CallbackDuringStopCannotRecurse) { + TestNetworkProvider network; + SendspinClient client(make_config(CALLBACK_TEST_PORT)); + client.set_network_provider(&network); + ReentrantMetadataListener listener(client); + client.add_metadata().set_listener(&listener); + ASSERT_TRUE(client.start()); + + { + FakeServer server(server_url(CALLBACK_TEST_PORT), "server-a"); + pump_until(client, [&] { return client.is_connected(); }); + + client.stop(); + + EXPECT_EQ(listener.clears, 1); + EXPECT_FALSE(listener.started_during_clear); + EXPECT_FALSE(listener.start_result_during_clear); + EXPECT_FALSE(client.is_started()); + wait_until([&] { return server.closed(); }); + } + + // The refused start() inside the callback left the client stopped; a real start() works. + ASSERT_TRUE(client.start()); + FakeServer server(server_url(CALLBACK_TEST_PORT), "server-b"); + pump_until(client, [&] { return client.is_connected(); }); + client.stop(); + EXPECT_EQ(listener.clears, 2); +} + +// Destroying a running client goodbyes its peer like stop() does, but delivers no listener +// callback: the listener here is released before the client, the natural order for a consumer +// that never called stop(), and the sanitizer turns any callback into a use-after-free. +TEST(ClientLifecycle, DestructorGoodbyesPeersWithoutCallbacks) { + TestNetworkProvider network; + FakeServer* server = nullptr; + auto listener = std::make_unique(); + { + SendspinClient client(make_config(DESTRUCTOR_TEST_PORT)); + client.set_network_provider(&network); + client.add_metadata().set_listener(listener.get()); + ASSERT_TRUE(client.start()); + + server = new FakeServer(server_url(DESTRUCTOR_TEST_PORT), "server-a"); + pump_until(client, [&] { return client.is_connected(); }); + + listener.reset(); + // Client destroyed here while established, with its listener already gone. + } + + wait_until([&] { return server->closed(); }); + EXPECT_TRUE(server->got_goodbye()); + delete server; +} + +} // namespace diff --git a/tests/test_client_teardown.cpp b/tests/test_client_teardown.cpp index 0dbd4c29..3e5bdb72 100644 --- a/tests/test_client_teardown.cpp +++ b/tests/test_client_teardown.cpp @@ -81,7 +81,7 @@ TEST(ClientTeardown, JoinsEveryThreadedRoleOnDestruction) { vis_cfg.support.rate_max = 30; client->add_visualizer(std::move(vis_cfg)); - ASSERT_TRUE(client->start_server()); + ASSERT_TRUE(client->start()); if (run > 0) { // Run 0 tears down mid-startup; later runs tear down threads parked in receives. diff --git a/tests/test_connection_lifecycle.cpp b/tests/test_connection_lifecycle.cpp index f44abd36..35c95215 100644 --- a/tests/test_connection_lifecycle.cpp +++ b/tests/test_connection_lifecycle.cpp @@ -380,7 +380,7 @@ TEST(ConnectionLifecycle, JunkProbeDoesNotBlockRealServer) { TestNetworkProvider network; SendspinClient client(make_config(PROBE_TEST_PORT)); client.set_network_provider(&network); - ASSERT_TRUE(client.start_server()); + ASSERT_TRUE(client.start()); client.loop(); // First tick binds the WS server // Hold a raw TCP connection open without ever speaking WebSocket. @@ -440,7 +440,7 @@ TEST(ConnectionLifecycle, SlowOutboundSurvivesUpgradeTier) { TestNetworkProvider network; SendspinClient client(make_config(OUTBOUND_TEST_PORT)); client.set_network_provider(&network); - ASSERT_TRUE(client.start_server()); + ASSERT_TRUE(client.start()); client.loop(); // First tick binds the WS server client.connect_to(server_url(PROXY_LISTEN_PORT)); @@ -479,7 +479,7 @@ TEST(ConnectionLifecycle, InFlightOutboundDoesNotBlockInboundAdmission) { TestNetworkProvider network; SendspinClient client(make_config(ADMIT_TEST_PORT)); client.set_network_provider(&network); - ASSERT_TRUE(client.start_server()); + ASSERT_TRUE(client.start()); client.loop(); // First tick binds the WS server client.connect_to(server_url(STALL_LISTEN_PORT)); @@ -509,7 +509,7 @@ TEST(ConnectionLifecycle, EarlyServerHelloDoesNotWedge) { TestNetworkProvider network; SendspinClient client(make_config(EARLY_HELLO_TEST_PORT)); client.set_network_provider(&network); - ASSERT_TRUE(client.start_server()); + ASSERT_TRUE(client.start()); client.loop(); // First tick binds the WS server FakeServer eager(server_url(EARLY_HELLO_TEST_PORT), "server-eager", {.hello_on_open = true}); @@ -530,7 +530,7 @@ TEST(ConnectionLifecycle, TwoServerRaceResolvedByPreference) { SendspinClient client(make_config(RACE_TEST_PORT)); client.set_network_provider(&network); client.set_persistence_provider(&persistence); - ASSERT_TRUE(client.start_server()); + ASSERT_TRUE(client.start()); client.loop(); // First tick binds the WS server // server-a establishes and is promoted into the empty slot first... @@ -561,7 +561,7 @@ TEST(ConnectionLifecycle, HeldProbesNeverOccupyNursery) { TestNetworkProvider network; SendspinClient client(make_config(EVICT_TEST_PORT)); client.set_network_provider(&network); - ASSERT_TRUE(client.start_server()); + ASSERT_TRUE(client.start()); client.loop(); // First tick binds the WS server // Two held raw probes, enough to fill every nursery slot if they were admitted at accept. @@ -595,7 +595,7 @@ TEST(ConnectionLifecycle, FullNurseryOfLivePeersRejectsNewcomer) { TestNetworkProvider network; SendspinClient client(make_config(REJECT_TEST_PORT)); client.set_network_provider(&network); - ASSERT_TRUE(client.start_server()); + ASSERT_TRUE(client.start()); client.loop(); // First tick binds the WS server // Two mute peers: they upgrade and receive client/hello but never answer it, occupying both From 942195a5d20216001e535edbf0aa88ba46288aa0 Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Thu, 10 Sep 2026 12:20:56 -0400 Subject: [PATCH 02/11] Add EventFlags::clear_all and a non-joining signal_stop to the drain roles Each threaded role's start() cleared a hand-listed set of stale command bits before creating its thread. A bit added to a role's enum later would be forgotten, and the artwork and visualizer lists already differed. clear_all() resets the whole group (masked to the 24 usable bits on FreeRTOS) so the new thread's first wait cannot see a command signalled between the previous join and the restart. ArtworkRole and VisualizerRole gain signal_stop(), which sets COMMAND_STOP and wakes the receive without joining, so a caller can overlap the thread's exit with other teardown; stop() is now signal_stop() plus the join. --- src/artwork_role.cpp | 17 ++++++++++++----- src/artwork_role_impl.h | 3 +++ src/platform/event_flags.h | 26 ++++++++++++++++++++++++++ src/sync_task.cpp | 7 +++---- src/visualizer_role.cpp | 19 +++++++++++++------ src/visualizer_role_impl.h | 3 +++ 6 files changed, 60 insertions(+), 15 deletions(-) diff --git a/src/artwork_role.cpp b/src/artwork_role.cpp index eb12d467..165b1f2b 100644 --- a/src/artwork_role.cpp +++ b/src/artwork_role.cpp @@ -101,10 +101,10 @@ bool ArtworkRole::Impl::start() { return false; } - // The flags survive a stop()/start() cycle. The exiting thread's own wait() normally clears - // COMMAND_STOP, but clear it here too so the new thread's first wait() can never see a stale - // stop and exit immediately. - this->drain_task->event_flags.clear(COMMAND_STOP); + // The flags survive a stop()/start() cycle, and a command signalled between the join and this + // start (cleanup() on a stopped role) is still set. Clear the whole group so the new thread's + // first wait() starts from a clean command state whatever bits the role defines. + this->drain_task->event_flags.clear_all(); platform_configure_thread("SsArt", 4096, static_cast(this->config.priority), this->config.psram_stack); @@ -112,7 +112,7 @@ bool ArtworkRole::Impl::start() { return true; } -void ArtworkRole::Impl::stop() const { +void ArtworkRole::Impl::signal_stop() const { if (!this->drain_task || !this->drain_task->drain_thread.joinable()) { return; } @@ -121,6 +121,13 @@ void ArtworkRole::Impl::stop() const { // pulls it out of its blocking queue receive. this->drain_task->event_flags.set(COMMAND_STOP); this->drain_task->notify_queue.wake_receiver(); +} + +void ArtworkRole::Impl::stop() const { + if (!this->drain_task || !this->drain_task->drain_thread.joinable()) { + return; + } + this->signal_stop(); this->drain_task->drain_thread.join(); // Joined, so this is the queue's only consumer: discard notifications the old thread never diff --git a/src/artwork_role_impl.h b/src/artwork_role_impl.h index 46a6d025..d9823d1e 100644 --- a/src/artwork_role_impl.h +++ b/src/artwork_role_impl.h @@ -181,6 +181,9 @@ struct ArtworkRole::Impl { // Helpers // ======================================== + /// @brief Asks the decode thread to exit without waiting for it; stop() joins. No-op when + /// the thread is not running. Lets a caller overlap the thread's exit with other teardown. + void signal_stop() const; void stop() const; void enqueue_stream_event(ArtworkEventType event) const; // Merges a single-slot display delta into the accumulated cross-thread update. Called under diff --git a/src/platform/event_flags.h b/src/platform/event_flags.h index 89c6377b..5a6ad4c1 100644 --- a/src/platform/event_flags.h +++ b/src/platform/event_flags.h @@ -92,6 +92,15 @@ class EventFlags { return xEventGroupClearBits(this->handle_, bits); } + /// @brief Clears every bit, returning the group to its freshly created state + /// + /// For resetting a group whose consumer thread has been joined before a new one starts, so + /// no caller has to enumerate the bits its group defines. + /// @return Bit pattern captured before clearing. + uint32_t clear_all() { + return xEventGroupClearBits(this->handle_, USABLE_BITS); + } + /// @brief Returns the current bit pattern /// @return Current bit pattern. uint32_t get() const { @@ -110,6 +119,14 @@ class EventFlags { } private: + /// The bits a FreeRTOS event group exposes: 24 with 32-bit ticks, 8 with 16-bit ticks. The + /// upper bits are reserved by the kernel and must never be passed to the clear/set calls. +#if configUSE_16_BIT_TICKS == 1 + static constexpr uint32_t USABLE_BITS = 0x00FFU; +#else + static constexpr uint32_t USABLE_BITS = 0x00FFFFFFU; +#endif + // Pointer fields EventGroupHandle_t handle_{nullptr}; }; @@ -190,6 +207,15 @@ class EventFlags { return old; } + /// @brief Clears every bit, returning the group to its freshly created state + /// + /// For resetting a group whose consumer thread has been joined before a new one starts, so + /// no caller has to enumerate the bits its group defines. + /// @return Bit pattern captured before clearing. + uint32_t clear_all() { + return this->clear(~0U); + } + /// @brief Returns the current bit pattern /// @return Current bit pattern. uint32_t get() const { diff --git a/src/sync_task.cpp b/src/sync_task.cpp index 7ab9f5b8..bdd365ed 100644 --- a/src/sync_task.cpp +++ b/src/sync_task.cpp @@ -121,10 +121,9 @@ bool SyncTask::start(bool task_stack_in_psram, unsigned priority) { return false; } - this->event_flags_.clear(EventGroupBits::TASK_RUNNING | EventGroupBits::TASK_STOPPED | - EventGroupBits::TASK_IDLE | EventGroupBits::COMMAND_STOP | - EventGroupBits::COMMAND_STREAM_END | - EventGroupBits::COMMAND_STREAM_CLEAR | EventGroupBits::COMMAND_START); + // A fresh thread starts from a clean group: no stale task state and no command signalled + // between the previous join and this start (cleanup() on a stopped task). + this->event_flags_.clear_all(); platform_configure_thread("Sendspin", SYNC_TASK_STACK_SIZE, static_cast(priority), task_stack_in_psram); diff --git a/src/visualizer_role.cpp b/src/visualizer_role.cpp index d552d194..cb93b0bc 100644 --- a/src/visualizer_role.cpp +++ b/src/visualizer_role.cpp @@ -176,11 +176,11 @@ bool VisualizerRole::Impl::start() { return false; } - // The flags survive a stop()/start() cycle. The exiting thread's own wait() normally clears - // COMMAND_STOP, but a flush or clear signalled after the join (cleanup() on a stopped role) - // is still set; clear all three so the new thread starts from a clean command state (stop() - // already emptied the ring). - this->drain_task->event_flags.clear(COMMAND_STOP | COMMAND_FLUSH | COMMAND_CLEAR); + // The flags survive a stop()/start() cycle, and a flush or clear signalled between the join + // and this start (cleanup() on a stopped role) is still set. Clear the whole group so the new + // thread starts from a clean command state whatever bits the role defines (stop() already + // emptied the ring). + this->drain_task->event_flags.clear_all(); platform_configure_thread("SsVis", 4096, static_cast(this->config.priority), this->config.psram_stack); @@ -188,7 +188,7 @@ bool VisualizerRole::Impl::start() { return true; } -void VisualizerRole::Impl::stop() const { +void VisualizerRole::Impl::signal_stop() const { if (!this->drain_task || !this->drain_task->drain_thread.joinable()) { return; } @@ -197,6 +197,13 @@ void VisualizerRole::Impl::stop() const { // it was parked in (display-time flags wait or ring buffer receive). this->drain_task->event_flags.set(COMMAND_STOP); this->drain_task->ring_buffer.wake_receiver(); +} + +void VisualizerRole::Impl::stop() const { + if (!this->drain_task || !this->drain_task->drain_thread.joinable()) { + return; + } + this->signal_stop(); this->drain_task->drain_thread.join(); // Joined, so this is the ring's only consumer (the single-consumer contract the ring diff --git a/src/visualizer_role_impl.h b/src/visualizer_role_impl.h index 56cad0c0..1912c535 100644 --- a/src/visualizer_role_impl.h +++ b/src/visualizer_role_impl.h @@ -111,6 +111,9 @@ struct VisualizerRole::Impl { // Internal helpers // ======================================== + /// @brief Asks the drain thread to exit without waiting for it; stop() joins. No-op when + /// the thread is not running. Lets a caller overlap the thread's exit with other teardown. + void signal_stop() const; void stop() const; void flush_ring_buffer() const; void signal_clear_marker() const; From 2234ca53e727beb688586ee66a8186b2bd5e1cff Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Thu, 10 Sep 2026 12:20:57 -0400 Subject: [PATCH 03/11] Scale the goodbye flush bound per peer and simplify ConnectionManager::stop GOODBYE_FLUSH_TIMEOUT_MS was one 50 ms window shared by every peer stop() goodbyes. On ESP the httpd worker hands the frames to lwIP one at a time, so several peers could exhaust the window and the last would lose its goodbye to the close. The wait is now the constant times the number of goodbyes issued. stop() no longer partitions deferred releases into goodbye and drop lists: every transport's disconnect() completes immediately on a disconnected connection, so the reason-less releases take the same path. The pending lifecycle-event drain that appeared three times (destructor, stop(), loop()) is one take_pending_events() helper. start() re-applies the server port, connection budget, and control port on every call so a restart listens with the config the client holds now rather than the values captured at first start. --- src/connection_manager.cpp | 63 +++++++++++++++++++++----------------- src/connection_manager.h | 30 ++++++++++++------ 2 files changed, 56 insertions(+), 37 deletions(-) diff --git a/src/connection_manager.cpp b/src/connection_manager.cpp index 36dbeb14..a9d0d758 100644 --- a/src/connection_manager.cpp +++ b/src/connection_manager.cpp @@ -94,12 +94,7 @@ ConnectionManager::~ConnectionManager() { // The two mutexes guard disjoint state and are taken in separate scopes, never nested. std::vector> pending_connected; std::vector> pending_disconnects; - { - std::lock_guard lock(this->conn_mutex_); - pending_connected = std::move(this->pending_connected_events_); - pending_disconnects = std::move(this->pending_disconnect_events_); - this->has_pending_events_.store(false, std::memory_order_release); - } + this->take_pending_events(pending_connected, pending_disconnects); std::shared_ptr current; std::vector nursery; @@ -231,14 +226,18 @@ void ConnectionManager::start() { // once the network is ready. Retry immediately rather than honoring a backoff from before // the stop. this->ws_server_start_retry_time_us_ = 0; - if (this->ws_server_ != nullptr) { - return; + const bool first_start = this->ws_server_ == nullptr; + if (first_start) { + this->ws_server_ = std::make_unique(); } - - this->ws_server_ = std::make_unique(); + // Applied on every start, not just the first: the transport is (re)created from these values + // when loop() starts it, so a restart listens with the config the client holds now. this->ws_server_->set_port(this->client_->config_.server_port); this->ws_server_->set_max_connections(this->client_->config_.server_max_connections); this->ws_server_->set_ctrl_port(this->client_->config_.httpd_ctrl_port); + if (!first_start) { + return; + } // Graceful rejection needs transport headroom: the manager can hold one established inbound // connection plus NURSERY_CAPACITY unproven ones, and rejecting a surplus peer with a @@ -294,7 +293,6 @@ void ConnectionManager::stop(SendspinGoodbyeReason reason) { // released here (see DeferredRelease): the goodbyes below run outside the lock, and a // rejection for a peer delivered during the wait can take the lock meanwhile. std::vector> to_goodbye; - std::vector> to_drop; { std::lock_guard lock(this->conn_ptr_mutex_); this->accepting_.store(false, std::memory_order_release); @@ -311,9 +309,12 @@ void ConnectionManager::stop(SendspinGoodbyeReason reason) { this->nursery_size_.store(0, std::memory_order_release); this->hello_retries_.clear(); // Releases already queued (a handoff loser, a reaped entry) had their dispatch disabled - // when they were queued; the shutdown goodbye replaces whatever reason they carried. + // when they were queued; the shutdown goodbye replaces whatever reason they carried. One + // queued without a reason has a transport that is already gone, and every transport's + // disconnect() completes immediately on a disconnected connection, so it needs no + // separate path. for (auto& release : this->deferred_releases_) { - (release.goodbye.has_value() ? to_goodbye : to_drop).push_back(std::move(release.conn)); + to_goodbye.push_back(std::move(release.conn)); } this->deferred_releases_.clear(); this->deferred_size_.store(0, std::memory_order_release); @@ -328,9 +329,13 @@ void ConnectionManager::stop(SendspinGoodbyeReason reason) { wait->add_pending(); conn->disconnect(reason, [wait] { wait->complete_one(); }); } - if (!wait->wait(GOODBYE_FLUSH_TIMEOUT_MS)) { - SS_LOGD(TAG, "Goodbye flush bound (%u ms) elapsed; closing regardless", - static_cast(GOODBYE_FLUSH_TIMEOUT_MS)); + // The bound scales with the goodbyes issued: on ESP they are handed to lwIP one at a time by + // the single httpd worker, so several peers need several quanta. + const uint32_t flush_bound_ms = + GOODBYE_FLUSH_TIMEOUT_MS * static_cast(to_goodbye.size()); + if (!wait->wait(flush_bound_ms)) { + SS_LOGD(TAG, "Goodbye flush bound (%u ms for %u goodbyes) elapsed; closing regardless", + static_cast(flush_bound_ms), static_cast(to_goodbye.size())); } // Tear the server down regardless. This joins every network thread on host and waits for @@ -340,20 +345,25 @@ void ConnectionManager::stop(SendspinGoodbyeReason reason) { this->ws_server_->stop(); } - // Drop the lifecycle events those closes queued: the connections they name are gone. Moved - // out under the lock and destroyed after it, since a destructor can join a transport thread. + // Drop the lifecycle events those closes queued: the connections they name are gone. std::vector> pending_connected; std::vector> pending_disconnects; - { - std::lock_guard lock(this->conn_mutex_); - pending_connected = std::move(this->pending_connected_events_); - pending_disconnects = std::move(this->pending_disconnect_events_); - this->has_pending_events_.store(false, std::memory_order_release); - } + this->take_pending_events(pending_connected, pending_disconnects); // Locals release here, outside every lock. An outbound connection's destructor stops its // transport synchronously; deferring that is not an option (see DeferredRelease). } +void ConnectionManager::take_pending_events( + std::vector>& connected, + std::vector>& disconnects) { + std::lock_guard lock(this->conn_mutex_); + connected = std::move(this->pending_connected_events_); + disconnects = std::move(this->pending_disconnect_events_); + this->pending_connected_events_.clear(); + this->pending_disconnect_events_.clear(); + this->has_pending_events_.store(false, std::memory_order_release); +} + void ConnectionManager::loop() { // Start WS server when network becomes ready. A persistent failure (e.g. the server port is // already in use) is retried with backoff instead of on every tick, which would spam the log. @@ -376,10 +386,7 @@ void ConnectionManager::loop() { // Sound because every push site sets has_pending_events_ = true under conn_mutex_ before // releasing it (see the field's doc comment in connection_manager.h). if (this->has_pending_events_.load(std::memory_order_acquire)) { - std::lock_guard lock(this->conn_mutex_); - connected_events.swap(this->pending_connected_events_); - disconnect_events.swap(this->pending_disconnect_events_); - this->has_pending_events_.store(false, std::memory_order_release); + this->take_pending_events(connected_events, disconnect_events); } // Also runs whenever the nursery is non-empty even with no swapped-out events: the diff --git a/src/connection_manager.h b/src/connection_manager.h index aa5cacee..172956f3 100644 --- a/src/connection_manager.h +++ b/src/connection_manager.h @@ -66,12 +66,14 @@ static constexpr int64_t LIVENESS_TOLERATED_MISSES = 2; /// @return Timeout in milliseconds; 0 or negative disables the check. int64_t resolve_liveness_timeout_ms(const SendspinClientConfig& config); -/// @brief Bound (milliseconds) on waiting for stop()'s goodbyes to be sent before the transports -/// are torn down +/// @brief Bound (milliseconds, per goodbye) on waiting for stop()'s goodbyes to be sent before +/// the transports are torn down /// -/// The host transports send synchronously, so on host the wait resolves before it starts. On the -/// ESP server path the goodbye is queued to the httpd worker, and this is a few scheduler quanta -/// for the worker to dequeue the frame and hand it to lwIP. Send completion is best-effort (see +/// stop() waits this long times the number of goodbyes it issued: on the ESP server path every +/// goodbye is queued to the single httpd worker and handed to lwIP in turn, so a fixed bound +/// would let the last of several peers lose its goodbye to the close. Per goodbye this is a few +/// scheduler quanta for the worker to dequeue the frame. The host transports send synchronously, +/// so on host the wait resolves before it starts. Send completion is best-effort (see /// SendspinConnection::send_text_message): a session that closes first never reports, so this is /// a cap on how long stop() blocks for its peers' sake, never a guarantee the goodbye arrived. static constexpr uint32_t GOODBYE_FLUSH_TIMEOUT_MS = 50; @@ -207,13 +209,14 @@ class ConnectionManager { /// @brief Opens admission and creates the WebSocket server on first use /// - /// Server configuration is read from the client's config. loop() starts the server once the - /// network provider reports ready. Main-loop thread only. + /// Server configuration is read from the client's config and applied on every call, so a + /// restart picks up the current values. loop() starts the server once the network provider + /// reports ready. Main-loop thread only. void start(); /// @brief Synchronous teardown: goodbyes every managed connection, waits up to - /// GOODBYE_FLUSH_TIMEOUT_MS for the sends to complete, then stops the WebSocket server and - /// releases every connection regardless + /// GOODBYE_FLUSH_TIMEOUT_MS per goodbye for the sends to complete, then stops the WebSocket + /// server and releases every connection regardless /// /// Closes admission first, so a peer delivered during the wait is rejected with a goodbye. /// Blocks on the transports' own teardown as well as the flush bound: the host server joins @@ -313,6 +316,15 @@ class ConnectionManager { /// @param conn The freshly connected connection to defer to loop(). void queue_pending_connected(std::shared_ptr conn); + /// @brief Moves both pending lifecycle event queues out under conn_mutex_ and clears + /// has_pending_events_ in the same critical section. Caller must NOT hold conn_mutex_ and + /// must let the returned connections release outside every lock (a connection destructor + /// can join its transport thread). + /// @param connected Receives pending_connected_events_. + /// @param disconnects Receives pending_disconnect_events_. + void take_pending_events(std::vector>& connected, + std::vector>& disconnects); + /// @brief Appends a connection to pending_disconnect_events_ and sets has_pending_events_ in /// the same critical section, so loop()'s lock-free gate can never miss a pushed event. /// Caller must hold conn_mutex_. From c1c3559a8d600fd47284606650770a6e1389b0bb Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Thu, 10 Sep 2026 12:20:57 -0400 Subject: [PATCH 04/11] Deliver the high-performance release lock-free and tighten the client lifecycle release_high_performance() called the listener inline. Its last release can run inside ConnectionManager::drop_connection(), which holds conn_ptr_mutex_ through cleanup_connection_state() (both the time-burst hold and the player's playback hold are released there), so a listener that reacted by calling disconnect() or connect_to() re-locked the non-recursive mutex on the same thread and hung the main loop. The release is now recorded and delivered at the top of drain_inbox(), which loop() and stop() both run with no lock held; an acquire that lands first cancels it so request and release stay paired. The started_/stopping_ pair becomes one atomic LifecycleState (STOPPED, RUNNING, STOPPING). is_started() is safe from any thread and reads false for the whole of stop(); connect_to() and disconnect() refuse unless RUNNING, so a callback fired during teardown cannot reach the manager mid-stop. stop() sets STOPPING first, signals the artwork and visualizer threads before the transport teardown so their exit overlaps it (the player keeps consuming until the network threads are joined), resets group and client state before the clear callbacks fire so a callback sees the stopped state through the getters, and only then drains. The destructor drops its redundant explicit role join; the role destructors perform it. Docs: internals.md gains the new stop sequence and a section on release delivery, the stale ConnectionManager::init_server reference is fixed, and the public stop() comment points at the integration guide for transport bounds. --- docs/integration-guide.md | 6 +- docs/internals.md | 43 +++++++---- include/sendspin/client.h | 73 ++++++++++++------- src/client.cpp | 145 ++++++++++++++++++++++++++------------ 4 files changed, 180 insertions(+), 87 deletions(-) diff --git a/docs/integration-guide.md b/docs/integration-guide.md index 23a08055..b82fee22 100644 --- a/docs/integration-guide.md +++ b/docs/integration-guide.md @@ -546,7 +546,7 @@ client.stop(); ## Stopping and Restarting -`stop()` is synchronous: when it returns the client is fully stopped. It sends a `client/goodbye` (reason `shutdown`) to every peer, waits up to a short bound (50 ms) for those sends to complete, then closes the server and every connection regardless, joins the role threads, resets every role, and delivers the roles' clear callbacks (`on_stream_end()`, `on_image_clear()`, `on_visualizer_stream_end()`, `on_metadata_clear()`, `on_controller_state_clear()`, `on_color_clear()`) before returning. It is a no-op on a stopped client. `is_started()` reports the state, and `loop()` is a no-op while stopped. +`stop()` is synchronous: when it returns the client is fully stopped. It sends a `client/goodbye` (reason `shutdown`) to every peer, waits up to a short bound (50 ms per peer) for those sends to complete, then closes the server and every connection regardless, joins the role threads, resets every role, and delivers the roles' clear callbacks (`on_stream_end()`, `on_image_clear()`, `on_visualizer_stream_end()`, `on_metadata_clear()`, `on_controller_state_clear()`, `on_color_clear()`) before returning. It is a no-op on a stopped client. `is_started()` reports the state, and `loop()` is a no-op while stopped. Restarting is `start()` again; start, stop, and start again can be repeated indefinitely, and a restarted client begins with no connection, no group state, and no role state from before the stop. @@ -556,7 +556,9 @@ Restarting is `start()` again; start, stop, and start again can be repeated inde - An outbound `connect_to()` connection's transport stop, which is synchronous (`esp_websocket_client_stop()` / `ix::WebSocket::stop()`). - A listener callback already running on a role thread: the join cannot interrupt it. `on_audio_write()` is bounded by its `timeout_ms`; `on_image_decode()` has no bound. -Listener callbacks fire from inside `stop()`. One that calls `start()` gets `false` and starts nothing; one that calls `stop()` or `connect_to()` is ignored. Call `stop()` only from the main loop thread: from a role-thread callback it would join the calling thread. +Listener callbacks fire from inside `stop()`, after every role and the group state have been reset, so a callback that reads the client through its getters sees the stopped state. One that calls `start()` gets `false` and starts nothing; one that calls `stop()`, `connect_to()`, or `disconnect()` is ignored. `is_started()` reads `false` throughout and is safe to call from any thread. Call `stop()` only from the main loop thread: from a role-thread callback it would join the calling thread. + +`on_release_high_performance()` is delivered from `loop()` (or from inside `stop()`) rather than from wherever the last hold was released, so it is always safe to call `disconnect()` or `connect_to()` from that callback. Destroying a running client performs the transport half of `stop()` (goodbye, bounded wait, close, join) but delivers no listener callback, so a consumer that destroys its listeners before the client is never called into. Call `stop()` first when the clear callbacks matter. diff --git a/docs/internals.md b/docs/internals.md index e63cf209..f5098c22 100644 --- a/docs/internals.md +++ b/docs/internals.md @@ -456,33 +456,50 @@ When a connection is lost (`on_connection_lost`): `SendspinClient::start()` loads persisted state, starts the threaded roles (player sync task, visualizer drain, artwork decode; a failure part-way stops the ones that did start), and calls `ConnectionManager::start()`, which opens admission (`accepting_`) and creates the `SendspinWsServer` on first use. The server itself is started by the manager's `loop()` once the network provider reports ready, so `is_started()` means "running", not "listening". -`SendspinClient::stop()` is synchronous and ordered so that every producer is gone before any state is reset: +`SendspinClient::stop()` is synchronous and ordered so that every producer is gone before any state is reset. The client's lifecycle is one atomic `lifecycle_` field (`STOPPED`, `RUNNING`, `STOPPING`); `is_started()` reads it from any thread. ```api -1. ConnectionManager::stop(SHUTDOWN) +0. lifecycle_ = STOPPING (is_started() reads false; loop() is a no-op; start() is refused and + stop()/connect_to()/disconnect() are ignored from here on, so a callback fired below cannot + recurse into the teardown) +1. VisualizerRole/ArtworkRole::Impl::signal_stop(): set COMMAND_STOP and wake, no join, so a + slow on_image_decode() or a parked drain exits while the transports close. The player is + not signalled yet: a network thread blocked on its ring (write_audio_chunk) needs the sync + task alive until the network threads are gone +2. ConnectionManager::stop(SHUTDOWN) ├─ Under conn_ptr_mutex_: accepting_ = false; disable_message_dispatch() on every managed │ connection; move the current slot, the nursery, and the deferred releases out; clear the │ hello retries ├─ Outside the lock: conn->disconnect(SHUTDOWN, completion) on each, completion counted by a - │ shared GoodbyeWait; wait up to GOODBYE_FLUSH_TIMEOUT_MS (50 ms) for the count to reach zero + │ shared GoodbyeWait; wait up to GOODBYE_FLUSH_TIMEOUT_MS (50 ms) per goodbye for the count + │ to reach zero (the ESP httpd worker hands the frames to lwIP one at a time) ├─ ws_server_->stop() regardless (host: joins every connection thread, each bounded by │ IXWebSocket's 300 ms close handshake; ESP: httpd_stop(), which runs queued sends first, │ then every session's close_fn and ctx free_fn, polling at 100 ms) - └─ Move the pending connected/disconnect event queues out under conn_mutex_; every moved-out - shared_ptr is released outside the locks (an outbound connection's destructor stops its - transport synchronously) -2. Role threads: PlayerRole/VisualizerRole/ArtworkRole::Impl::stop() join, then each discards + └─ take_pending_events(): move the pending connected/disconnect event queues out under + conn_mutex_; every moved-out shared_ptr is released outside the locks (an outbound + connection's destructor stops its transport synchronously) +3. Role threads: PlayerRole/VisualizerRole/ArtworkRole::Impl::stop() join, then each discards its ring/queue content (sole consumer after the join) -3. started_ = false (loop() is a no-op and connect_to() is refused from here on) -4. cleanup_connection_state() (the same reset a lost connection triggers), then drain_inbox() - delivers the CLEARED / STREAM_END callbacks it queued; group_state_ and state_ are reset +4. cleanup_connection_state() (the same reset a lost connection triggers, including the group + slot), then group_state_ and state_ are reset +5. drain_inbox() delivers the CLEARED / STREAM_END callbacks step 4 queued. Every getter already + reports the stopped state, so a callback that reads the client sees what a caller sees once + stop() returns +6. lifecycle_ = STOPPED ``` The goodbye completion is best-effort: on ESP a session that closes before its queued worker runs, or whose `weak_ptr` no longer resolves, never reports, which is why the wait is bounded rather than exact. The `GoodbyeWait` record is held by `shared_ptr` and captured by value in each completion, so a completion that runs late on a transport thread touches nothing `stop()` owns. A peer delivered by the ws_server while admission is closed is rejected in `on_new_connection()` with a shutdown goodbye, the same shape as the nursery-full rejection. -`stopping_` guards re-entrancy: a listener callback fired from inside `stop()` that calls `start()` gets `false`, and one that calls `stop()` returns immediately. The reset in step 4 runs with no manager lock held, so it does not reach the pre-existing lock re-entry in which `drop_connection()` holds `conn_ptr_mutex_` through `cleanup_connection_state()` and a listener's `on_release_high_performance()` calls `disconnect()`. +`ConnectionManager::start()` re-applies the server port, connection budget, and control port from the client config on every call, so a restart listens with the values the client holds now rather than the ones captured at the first start. -The client destructor performs steps 1 and 2 only, so a consumer that destroyed its listeners first is never called into; the roles' own destructors then run as before. +Each threaded role's `start()` calls `EventFlags::clear_all()` before creating its thread rather than clearing a hand-listed set of bits: a command signalled between the previous join and the restart (`cleanup()` on a stopped role) would otherwise survive into the new thread's first wait, and a bit added to the role's enum later cannot be forgotten. + +The client destructor performs steps 1 and 2 only, so a consumer that destroyed its listeners first is never called into; the roles' own destructors then join their threads as before. + +### High-performance release delivery + +`release_high_performance()` never calls the listener inline. The last release can run inside `drop_connection()`, which holds `conn_ptr_mutex_` through `cleanup_connection_state()` (both the time-burst hold and the player's playback hold are released there), and a listener whose `on_release_high_performance()` reacts by calling `disconnect()` or `connect_to()` would re-lock the same non-recursive mutex on the same thread. The release is recorded in `high_performance_release_pending_` and delivered at the top of `drain_inbox()`, which every path (`loop()` and `stop()`) runs with no manager lock held. An `acquire_high_performance()` that lands before the delivery cancels it instead of issuing a second request, so the listener always sees request and release strictly alternate. ### Graceful Disconnect @@ -504,7 +521,7 @@ Queued send workers capture a `weak_ptr` to the origin The send workers also enforce the protocol's "hello is always first" rule: a frame is dropped unless `client_hello_sent_` is set on the resolved connection, *unless* the caller passed `allow_before_hello=true`. Exactly two callers do — the `client/hello` itself (which would otherwise gate its own send and deadlock) and `goodbye` — so a stale or out-of-order frame can never precede the handshake. The `weak_ptr` guards identity; the gate guards ordering; the two are independent. -The host build does not need this scheme: `SendspinWsServer` (host) routes IXWebSocket messages by calling `find_connection_callback_` to resolve a synthetic sockfd back to the connection that `ConnectionManager` is holding. The ESP build keeps the `set_find_connection_callback()` setter as a no-op stub for symmetry; see the comment at the call site in `ConnectionManager::init_server`. +The host build does not need this scheme: `SendspinWsServer` (host) routes IXWebSocket messages by calling `find_connection_callback_` to resolve a synthetic sockfd back to the connection that `ConnectionManager` is holding. The ESP build keeps the `set_find_connection_callback()` setter as a no-op stub for symmetry; see the comment at the call site in `ConnectionManager::start`. ## Ordering Guarantees Summary diff --git a/include/sendspin/client.h b/include/sendspin/client.h index 5e7df32f..891720ab 100644 --- a/include/sendspin/client.h +++ b/include/sendspin/client.h @@ -213,32 +213,32 @@ class SendspinClient { /// @brief Stops the client and returns only once it is fully stopped /// - /// Sends a client/goodbye (reason shutdown) to every peer, waits up to a short bound for - /// those sends to complete, then closes the server and every connection regardless, joins - /// the role threads, resets every role, and delivers the roles' clear callbacks - /// (on_stream_end(), on_image_clear(), on_metadata_clear(), ...) before returning. No-op - /// when stopped. Calling start() afterwards restarts the client; start, stop, and start - /// again can be repeated indefinitely. + /// Sends a client/goodbye (reason shutdown) to every peer, waits a short bound for those + /// sends to complete, then closes the server and every connection regardless, joins the role + /// threads, resets every role, and delivers the roles' clear callbacks (on_stream_end(), + /// on_image_clear(), on_metadata_clear(), ...) before returning. No-op when stopped. Calling + /// start() afterwards restarts the client; start, stop, and start again can be repeated + /// indefinitely. /// - /// Blocking is bounded, but not by the goodbye bound alone. It also includes: the - /// transports' own close (the host server waits up to 300 ms per connection for the close - /// handshake; the ESP server waits for the httpd task to exit, which polls at 100 ms and - /// first finishes any queued send, up to its send timeout for a peer that stops reading); an - /// outbound connect_to() transport's synchronous stop; and any listener callback already - /// running on a role thread, which the join cannot interrupt (on_audio_write() is bounded by - /// its timeout_ms, on_image_decode() is not). + /// Blocking is bounded by the goodbye wait, the transports' own close, and any listener + /// callback already running on a role thread, which the join cannot interrupt. The + /// per-transport bounds are described in docs/integration-guide.md (Stopping and + /// Restarting). /// - /// Listener callbacks fire from inside this call. One that calls start() has no effect and - /// returns false; one that calls stop() or connect_to() is ignored. Main-loop thread only: - /// calling it from a role-thread callback would join the calling thread. + /// Listener callbacks fire from inside this call, after every role has been reset, so the + /// state they observe through the getters is the stopped state. One that calls start() has + /// no effect and returns false; one that calls stop(), connect_to(), or disconnect() is + /// ignored. Main-loop thread only: calling it from a role-thread callback would join the + /// calling thread. void stop(); /// @brief Returns true between a successful start() and stop() /// /// Running means the role threads are up and the server is armed, not that the server is - /// listening yet (that waits for the network provider). + /// listening yet (that waits for the network provider). Reads false for the whole duration + /// of stop(), including from the clear callbacks it fires. Safe to call from any thread. bool is_started() const { - return this->started_; + return this->lifecycle_.load(std::memory_order_acquire) == LifecycleState::RUNNING; } /// @brief Starts the client @@ -251,15 +251,16 @@ class SendspinClient { /// @brief Initiates a client connection to a Sendspin server at the given URL /// - /// Ignored (with a warning) while the client is not started. Must be called from the main - /// loop thread: it tears down and replaces connection state (time filter, dispatch, client - /// state) directly rather than deferring to loop(), so calling it concurrently with loop() - /// would race those mutations. + /// Ignored (with a warning) unless the client is running, including from a callback fired + /// inside stop(). Must be called from the main loop thread: it tears down and replaces + /// connection state (time filter, dispatch, client state) directly rather than deferring to + /// loop(), so calling it concurrently with loop() would race those mutations. /// @param url WebSocket server URL (e.g., "ws://server.local:8927/sendspin") void connect_to(const std::string& url); /// @brief Disconnects from the current server with the given reason /// + /// Ignored unless the client is running, including from a callback fired inside stop(). /// Must be called from the main loop thread: the blocking transport close runs outside the /// manager lock, so a call from another thread could race loop()'s own release of the same /// connection (two concurrent transport stops). @@ -451,6 +452,11 @@ class SendspinClient { void acquire_high_performance(); /// @brief Releases a ref-counted high-performance networking request + /// + /// The listener's on_release_high_performance() is not called inline: the last release can + /// run inside ConnectionManager::drop_connection(), which holds conn_ptr_mutex_, and a + /// listener that reacts by calling disconnect() or connect_to() would re-lock it on the same + /// thread. The release is recorded and delivered on the next drain with no lock held. void release_high_performance(); private: @@ -461,6 +467,15 @@ class SendspinClient { /// listener callbacks on the calling (main-loop) thread. Shared by loop() and stop(). void drain_inbox(); + /// @brief Delivers a high-performance release the counter recorded while a manager lock was + /// held (see release_high_performance()). Main-loop thread, no lock held. + void deliver_pending_high_performance_release(); + + /// @brief Asks the artwork and visualizer threads to exit without joining them, so their + /// exit overlaps the transport teardown. The player is excluded: its ring must keep a + /// consumer until the network threads are gone (see stop()). + void signal_drain_role_stops(); + /// @brief Stops and joins every threaded role; each is a no-op if not running void stop_role_threads(); @@ -555,11 +570,15 @@ class SendspinClient { // 8-bit fields bool high_performance_held_for_time_{false}; std::atomic high_performance_ref_count_{0}; - /// True between a successful start() and stop(); see is_started(). - bool started_{false}; - /// True for the duration of stop(). Refuses a start() and ignores a stop() issued by a - /// listener callback fired from inside the teardown, which would otherwise recurse into it. - bool stopping_{false}; + /// Set when the high-performance count reaches zero; the listener's release callback is + /// delivered from the main loop with no manager lock held (see release_high_performance()). + std::atomic high_performance_release_pending_{false}; + /// Where the client is in its lifecycle. Written only by start()/stop() on the main loop; + /// atomic so is_started() can be read from any thread. STOPPING covers the whole of stop(): + /// start() is refused and stop()/connect_to()/disconnect() are ignored while it is set, so a + /// listener callback fired from inside the teardown cannot recurse into it. + enum class LifecycleState : uint8_t { STOPPED, RUNNING, STOPPING }; + std::atomic lifecycle_{LifecycleState::STOPPED}; }; } // namespace sendspin diff --git a/src/client.cpp b/src/client.cpp index fd6d3dba..710e738d 100644 --- a/src/client.cpp +++ b/src/client.cpp @@ -79,17 +79,19 @@ SendspinClient::SendspinClient(SendspinClientConfig config) } SendspinClient::~SendspinClient() { - // Transport-only teardown: goodbye and close every peer and join every thread, exactly as - // stop() does, but deliver no listener callback. A consumer that destroys its listeners - // before the client (the natural declaration order when a listener needs a role reference) - // is never called into from here. - if (this->started_) { + // Transport-only teardown: goodbye and close every peer in the same order as stop(), but + // deliver no listener callback. A consumer that destroys its listeners before the client + // (the natural declaration order when a listener needs a role reference) is never called + // into from here. The role threads are joined by the role resets below, whose destructors + // run the same stop() the explicit path would. + if (this->lifecycle_.load(std::memory_order_relaxed) != LifecycleState::STOPPED) { + this->signal_drain_role_stops(); this->connection_manager_->stop(SendspinGoodbyeReason::SHUTDOWN); - this->stop_role_threads(); } - // Stop background threads before tearing down connections. Every role is reset explicitly - // (not just the threaded ones): role InboxSlots release their topic-bit claims against + // The network threads are gone (above, or never started), so the role threads are the only + // producers left; each role's destructor joins its own. Every role is reset explicitly (not + // just the threaded ones): role InboxSlots release their topic-bit claims against // event_state_'s Inbox on destruction, so all roles must be gone before the alphabetized // member order destroys event_state_. #ifdef SENDSPIN_ENABLE_PLAYER @@ -126,12 +128,14 @@ LogLevel SendspinClient::get_log_level() { // ============================================================================ bool SendspinClient::start() { - if (this->started_) { - return true; - } - if (this->stopping_) { - SS_LOGW(TAG, "start() ignored: called from a callback while stop() is in progress"); - return false; + switch (this->lifecycle_.load(std::memory_order_relaxed)) { + case LifecycleState::RUNNING: + return true; + case LifecycleState::STOPPING: + SS_LOGW(TAG, "start() ignored: called from a callback while stop() is in progress"); + return false; + case LifecycleState::STOPPED: + break; } // Load persisted state @@ -163,39 +167,62 @@ bool SendspinClient::start() { // Open admission and create the WebSocket server (started by loop() once the network is // ready). this->connection_manager_->start(); - this->started_ = true; + this->lifecycle_.store(LifecycleState::RUNNING, std::memory_order_release); return true; } void SendspinClient::stop() { - if (!this->started_ || this->stopping_) { + if (this->lifecycle_.load(std::memory_order_relaxed) != LifecycleState::RUNNING) { return; } - this->stopping_ = true; - - // 1. Transports first: goodbye every peer, wait up to the flush bound, then close the server - // and every connection. This joins every network thread, so nothing reaches a role or the - // inbox from the network after it returns, and a network thread blocked on ring space - // (write_audio_chunk) resolves while its consumer is still alive. + // From here the client reads as stopped: is_started() is false, loop() is a no-op, and + // start()/stop()/connect_to()/disconnect() are refused, so a listener callback fired below + // cannot recurse into the teardown, restart the server, or admit a connection. + this->lifecycle_.store(LifecycleState::STOPPING, std::memory_order_release); + + // 1. Ask the artwork and visualizer threads to exit now, so a slow on_image_decode() or a + // parked drain overlaps the transport teardown instead of following it. Their inbound + // channels never block a network thread (a zero-timeout queue send, a bounded ring + // acquire), so they need no consumer while the transports close. The player's ring does: + // a network thread blocked on ring space (write_audio_chunk) resolves only while the sync + // task is alive, so the player is signalled in step 3, after the network threads are gone. + this->signal_drain_role_stops(); + + // 2. Transports: goodbye every peer, wait up to the flush bound, then close the server and + // every connection. This joins every network thread, so nothing reaches a role or the + // inbox from the network after it returns. this->connection_manager_->stop(SendspinGoodbyeReason::SHUTDOWN); - // 2. Role threads. Each role discards its ring/queue content after its own join. + // 3. Role threads. Each role discards its ring/queue content after its own join. this->stop_role_threads(); - // From here the client reads as stopped: loop() is a no-op and connect_to() is refused, so a - // listener callback below cannot restart the server or admit a connection. - this->started_ = false; - - // 3. Reset per-connection and role state exactly as a lost connection does, then deliver - // the clear callbacks it queued now rather than on a loop() tick that is not coming. With - // every producer thread joined, the state the drain leaves behind is the state a restart - // begins from. + // 4. Reset per-connection and role state exactly as a lost connection does. With every + // producer thread joined, the state this leaves behind is the state a restart begins + // from. The group slot is reset here too, so the drain below cannot repopulate + // group_state_ from a delta that arrived before the stop. this->cleanup_connection_state(); - this->drain_inbox(); this->group_state_ = GroupUpdateObject{}; this->state_ = SendspinClientState::SYNCHRONIZED; - this->stopping_ = false; + // 5. Deliver the clear callbacks the cleanup queued, now rather than on a loop() tick that is + // not coming. Every getter already reports the stopped state, so a callback that reads + // the client sees exactly what a caller sees once stop() returns. + this->drain_inbox(); + + this->lifecycle_.store(LifecycleState::STOPPED, std::memory_order_release); +} + +void SendspinClient::signal_drain_role_stops() { +#ifdef SENDSPIN_ENABLE_VISUALIZER + if (this->visualizer_) { + this->visualizer_->impl_->signal_stop(); + } +#endif +#ifdef SENDSPIN_ENABLE_ARTWORK + if (this->artwork_) { + this->artwork_->impl_->signal_stop(); + } +#endif } void SendspinClient::stop_role_threads() { @@ -217,21 +244,27 @@ void SendspinClient::stop_role_threads() { } void SendspinClient::connect_to(const std::string& url) { - if (!this->started_) { - SS_LOGW(TAG, "connect_to() ignored: client is not started"); + if (!this->is_started()) { + SS_LOGW(TAG, "connect_to() ignored: client is not running"); return; } this->connection_manager_->connect_to(url); } void SendspinClient::disconnect(SendspinGoodbyeReason reason) { + // A stopped client has nothing to disconnect, and inside stop() the manager is already + // goodbying every peer; a second pass would race the first. + if (!this->is_started()) { + SS_LOGD(TAG, "disconnect() ignored: client is not running"); + return; + } this->connection_manager_->disconnect(reason); } void SendspinClient::loop() { // A stopped client is quiescent: no connections, no threads, and the manager loop must not // restart the WebSocket server the moment the network reads ready. - if (!this->started_) { + if (!this->is_started()) { return; } @@ -261,6 +294,8 @@ void SendspinClient::loop() { } void SendspinClient::drain_inbox() { + this->deliver_pending_high_performance_release(); + // Process deferred events: all state mutations and user callbacks happen here, on the main // loop thread, to avoid cross-thread data races. Two poll() snapshots gate the work below: // inbox_bits (here) gates only the event-ring drain immediately following it; slot_bits @@ -475,7 +510,7 @@ void SendspinClient::drain_inbox() { #ifdef SENDSPIN_ENABLE_PLAYER PlayerRole& SendspinClient::add_player(PlayerRoleConfig config) { - if (this->started_) { + if (this->lifecycle_.load(std::memory_order_relaxed) != LifecycleState::STOPPED) { SS_LOGW(TAG, "add_player() called while started; role may not initialize correctly"); } this->player_ = @@ -487,7 +522,7 @@ PlayerRole& SendspinClient::add_player(PlayerRoleConfig config) { #ifdef SENDSPIN_ENABLE_CONTROLLER ControllerRole& SendspinClient::add_controller() { - if (this->started_) { + if (this->lifecycle_.load(std::memory_order_relaxed) != LifecycleState::STOPPED) { SS_LOGW(TAG, "add_controller() called while started"); } this->controller_ = std::make_unique(this); @@ -498,7 +533,7 @@ ControllerRole& SendspinClient::add_controller() { #ifdef SENDSPIN_ENABLE_METADATA MetadataRole& SendspinClient::add_metadata() { - if (this->started_) { + if (this->lifecycle_.load(std::memory_order_relaxed) != LifecycleState::STOPPED) { SS_LOGW(TAG, "add_metadata() called while started"); } this->metadata_ = std::make_unique(this); @@ -509,7 +544,7 @@ MetadataRole& SendspinClient::add_metadata() { #ifdef SENDSPIN_ENABLE_COLOR ColorRole& SendspinClient::add_color() { - if (this->started_) { + if (this->lifecycle_.load(std::memory_order_relaxed) != LifecycleState::STOPPED) { SS_LOGW(TAG, "add_color() called while started"); } this->color_ = std::make_unique(this); @@ -520,7 +555,7 @@ ColorRole& SendspinClient::add_color() { #ifdef SENDSPIN_ENABLE_ARTWORK ArtworkRole& SendspinClient::add_artwork(ArtworkRoleConfig config) { - if (this->started_) { + if (this->lifecycle_.load(std::memory_order_relaxed) != LifecycleState::STOPPED) { SS_LOGW(TAG, "add_artwork() called while started"); } this->artwork_ = std::make_unique(std::move(config), this); @@ -531,7 +566,7 @@ ArtworkRole& SendspinClient::add_artwork(ArtworkRoleConfig config) { #ifdef SENDSPIN_ENABLE_VISUALIZER VisualizerRole& SendspinClient::add_visualizer(VisualizerRoleConfig config) { - if (this->started_) { + if (this->lifecycle_.load(std::memory_order_relaxed) != LifecycleState::STOPPED) { SS_LOGW(TAG, "add_visualizer() called while started"); } this->visualizer_ = std::make_unique(std::move(config), this); @@ -595,7 +630,15 @@ void SendspinClient::send_text(const std::string& text) { } void SendspinClient::acquire_high_performance() { - if (this->high_performance_ref_count_.fetch_add(1) == 0 && this->listener_) { + if (this->high_performance_ref_count_.fetch_add(1) != 0) { + return; + } + // A release recorded but not yet delivered is cancelled by this re-acquire: the listener + // still holds high performance from its earlier request, so it gets neither callback. + if (this->high_performance_release_pending_.exchange(false)) { + return; + } + if (this->listener_) { this->listener_->on_request_high_performance(); } } @@ -606,14 +649,26 @@ void SendspinClient::release_high_performance() { uint8_t count = this->high_performance_ref_count_.load(); while (count != 0) { if (this->high_performance_ref_count_.compare_exchange_weak(count, count - 1)) { - if (count == 1 && this->listener_) { - this->listener_->on_release_high_performance(); + if (count == 1) { + // Recorded, not delivered: this can run under conn_ptr_mutex_ (drop_connection -> + // cleanup_connection_state -> here) and the listener may call back into the + // manager. drain_inbox() delivers it lock-free on the main loop. + this->high_performance_release_pending_.store(true); } return; } } } +void SendspinClient::deliver_pending_high_performance_release() { + if (!this->high_performance_release_pending_.exchange(false)) { + return; + } + if (this->listener_) { + this->listener_->on_release_high_performance(); + } +} + // ============================================================================ // Private helpers // ============================================================================ From c54298a0fc5b6e67b48bdf7bcd1156ca2f8a8f0f Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Thu, 10 Sep 2026 12:20:58 -0400 Subject: [PATCH 05/11] Share the loopback test scaffolding and cover the rollback and release paths test_client_lifecycle.cpp had copied server_url, make_config, TestNetworkProvider, the pump helpers, and FakeServer from test_connection_lifecycle.cpp. They now live in tests/test_support.h, with one FakeServer whose options cover both files (answer_time added). New tests: FailedRoleStartRollsBackAndRetryStartsClean makes a later role fail (a visualizer whose ring cannot be created) after the player started, then re-adds a working role and streams audio; without the rollback join the retry fails because SyncTask::start() refuses a running thread. ReleaseCallbackMayReenterTheManager drops a peer mid time-burst and calls disconnect() and connect_to() from on_release_high_performance(); on the inline-callback code it deadlocks. CallbackDuringStopCannotRecurse now also asserts the clear callback sees empty group state and that disconnect() and connect_to() are ignored from it. --- tests/test_client_lifecycle.cpp | 258 ++++++++++++---------------- tests/test_connection_lifecycle.cpp | 147 +--------------- tests/test_support.h | 211 +++++++++++++++++++++++ 3 files changed, 331 insertions(+), 285 deletions(-) create mode 100644 tests/test_support.h diff --git a/tests/test_client_lifecycle.cpp b/tests/test_client_lifecycle.cpp index 4bfc1d4a..c9b8b215 100644 --- a/tests/test_client_lifecycle.cpp +++ b/tests/test_client_lifecycle.cpp @@ -26,9 +26,10 @@ #include "sendspin/config.h" #include "sendspin/metadata_role.h" #include "sendspin/player_role.h" +#include "sendspin/visualizer_role.h" +#include "test_support.h" #include -#include #include #include @@ -45,7 +46,8 @@ #include #include -using namespace sendspin; // NOLINT(google-build-using-namespace): test-local convenience +using namespace sendspin; // NOLINT(google-build-using-namespace): test-local convenience +using namespace sendspin::test; // NOLINT(google-build-using-namespace): shared loopback scaffolding namespace { @@ -56,47 +58,8 @@ constexpr uint16_t NURSERY_GOODBYE_TEST_PORT = 18992; constexpr uint16_t STREAM_TEST_PORT = 18993; constexpr uint16_t CALLBACK_TEST_PORT = 18994; constexpr uint16_t DESTRUCTOR_TEST_PORT = 18995; - -std::string server_url(uint16_t port) { - return "ws://127.0.0.1:" + std::to_string(port) + "/sendspin"; -} - -SendspinClientConfig make_config(uint16_t port) { - SendspinClientConfig config; - config.client_id = "lifecycle-test-client"; - config.name = "Lifecycle Test Client"; - config.server_port = port; - return config; -} - -class TestNetworkProvider : public SendspinNetworkProvider { -public: - bool is_network_ready() override { - return true; - } -}; - -// Pumps client.loop() until pred() is true. No timeout: a regression hangs here and the suite -// watchdog reports it. -void pump_until(SendspinClient& client, const std::function& pred) { - for (;;) { - client.loop(); - if (pred()) { - return; - } - std::this_thread::sleep_for(std::chrono::milliseconds(2)); - } -} - -// Pumps client.loop() for a fixed window. Only for "must not happen" checks: a window that is -// too short can miss a regression, never fail a correct run. -void pump_for(SendspinClient& client, int duration_ms) { - const auto deadline = std::chrono::steady_clock::now() + std::chrono::milliseconds(duration_ms); - while (std::chrono::steady_clock::now() < deadline) { - client.loop(); - std::this_thread::sleep_for(std::chrono::milliseconds(2)); - } -} +constexpr uint16_t ROLLBACK_TEST_PORT = 18996; +constexpr uint16_t HIGH_PERF_TEST_PORT = 18997; /// Reports whether anything is listening on the loopback port. bool port_accepts(uint16_t port) { @@ -113,103 +76,6 @@ bool port_accepts(uint16_t port) { return connected; } -/// Behavior knobs for FakeServer. -struct FakeServerOptions { - bool answer_hello{true}; ///< Reply to client/hello with server/hello (false: a peer that - ///< upgrades and then never establishes, so it stays in the nursery) -}; - -/// A minimal Sendspin server: an IXWebSocket client that connects to the SendspinClient's WS -/// server, answers client/hello with server/hello, answers client/time with a server/time whose -/// clock is the client's own (both sides read platform_time_us(), so the offset is ~0 and audio -/// timestamps mean what they say), and records the goodbye and close. -class FakeServer { -public: - FakeServer(const std::string& url, std::string server_id, FakeServerOptions options = {}) - : server_id_(std::move(server_id)) { - this->ws_.setUrl(url); - this->ws_.disableAutomaticReconnection(); - this->ws_.setOnMessageCallback([this, options](const ix::WebSocketMessagePtr& msg) { - if (msg->type == ix::WebSocketMessageType::Message) { - const std::string& text = msg->str; - if (text.find("client/hello") != std::string::npos) { - this->got_client_hello_.store(true); - if (options.answer_hello) { - this->ws_.send( - std::string(R"({"type":"server/hello","payload":{"server_id":")") + - this->server_id_ + - R"(","name":"Fake Server","version":1,"active_roles":["player"],)" + - R"("connection_reason":"discovery"}})"); - } - } else if (text.find("client/time") != std::string::npos) { - const auto pos = text.find("\"client_transmitted\":"); - if (pos != std::string::npos) { - const long long client_transmitted = - std::strtoll(text.c_str() + pos + 21, nullptr, 10); - const int64_t now = platform_time_us(); - this->ws_.send(std::string(R"({"type":"server/time","payload":{)") + - "\"client_transmitted\":" + - std::to_string(client_transmitted) + - ",\"server_received\":" + std::to_string(now) + - ",\"server_transmitted\":" + std::to_string(now) + "}}"); - } - } else if (text.find("client/goodbye") != std::string::npos) { - this->got_goodbye_.store(true); - } - } else if (msg->type == ix::WebSocketMessageType::Close || - msg->type == ix::WebSocketMessageType::Error) { - this->closed_.store(true); - } - }); - this->ws_.start(); - } - - ~FakeServer() { - this->ws_.stop(); - } - - void send_text(const std::string& text) { - this->ws_.send(text); - } - - /// Sends one player audio chunk: binary type 4, big-endian server timestamp, PCM payload. - void send_audio(int64_t timestamp_us, size_t payload_bytes) { - std::string frame; - frame.push_back(static_cast(4)); - for (int shift = 56; shift >= 0; shift -= 8) { - frame.push_back(static_cast((timestamp_us >> shift) & 0xFF)); - } - frame.append(payload_bytes, '\0'); - this->ws_.sendBinary(frame); - } - - bool closed() const { - return this->closed_.load(); - } - - bool got_client_hello() const { - return this->got_client_hello_.load(); - } - - bool got_goodbye() const { - return this->got_goodbye_.load(); - } - -private: - ix::WebSocket ws_; - std::string server_id_; - std::atomic closed_{false}; - std::atomic got_client_hello_{false}; - std::atomic got_goodbye_{false}; -}; - -/// Blocks until pred() is true without pumping the client (for checks on a stopped client). -void wait_until(const std::function& pred) { - while (!pred()) { - std::this_thread::sleep_for(std::chrono::milliseconds(2)); - } -} - /// Counts the player lifecycle callbacks and audio writes; the write itself is a sink. class CountingPlayerListener : public PlayerRoleListener { public: @@ -237,12 +103,17 @@ class ReentrantMetadataListener : public MetadataRoleListener { void on_metadata_clear() override { ++this->clears; this->started_during_clear = this->client_.is_started(); + this->group_had_state_during_clear = + this->client_.get_group_state().playback_state.has_value(); this->start_result_during_clear = this->client_.start(); - this->client_.stop(); // Must be ignored, not recurse + this->client_.stop(); // Must be ignored, not recurse + this->client_.disconnect(SendspinGoodbyeReason::SHUTDOWN); // Must be ignored + this->client_.connect_to("ws://127.0.0.1:1/sendspin"); // Must be ignored } int clears{0}; bool started_during_clear{true}; + bool group_had_state_during_clear{true}; bool start_result_during_clear{true}; private: @@ -408,7 +279,8 @@ TEST(ClientLifecycle, StopEndsTheStreamAndRestartPlaysAgain) { for (int cycle = 0; cycle < 2; ++cycle) { ASSERT_TRUE(client.start()); - FakeServer server(server_url(STREAM_TEST_PORT), "server-" + std::to_string(cycle)); + FakeServer server(server_url(STREAM_TEST_PORT), "server-" + std::to_string(cycle), + FakeServerOptions{.answer_time = true}); pump_until(client, [&] { return client.is_connected(); }); server.send_text(stream_start_pcm_json()); @@ -430,8 +302,9 @@ TEST(ClientLifecycle, StopEndsTheStreamAndRestartPlaysAgain) { } // A listener callback fired from inside stop() cannot re-enter the lifecycle: start() reports -// failure and starts nothing, stop() is ignored rather than recursing, and the client reads as -// stopped. Afterwards the client restarts normally. +// failure and starts nothing, stop()/disconnect()/connect_to() are ignored rather than recursing, +// and the client (its started flag and its group state) already reads as stopped. Afterwards the +// client restarts normally. TEST(ClientLifecycle, CallbackDuringStopCannotRecurse) { TestNetworkProvider network; SendspinClient client(make_config(CALLBACK_TEST_PORT)); @@ -443,11 +316,15 @@ TEST(ClientLifecycle, CallbackDuringStopCannotRecurse) { { FakeServer server(server_url(CALLBACK_TEST_PORT), "server-a"); pump_until(client, [&] { return client.is_connected(); }); + // Group state the callback must already see reset. + server.send_text(R"({"type":"group/update","payload":{"playback_state":"playing"}})"); + pump_until(client, [&] { return client.get_group_state().playback_state.has_value(); }); client.stop(); EXPECT_EQ(listener.clears, 1); EXPECT_FALSE(listener.started_during_clear); + EXPECT_FALSE(listener.group_had_state_during_clear); EXPECT_FALSE(listener.start_result_during_clear); EXPECT_FALSE(client.is_started()); wait_until([&] { return server.closed(); }); @@ -486,4 +363,97 @@ TEST(ClientLifecycle, DestructorGoodbyesPeersWithoutCallbacks) { delete server; } +// A role that fails to start part-way through start() rolls the roles before it back: here the +// player comes up and the visualizer (a ring too small to create) refuses, so start() reports +// failure and the client stays stopped. Replacing the broken role and starting again succeeds, +// which needs the first attempt to have joined the player's sync task: SyncTask::start() refuses +// a thread that is still running, so a rollback that skipped the join fails the retry too. +TEST(ClientLifecycle, FailedRoleStartRollsBackAndRetryStartsClean) { + TestNetworkProvider network; + CountingPlayerListener listener; + SendspinClient client(make_config(ROLLBACK_TEST_PORT)); + client.set_network_provider(&network); + client.add_player(make_player_config()).set_listener(&listener); + + VisualizerRoleConfig broken; + broken.support.types = {VisualizerDataType::LOUDNESS}; + broken.support.buffer_capacity = 0; // Below the ring's minimum: start() fails + broken.support.rate_max = 30; + client.add_visualizer(std::move(broken)); + + EXPECT_FALSE(client.start()); + EXPECT_FALSE(client.is_started()); + EXPECT_FALSE(client.start()); // Still broken, still refused, still not stuck half-started + + VisualizerRoleConfig working; + working.support.types = {VisualizerDataType::LOUDNESS}; + working.support.buffer_capacity = 4096; + working.support.rate_max = 30; + client.add_visualizer(std::move(working)); + + ASSERT_TRUE(client.start()); + FakeServer server(server_url(ROLLBACK_TEST_PORT), "server-a", + FakeServerOptions{.answer_time = true}); + pump_until(client, [&] { return client.is_connected(); }); + server.send_text(stream_start_pcm_json()); + pump_until(client, [&] { return listener.stream_starts == 1; }); + stream_audio_until(client, server, listener, 1); // The rolled-back player plays again + client.stop(); + EXPECT_EQ(listener.stream_ends, 1); +} + +/// Reacts to the high-performance release the way a consumer that reconfigures its radio might: +/// by calling back into the client. +class DisconnectingClientListener : public SendspinClientListener { +public: + explicit DisconnectingClientListener(SendspinClient& client) : client_(client) {} + + void on_request_high_performance() override { + ++this->requests; + } + void on_release_high_performance() override { + ++this->releases; + this->client_.disconnect(SendspinGoodbyeReason::SHUTDOWN); + this->client_.connect_to("ws://127.0.0.1:1/sendspin"); + } + + int requests{0}; + int releases{0}; + +private: + SendspinClient& client_; +}; + +// The high-performance hold taken for a time burst is released inside the connection-loss path, +// which runs under the manager lock. A listener that calls disconnect()/connect_to() from +// on_release_high_performance() must not deadlock: the release is delivered from the drain with +// no lock held. Before the fix this test hangs on the mutex and the suite watchdog reports it. +TEST(ClientLifecycle, ReleaseCallbackMayReenterTheManager) { + TestNetworkProvider network; + auto config = make_config(HIGH_PERF_TEST_PORT); + config.time_burst_interval_ms = 50; + SendspinClient client(std::move(config)); + client.set_network_provider(&network); + DisconnectingClientListener listener(client); + client.set_listener(&listener); + ASSERT_TRUE(client.start()); + + auto server = std::make_unique(server_url(HIGH_PERF_TEST_PORT), "server-a"); + pump_until(client, [&] { return client.is_connected(); }); + // The default FakeServer never answers client/time, so the burst stays open and the hold + // stays held until the connection is lost. + pump_until(client, [&] { return listener.requests == 1; }); + EXPECT_EQ(listener.releases, 0); + + server.reset(); // Peer goes away mid-burst: drop_connection releases the hold under the lock + pump_until(client, [&] { return listener.releases == 1; }); + EXPECT_FALSE(client.is_connected()); + + // Request and release stay paired across a reconnect. + FakeServer again(server_url(HIGH_PERF_TEST_PORT), "server-b"); + pump_until(client, [&] { return client.is_connected() && listener.requests == 2; }); + client.stop(); + EXPECT_EQ(listener.releases, 2); +} + } // namespace diff --git a/tests/test_connection_lifecycle.cpp b/tests/test_connection_lifecycle.cpp index 35c95215..44811c35 100644 --- a/tests/test_connection_lifecycle.cpp +++ b/tests/test_connection_lifecycle.cpp @@ -22,6 +22,7 @@ #include "connection_manager.h" // fnv1_hash, resolve_liveness_timeout_ms #include "sendspin/client.h" #include "sendspin/config.h" +#include "test_support.h" #include #include #include @@ -41,7 +42,8 @@ #include #include -using namespace sendspin; // NOLINT(google-build-using-namespace): test-local convenience +using namespace sendspin; // NOLINT(google-build-using-namespace): test-local convenience +using namespace sendspin::test; // NOLINT(google-build-using-namespace): shared loopback scaffolding namespace { @@ -60,35 +62,6 @@ constexpr uint16_t LIVENESS_TEST_PORT = 18983; constexpr uint16_t LIVENESS_CONTROL_PORT = 18984; constexpr uint16_t LIVENESS_DISABLED_PORT = 18985; -std::string server_url(uint16_t port) { - return "ws://127.0.0.1:" + std::to_string(port) + "/sendspin"; -} - -std::string server_hello_json(const std::string& server_id, const std::string& reason) { - return std::string(R"({"type":"server/hello","payload":{"server_id":")") + server_id + - R"(","name":"Fake Server","version":1,"active_roles":["player"],)" + - R"("connection_reason":")" + reason + R"("}})"; -} - -// Any complete inbound frame counts for liveness, so the reply's timestamps need not be real. -constexpr const char* SERVER_TIME_JSON = - R"({"type":"server/time","payload":{"client_transmitted":0,"server_received":1000,"server_transmitted":1001}})"; - -SendspinClientConfig make_config(uint16_t port) { - SendspinClientConfig config; - config.client_id = "lifecycle-test-client"; - config.name = "Lifecycle Test Client"; - config.server_port = port; - return config; -} - -class TestNetworkProvider : public SendspinNetworkProvider { -public: - bool is_network_ready() override { - return true; - } -}; - class TestPersistenceProvider : public SendspinPersistenceProvider { public: explicit TestPersistenceProvider(uint32_t hash) : hash_(hash) {} @@ -101,27 +74,6 @@ class TestPersistenceProvider : public SendspinPersistenceProvider { uint32_t hash_; }; -// Pumps client.loop() until pred() is true. No timeout: a regression hangs here and the CTest -// TIMEOUT reports it. -void pump_until(SendspinClient& client, const std::function& pred) { - for (;;) { - client.loop(); - if (pred()) { - return; - } - std::this_thread::sleep_for(std::chrono::milliseconds(2)); - } -} - -// Pumps client.loop() for a fixed window. Only for spacing events or "must not happen" checks: -// a window that is too short can miss a regression, never fail a correct run. -void pump_for(SendspinClient& client, int duration_ms) { - const auto deadline = std::chrono::steady_clock::now() + std::chrono::milliseconds(duration_ms); - while (std::chrono::steady_clock::now() < deadline) { - client.loop(); - std::this_thread::sleep_for(std::chrono::milliseconds(2)); - } -} int connect_loopback(uint16_t port) { int fd = ::socket(AF_INET, SOCK_STREAM, 0); @@ -155,93 +107,6 @@ bool socket_closed(int fd) { } } -/// Behavior knobs for FakeServer. -struct FakeServerOptions { - bool hello_on_open{false}; ///< Send server/hello immediately on Open, before any - ///< client/hello arrives (a nonconforming peer) - bool answer_hello{true}; ///< Reply to client/hello with server/hello (false: mute peer - ///< that upgrades and then never establishes) - bool answer_time{false}; ///< Reply to client/time with server/time (a live server); false - ///< models a peer whose socket went silent after establishing -}; - -/// A minimal Sendspin "server": an IXWebSocket client that connects to the SendspinClient's WS -/// server (the server-initiated discovery direction) and answers client/hello with server/hello, -/// per the given options. -class FakeServer { -public: - FakeServer(const std::string& url, std::string server_id, FakeServerOptions options = {}) - : server_id_(std::move(server_id)) { - this->ws_.setUrl(url); - this->ws_.disableAutomaticReconnection(); - this->ws_.setOnMessageCallback([this, options](const ix::WebSocketMessagePtr& msg) { - if (msg->type == ix::WebSocketMessageType::Open) { - if (options.hello_on_open) { - this->ws_.send(server_hello_json(this->server_id_, "discovery")); - } - } else if (msg->type == ix::WebSocketMessageType::Message && - msg->str.find("client/hello") != std::string::npos) { - this->got_client_hello_.store(true); - if (options.answer_hello) { - this->ws_.send(server_hello_json(this->server_id_, "discovery")); - } - } else if (msg->type == ix::WebSocketMessageType::Message && - msg->str.find("client/goodbye") != std::string::npos) { - { - std::lock_guard lock(this->goodbye_mutex_); - this->goodbye_message_ = msg->str; - } - this->got_goodbye_.store(true); - } else if (msg->type == ix::WebSocketMessageType::Message && - msg->str.find("client/time") != std::string::npos) { - this->got_client_time_.store(true); - if (options.answer_time) { - this->ws_.send(SERVER_TIME_JSON); - } - } else if (msg->type == ix::WebSocketMessageType::Close || - msg->type == ix::WebSocketMessageType::Error) { - this->closed_.store(true); - } - }); - this->ws_.start(); - } - - ~FakeServer() { - this->ws_.stop(); - } - - bool closed() const { - return this->closed_.load(); - } - - bool got_client_hello() const { - return this->got_client_hello_.load(); - } - - bool got_goodbye() const { - return this->got_goodbye_.load(); - } - - std::string goodbye_message() const { - std::lock_guard lock(this->goodbye_mutex_); - return this->goodbye_message_; - } - - bool got_client_time() const { - return this->got_client_time_.load(); - } - -private: - ix::WebSocket ws_; - std::string server_id_; - mutable std::mutex goodbye_mutex_; - std::string goodbye_message_; - std::atomic closed_{false}; - std::atomic got_client_hello_{false}; - std::atomic got_goodbye_{false}; - std::atomic got_client_time_{false}; -}; - /// TCP relay that accepts one connection, sits on it without reading for delay_ms (the peer's /// WebSocket upgrade request waits in the kernel buffer), then connects to the backend and pumps /// bytes both ways. Simulates a slow network path in front of a real Sendspin server. @@ -644,7 +509,7 @@ TEST(ConnectionLifecycle, SilentEstablishedPeerIsDropped) { config.liveness_timeout_ms = 300; SendspinClient client(config); client.set_network_provider(&network); - ASSERT_TRUE(client.start_server()); + ASSERT_TRUE(client.start()); client.loop(); // First tick binds the WS server FakeServer silent(server_url(LIVENESS_TEST_PORT), "server-silent", {.answer_time = false}); @@ -668,7 +533,7 @@ TEST(ConnectionLifecycle, AnsweringPeerSurvivesLivenessTimeout) { config.liveness_timeout_ms = 300; SendspinClient client(config); client.set_network_provider(&network); - ASSERT_TRUE(client.start_server()); + ASSERT_TRUE(client.start()); client.loop(); // First tick binds the WS server FakeServer live(server_url(LIVENESS_CONTROL_PORT), "server-live", {.answer_time = true}); @@ -694,7 +559,7 @@ TEST(ConnectionLifecycle, DisabledLivenessKeepsSilentPeer) { config.liveness_timeout_ms = 0; SendspinClient client(config); client.set_network_provider(&network); - ASSERT_TRUE(client.start_server()); + ASSERT_TRUE(client.start()); client.loop(); // First tick binds the WS server FakeServer silent(server_url(LIVENESS_DISABLED_PORT), "server-silent", {.answer_time = false}); diff --git a/tests/test_support.h b/tests/test_support.h new file mode 100644 index 00000000..f395f583 --- /dev/null +++ b/tests/test_support.h @@ -0,0 +1,211 @@ +// Copyright 2026 Sendspin Contributors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +/// @file test_support.h +/// @brief Loopback scaffolding shared by the tests that drive a whole SendspinClient: an +/// IXWebSocket endpoint that plays the Sendspin server, a network provider that is always ready, +/// and pump helpers that tick client.loop() while waiting on a predicate. + +#pragma once + +#include "platform/time.h" +#include "sendspin/client.h" +#include "sendspin/config.h" + +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace sendspin::test { + +inline std::string server_url(uint16_t port) { + return "ws://127.0.0.1:" + std::to_string(port) + "/sendspin"; +} + +inline std::string server_hello_json(const std::string& server_id, const std::string& reason) { + return std::string(R"({"type":"server/hello","payload":{"server_id":")") + server_id + + R"(","name":"Fake Server","version":1,"active_roles":["player"],)" + + R"("connection_reason":")" + reason + R"("}})"; +} + +inline SendspinClientConfig make_config(uint16_t port) { + SendspinClientConfig config; + config.client_id = "lifecycle-test-client"; + config.name = "Lifecycle Test Client"; + config.server_port = port; + return config; +} + +class TestNetworkProvider : public SendspinNetworkProvider { +public: + bool is_network_ready() override { + return true; + } +}; + +/// Pumps client.loop() until pred() is true. No timeout: a regression hangs here and the suite +/// watchdog reports it. +inline void pump_until(SendspinClient& client, const std::function& pred) { + for (;;) { + client.loop(); + if (pred()) { + return; + } + std::this_thread::sleep_for(std::chrono::milliseconds(2)); + } +} + +/// Pumps client.loop() for a fixed window. Only for spacing events or "must not happen" checks: +/// a window that is too short can miss a regression, never fail a correct run. +inline void pump_for(SendspinClient& client, int duration_ms) { + const auto deadline = std::chrono::steady_clock::now() + std::chrono::milliseconds(duration_ms); + while (std::chrono::steady_clock::now() < deadline) { + client.loop(); + std::this_thread::sleep_for(std::chrono::milliseconds(2)); + } +} + +/// Blocks until pred() is true without pumping the client (for checks on a stopped client). +inline void wait_until(const std::function& pred) { + while (!pred()) { + std::this_thread::sleep_for(std::chrono::milliseconds(2)); + } +} + +/// Behavior knobs for FakeServer. +struct FakeServerOptions { + bool hello_on_open{false}; ///< Send server/hello immediately on Open, before any + ///< client/hello arrives (a nonconforming peer) + bool answer_hello{true}; ///< Reply to client/hello with server/hello (false: mute peer + ///< that upgrades and then never establishes) + bool answer_time{false}; ///< Reply to client/time with a server/time whose clock is the + ///< client's own (both sides read platform_time_us(), so the + ///< offset is ~0 and audio timestamps mean what they say) +}; + +/// A minimal Sendspin server: an IXWebSocket client that connects to the SendspinClient's WS +/// server (the server-initiated discovery direction), answers client/hello with server/hello per +/// the given options, and records the goodbye and close. +class FakeServer { +public: + FakeServer(const std::string& url, std::string server_id, FakeServerOptions options = {}) + : server_id_(std::move(server_id)) { + this->ws_.setUrl(url); + this->ws_.disableAutomaticReconnection(); + this->ws_.setOnMessageCallback([this, options](const ix::WebSocketMessagePtr& msg) { + if (msg->type == ix::WebSocketMessageType::Open) { + if (options.hello_on_open) { + this->ws_.send(server_hello_json(this->server_id_, "discovery")); + } + } else if (msg->type == ix::WebSocketMessageType::Message) { + const std::string& text = msg->str; + if (text.find("client/hello") != std::string::npos) { + this->got_client_hello_.store(true); + if (options.answer_hello) { + this->ws_.send(server_hello_json(this->server_id_, "discovery")); + } + } else if (text.find("client/time") != std::string::npos) { + this->got_client_time_.store(true); + if (options.answer_time) { + this->answer_time(text); + } + } else if (text.find("client/goodbye") != std::string::npos) { + { + std::lock_guard lock(this->goodbye_mutex_); + this->goodbye_message_ = text; + } + this->got_goodbye_.store(true); + } + } else if (msg->type == ix::WebSocketMessageType::Close || + msg->type == ix::WebSocketMessageType::Error) { + this->closed_.store(true); + } + }); + this->ws_.start(); + } + + ~FakeServer() { + this->ws_.stop(); + } + + void send_text(const std::string& text) { + this->ws_.send(text); + } + + /// Sends one player audio chunk: binary type 4, big-endian server timestamp, PCM payload. + void send_audio(int64_t timestamp_us, size_t payload_bytes) { + std::string frame; + frame.push_back(static_cast(4)); + for (int shift = 56; shift >= 0; shift -= 8) { + frame.push_back(static_cast((timestamp_us >> shift) & 0xFF)); + } + frame.append(payload_bytes, '\0'); + this->ws_.sendBinary(frame); + } + + bool closed() const { + return this->closed_.load(); + } + + bool got_client_hello() const { + return this->got_client_hello_.load(); + } + + bool got_goodbye() const { + return this->got_goodbye_.load(); + } + + std::string goodbye_message() const { + std::lock_guard lock(this->goodbye_mutex_); + return this->goodbye_message_; + } + + bool got_client_time() const { + return this->got_client_time_.load(); + } + +private: + void answer_time(const std::string& client_time_text) { + const auto pos = client_time_text.find("\"client_transmitted\":"); + if (pos == std::string::npos) { + return; + } + const long long client_transmitted = + std::strtoll(client_time_text.c_str() + pos + 21, nullptr, 10); + const int64_t now = platform_time_us(); + this->ws_.send(std::string(R"({"type":"server/time","payload":{)") + + "\"client_transmitted\":" + std::to_string(client_transmitted) + + ",\"server_received\":" + std::to_string(now) + + ",\"server_transmitted\":" + std::to_string(now) + "}}"); + } + + ix::WebSocket ws_; + std::string server_id_; + mutable std::mutex goodbye_mutex_; + std::string goodbye_message_; + std::atomic closed_{false}; + std::atomic got_client_hello_{false}; + std::atomic got_goodbye_{false}; + std::atomic got_client_time_{false}; +}; + +} // namespace sendspin::test From 686c045a4f9599b594661732b5a3cdf1289e124e Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Tue, 15 Sep 2026 12:35:34 -0400 Subject: [PATCH 06/11] Deliver the high-performance release inline under a listener contract The release callback can run inside the connection-loss path, which holds conn_ptr_mutex_. Instead of recording the release and delivering it from the next drain, document that on_request_high_performance() and on_release_high_performance() only toggle the platform networking mode and never call back into the client, and call the listener inline again. The pending flag, its cancel-on-reacquire rule, and the drain-time delivery are removed; the pairing test keeps its assertions with a listener that obeys the contract. --- docs/integration-guide.md | 2 +- docs/internals.md | 2 +- include/sendspin/client.h | 20 +++++++++----------- src/client.cpp | 28 +++------------------------- tests/test_client_lifecycle.cpp | 27 +++++++++------------------ 5 files changed, 23 insertions(+), 56 deletions(-) diff --git a/docs/integration-guide.md b/docs/integration-guide.md index b82fee22..f353f881 100644 --- a/docs/integration-guide.md +++ b/docs/integration-guide.md @@ -558,7 +558,7 @@ Restarting is `start()` again; start, stop, and start again can be repeated inde Listener callbacks fire from inside `stop()`, after every role and the group state have been reset, so a callback that reads the client through its getters sees the stopped state. One that calls `start()` gets `false` and starts nothing; one that calls `stop()`, `connect_to()`, or `disconnect()` is ignored. `is_started()` reads `false` throughout and is safe to call from any thread. Call `stop()` only from the main loop thread: from a role-thread callback it would join the calling thread. -`on_release_high_performance()` is delivered from `loop()` (or from inside `stop()`) rather than from wherever the last hold was released, so it is always safe to call `disconnect()` or `connect_to()` from that callback. +`on_request_high_performance()` and `on_release_high_performance()` can fire while the client holds an internal lock, so their bodies must only toggle the platform networking mode and must not call any client or role method. Destroying a running client performs the transport half of `stop()` (goodbye, bounded wait, close, join) but delivers no listener callback, so a consumer that destroys its listeners before the client is never called into. Call `stop()` first when the clear callbacks matter. diff --git a/docs/internals.md b/docs/internals.md index f5098c22..f2e949b0 100644 --- a/docs/internals.md +++ b/docs/internals.md @@ -499,7 +499,7 @@ The client destructor performs steps 1 and 2 only, so a consumer that destroyed ### High-performance release delivery -`release_high_performance()` never calls the listener inline. The last release can run inside `drop_connection()`, which holds `conn_ptr_mutex_` through `cleanup_connection_state()` (both the time-burst hold and the player's playback hold are released there), and a listener whose `on_release_high_performance()` reacts by calling `disconnect()` or `connect_to()` would re-lock the same non-recursive mutex on the same thread. The release is recorded in `high_performance_release_pending_` and delivered at the top of `drain_inbox()`, which every path (`loop()` and `stop()`) runs with no manager lock held. An `acquire_high_performance()` that lands before the delivery cancels it instead of issuing a second request, so the listener always sees request and release strictly alternate. +`release_high_performance()` calls the listener inline. The last release can run inside `drop_connection()`, which holds `conn_ptr_mutex_` through `cleanup_connection_state()` (both the time-burst hold and the player's playback hold are released there), so the listener contract for `on_request_high_performance()` / `on_release_high_performance()` is that the body toggles the platform's networking mode and nothing else: it must not call back into the client or a role. ### Graceful Disconnect diff --git a/include/sendspin/client.h b/include/sendspin/client.h index 891720ab..c20d2d26 100644 --- a/include/sendspin/client.h +++ b/include/sendspin/client.h @@ -68,9 +68,16 @@ class SendspinClientListener { /// @brief Called when the library needs high-performance networking (e.g., disable WiFi /// power saving) + /// + /// Toggle the platform's networking mode and return. This callback and its release can fire + /// while the client holds an internal lock (the last release runs inside the connection-loss + /// path), so the body must not call any SendspinClient or role method. virtual void on_request_high_performance() {} /// @brief Called when the library no longer needs high-performance networking + /// + /// Same contract as on_request_high_performance(): toggle the platform mode only, never call + /// back into the client. virtual void on_release_high_performance() {} }; @@ -453,10 +460,8 @@ class SendspinClient { /// @brief Releases a ref-counted high-performance networking request /// - /// The listener's on_release_high_performance() is not called inline: the last release can - /// run inside ConnectionManager::drop_connection(), which holds conn_ptr_mutex_, and a - /// listener that reacts by calling disconnect() or connect_to() would re-lock it on the same - /// thread. The release is recorded and delivered on the next drain with no lock held. + /// The last release calls the listener inline, possibly under conn_ptr_mutex_ (the + /// connection-loss path); the listener contract forbids calling back into the client there. void release_high_performance(); private: @@ -467,10 +472,6 @@ class SendspinClient { /// listener callbacks on the calling (main-loop) thread. Shared by loop() and stop(). void drain_inbox(); - /// @brief Delivers a high-performance release the counter recorded while a manager lock was - /// held (see release_high_performance()). Main-loop thread, no lock held. - void deliver_pending_high_performance_release(); - /// @brief Asks the artwork and visualizer threads to exit without joining them, so their /// exit overlaps the transport teardown. The player is excluded: its ring must keep a /// consumer until the network threads are gone (see stop()). @@ -570,9 +571,6 @@ class SendspinClient { // 8-bit fields bool high_performance_held_for_time_{false}; std::atomic high_performance_ref_count_{0}; - /// Set when the high-performance count reaches zero; the listener's release callback is - /// delivered from the main loop with no manager lock held (see release_high_performance()). - std::atomic high_performance_release_pending_{false}; /// Where the client is in its lifecycle. Written only by start()/stop() on the main loop; /// atomic so is_started() can be read from any thread. STOPPING covers the whole of stop(): /// start() is refused and stop()/connect_to()/disconnect() are ignored while it is set, so a diff --git a/src/client.cpp b/src/client.cpp index 710e738d..3f80db98 100644 --- a/src/client.cpp +++ b/src/client.cpp @@ -294,8 +294,6 @@ void SendspinClient::loop() { } void SendspinClient::drain_inbox() { - this->deliver_pending_high_performance_release(); - // Process deferred events: all state mutations and user callbacks happen here, on the main // loop thread, to avoid cross-thread data races. Two poll() snapshots gate the work below: // inbox_bits (here) gates only the event-ring drain immediately following it; slot_bits @@ -630,15 +628,7 @@ void SendspinClient::send_text(const std::string& text) { } void SendspinClient::acquire_high_performance() { - if (this->high_performance_ref_count_.fetch_add(1) != 0) { - return; - } - // A release recorded but not yet delivered is cancelled by this re-acquire: the listener - // still holds high performance from its earlier request, so it gets neither callback. - if (this->high_performance_release_pending_.exchange(false)) { - return; - } - if (this->listener_) { + if (this->high_performance_ref_count_.fetch_add(1) == 0 && this->listener_) { this->listener_->on_request_high_performance(); } } @@ -649,26 +639,14 @@ void SendspinClient::release_high_performance() { uint8_t count = this->high_performance_ref_count_.load(); while (count != 0) { if (this->high_performance_ref_count_.compare_exchange_weak(count, count - 1)) { - if (count == 1) { - // Recorded, not delivered: this can run under conn_ptr_mutex_ (drop_connection -> - // cleanup_connection_state -> here) and the listener may call back into the - // manager. drain_inbox() delivers it lock-free on the main loop. - this->high_performance_release_pending_.store(true); + if (count == 1 && this->listener_) { + this->listener_->on_release_high_performance(); } return; } } } -void SendspinClient::deliver_pending_high_performance_release() { - if (!this->high_performance_release_pending_.exchange(false)) { - return; - } - if (this->listener_) { - this->listener_->on_release_high_performance(); - } -} - // ============================================================================ // Private helpers // ============================================================================ diff --git a/tests/test_client_lifecycle.cpp b/tests/test_client_lifecycle.cpp index c9b8b215..55c746bf 100644 --- a/tests/test_client_lifecycle.cpp +++ b/tests/test_client_lifecycle.cpp @@ -402,39 +402,31 @@ TEST(ClientLifecycle, FailedRoleStartRollsBackAndRetryStartsClean) { EXPECT_EQ(listener.stream_ends, 1); } -/// Reacts to the high-performance release the way a consumer that reconfigures its radio might: -/// by calling back into the client. -class DisconnectingClientListener : public SendspinClientListener { +/// Counts high-performance requests and releases without touching the client, as the listener +/// contract requires. +class CountingClientListener : public SendspinClientListener { public: - explicit DisconnectingClientListener(SendspinClient& client) : client_(client) {} - void on_request_high_performance() override { ++this->requests; } void on_release_high_performance() override { ++this->releases; - this->client_.disconnect(SendspinGoodbyeReason::SHUTDOWN); - this->client_.connect_to("ws://127.0.0.1:1/sendspin"); } int requests{0}; int releases{0}; - -private: - SendspinClient& client_; }; -// The high-performance hold taken for a time burst is released inside the connection-loss path, -// which runs under the manager lock. A listener that calls disconnect()/connect_to() from -// on_release_high_performance() must not deadlock: the release is delivered from the drain with -// no lock held. Before the fix this test hangs on the mutex and the suite watchdog reports it. -TEST(ClientLifecycle, ReleaseCallbackMayReenterTheManager) { +// The high-performance hold taken for a time burst is released inside the connection-loss path +// and again by stop(); request and release stay paired across a peer loss, a reconnect, and the +// stop. +TEST(ClientLifecycle, HighPerformanceRequestAndReleaseStayPaired) { TestNetworkProvider network; auto config = make_config(HIGH_PERF_TEST_PORT); config.time_burst_interval_ms = 50; SendspinClient client(std::move(config)); client.set_network_provider(&network); - DisconnectingClientListener listener(client); + CountingClientListener listener; client.set_listener(&listener); ASSERT_TRUE(client.start()); @@ -445,11 +437,10 @@ TEST(ClientLifecycle, ReleaseCallbackMayReenterTheManager) { pump_until(client, [&] { return listener.requests == 1; }); EXPECT_EQ(listener.releases, 0); - server.reset(); // Peer goes away mid-burst: drop_connection releases the hold under the lock + server.reset(); // Peer goes away mid-burst: drop_connection releases the hold pump_until(client, [&] { return listener.releases == 1; }); EXPECT_FALSE(client.is_connected()); - // Request and release stay paired across a reconnect. FakeServer again(server_url(HIGH_PERF_TEST_PORT), "server-b"); pump_until(client, [&] { return client.is_connected() && listener.requests == 2; }); client.stop(); From 2fed2f15cb41d3b27f97cd5214087415c052c637 Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Tue, 15 Sep 2026 12:43:30 -0400 Subject: [PATCH 07/11] Tighten the lifecycle branch after review - accepting_ is a plain bool: every access is under conn_ptr_mutex_ and loop() never reads it, so it moves out of the lock-free-hint section. - ConnectionManager::start() configures the server object on first start only; the client config is immutable, so the per-restart re-application and its comment described a mutability that does not exist. - The host stop() bound is documented as the 3 s handshake timeout a raw never-upgraded socket can hold, not the 300 ms WebSocket close. - stop() and the destructor share close_transports() for the signal-then-close sequence. - ArtworkRole/VisualizerRole signal_stop() returns whether it signalled a running thread, and stop() uses that instead of repeating the guard. --- docs/integration-guide.md | 2 +- docs/internals.md | 7 ++++--- include/sendspin/client.h | 4 ++++ src/artwork_role.cpp | 8 ++++---- src/artwork_role_impl.h | 7 ++++--- src/client.cpp | 32 ++++++++++++++++++-------------- src/connection_manager.cpp | 20 +++++++++----------- src/connection_manager.h | 24 ++++++++++++------------ src/visualizer_role.cpp | 8 ++++---- src/visualizer_role_impl.h | 7 ++++--- 10 files changed, 64 insertions(+), 55 deletions(-) diff --git a/docs/integration-guide.md b/docs/integration-guide.md index f353f881..82798361 100644 --- a/docs/integration-guide.md +++ b/docs/integration-guide.md @@ -552,7 +552,7 @@ Restarting is `start()` again; start, stop, and start again can be repeated inde `stop()` may block, but the wait is bounded. Besides the goodbye bound it includes: -- The transports' own close. The host server waits up to 300 ms per connection for the WebSocket close handshake. The ESP server waits for the httpd task to exit, which polls at 100 ms and first finishes any queued send, which can take up to httpd's send timeout for a peer that has stopped reading. +- The transports' own close. The host server joins every accepted connection thread; a WebSocket peer completes its close handshake within about 300 ms, but a raw socket that connected and never completed the upgrade holds the join for the full 3 s handshake timeout. The ESP server waits for the httpd task to exit, which polls at 100 ms and first finishes any queued send, which can take up to httpd's send timeout for a peer that has stopped reading. - An outbound `connect_to()` connection's transport stop, which is synchronous (`esp_websocket_client_stop()` / `ix::WebSocket::stop()`). - A listener callback already running on a role thread: the join cannot interrupt it. `on_audio_write()` is bounded by its `timeout_ms`; `on_image_decode()` has no bound. diff --git a/docs/internals.md b/docs/internals.md index f2e949b0..e172a8c5 100644 --- a/docs/internals.md +++ b/docs/internals.md @@ -473,8 +473,9 @@ When a connection is lost (`on_connection_lost`): ├─ Outside the lock: conn->disconnect(SHUTDOWN, completion) on each, completion counted by a │ shared GoodbyeWait; wait up to GOODBYE_FLUSH_TIMEOUT_MS (50 ms) per goodbye for the count │ to reach zero (the ESP httpd worker hands the frames to lwIP one at a time) - ├─ ws_server_->stop() regardless (host: joins every connection thread, each bounded by - │ IXWebSocket's 300 ms close handshake; ESP: httpd_stop(), which runs queued sends first, + ├─ ws_server_->stop() regardless (host: joins every accepted connection thread, a WebSocket + │ peer within its ~300 ms close handshake and a raw never-upgraded socket within the 3 s + │ WS_HANDSHAKE_TIMEOUT_SECS; ESP: httpd_stop(), which runs queued sends first, │ then every session's close_fn and ctx free_fn, polling at 100 ms) └─ take_pending_events(): move the pending connected/disconnect event queues out under conn_mutex_; every moved-out shared_ptr is released outside the locks (an outbound @@ -491,7 +492,7 @@ When a connection is lost (`on_connection_lost`): The goodbye completion is best-effort: on ESP a session that closes before its queued worker runs, or whose `weak_ptr` no longer resolves, never reports, which is why the wait is bounded rather than exact. The `GoodbyeWait` record is held by `shared_ptr` and captured by value in each completion, so a completion that runs late on a transport thread touches nothing `stop()` owns. A peer delivered by the ws_server while admission is closed is rejected in `on_new_connection()` with a shutdown goodbye, the same shape as the nursery-full rejection. -`ConnectionManager::start()` re-applies the server port, connection budget, and control port from the client config on every call, so a restart listens with the values the client holds now rather than the ones captured at the first start. +`ConnectionManager::start()` creates and configures the server object on the first call only; the client config is immutable for the client's lifetime, so a restart reuses the object and its settings. Each threaded role's `start()` calls `EventFlags::clear_all()` before creating its thread rather than clearing a hand-listed set of bits: a command signalled between the previous join and the restart (`cleanup()` on a stopped role) would otherwise survive into the new thread's first wait, and a bit added to the role's enum later cannot be forgotten. diff --git a/include/sendspin/client.h b/include/sendspin/client.h index c20d2d26..77ede130 100644 --- a/include/sendspin/client.h +++ b/include/sendspin/client.h @@ -472,6 +472,10 @@ class SendspinClient { /// listener callbacks on the calling (main-loop) thread. Shared by loop() and stop(). void drain_inbox(); + /// @brief Signals the drain roles, then goodbyes and closes every transport, joining the + /// network threads. The shared first half of stop() and the destructor's teardown. + void close_transports(); + /// @brief Asks the artwork and visualizer threads to exit without joining them, so their /// exit overlaps the transport teardown. The player is excluded: its ring must keep a /// consumer until the network threads are gone (see stop()). diff --git a/src/artwork_role.cpp b/src/artwork_role.cpp index 165b1f2b..72e48ecf 100644 --- a/src/artwork_role.cpp +++ b/src/artwork_role.cpp @@ -112,22 +112,22 @@ bool ArtworkRole::Impl::start() { return true; } -void ArtworkRole::Impl::signal_stop() const { +bool ArtworkRole::Impl::signal_stop() const { if (!this->drain_task || !this->drain_task->drain_thread.joinable()) { - return; + return false; } // Set the flag before waking: the thread re-checks its command flags at the top of every // loop iteration, so this ordering guarantees it observes the stop as soon as the wake // pulls it out of its blocking queue receive. this->drain_task->event_flags.set(COMMAND_STOP); this->drain_task->notify_queue.wake_receiver(); + return true; } void ArtworkRole::Impl::stop() const { - if (!this->drain_task || !this->drain_task->drain_thread.joinable()) { + if (!this->signal_stop()) { return; } - this->signal_stop(); this->drain_task->drain_thread.join(); // Joined, so this is the queue's only consumer: discard notifications the old thread never diff --git a/src/artwork_role_impl.h b/src/artwork_role_impl.h index d9823d1e..1f979b0e 100644 --- a/src/artwork_role_impl.h +++ b/src/artwork_role_impl.h @@ -181,9 +181,10 @@ struct ArtworkRole::Impl { // Helpers // ======================================== - /// @brief Asks the decode thread to exit without waiting for it; stop() joins. No-op when - /// the thread is not running. Lets a caller overlap the thread's exit with other teardown. - void signal_stop() const; + /// @brief Asks the decode thread to exit without waiting for it; stop() joins. Lets a caller + /// overlap the thread's exit with other teardown. + /// @return true if a running thread was signalled, false if none was running. + bool signal_stop() const; void stop() const; void enqueue_stream_event(ArtworkEventType event) const; // Merges a single-slot display delta into the accumulated cross-thread update. Called under diff --git a/src/client.cpp b/src/client.cpp index 3f80db98..3dced602 100644 --- a/src/client.cpp +++ b/src/client.cpp @@ -85,8 +85,7 @@ SendspinClient::~SendspinClient() { // into from here. The role threads are joined by the role resets below, whose destructors // run the same stop() the explicit path would. if (this->lifecycle_.load(std::memory_order_relaxed) != LifecycleState::STOPPED) { - this->signal_drain_role_stops(); - this->connection_manager_->stop(SendspinGoodbyeReason::SHUTDOWN); + this->close_transports(); } // The network threads are gone (above, or never started), so the role threads are the only @@ -180,18 +179,9 @@ void SendspinClient::stop() { // cannot recurse into the teardown, restart the server, or admit a connection. this->lifecycle_.store(LifecycleState::STOPPING, std::memory_order_release); - // 1. Ask the artwork and visualizer threads to exit now, so a slow on_image_decode() or a - // parked drain overlaps the transport teardown instead of following it. Their inbound - // channels never block a network thread (a zero-timeout queue send, a bounded ring - // acquire), so they need no consumer while the transports close. The player's ring does: - // a network thread blocked on ring space (write_audio_chunk) resolves only while the sync - // task is alive, so the player is signalled in step 3, after the network threads are gone. - this->signal_drain_role_stops(); - - // 2. Transports: goodbye every peer, wait up to the flush bound, then close the server and - // every connection. This joins every network thread, so nothing reaches a role or the - // inbox from the network after it returns. - this->connection_manager_->stop(SendspinGoodbyeReason::SHUTDOWN); + // 1-2. Signal the drain roles, then goodbye and close every transport (see + // close_transports()). Nothing reaches a role or the inbox from the network after this. + this->close_transports(); // 3. Role threads. Each role discards its ring/queue content after its own join. this->stop_role_threads(); @@ -212,6 +202,20 @@ void SendspinClient::stop() { this->lifecycle_.store(LifecycleState::STOPPED, std::memory_order_release); } +void SendspinClient::close_transports() { + // 1. Ask the artwork and visualizer threads to exit now, so a slow on_image_decode() or a + // parked drain overlaps the transport teardown instead of following it. Their inbound + // channels never block a network thread (a zero-timeout queue send, a bounded ring + // acquire), so they need no consumer while the transports close. The player's ring does: + // a network thread blocked on ring space (write_audio_chunk) resolves only while the sync + // task is alive, so the player is joined by the caller after the network threads are gone. + this->signal_drain_role_stops(); + + // 2. Transports: goodbye every peer, wait up to the flush bound, then close the server and + // every connection. This joins every network thread. + this->connection_manager_->stop(SendspinGoodbyeReason::SHUTDOWN); +} + void SendspinClient::signal_drain_role_stops() { #ifdef SENDSPIN_ENABLE_VISUALIZER if (this->visualizer_) { diff --git a/src/connection_manager.cpp b/src/connection_manager.cpp index a9d0d758..7105a964 100644 --- a/src/connection_manager.cpp +++ b/src/connection_manager.cpp @@ -220,24 +220,22 @@ void ConnectionManager::disconnect(SendspinGoodbyeReason reason) { void ConnectionManager::start() { { std::lock_guard lock(this->conn_ptr_mutex_); - this->accepting_.store(true, std::memory_order_release); + this->accepting_ = true; } // A restart reuses the server object: stop() only stopped it, and loop() starts it again // once the network is ready. Retry immediately rather than honoring a backoff from before // the stop. this->ws_server_start_retry_time_us_ = 0; - const bool first_start = this->ws_server_ == nullptr; - if (first_start) { - this->ws_server_ = std::make_unique(); + if (this->ws_server_ != nullptr) { + return; } - // Applied on every start, not just the first: the transport is (re)created from these values - // when loop() starts it, so a restart listens with the config the client holds now. + + // First start: create the server object and configure it once. The config is immutable for + // the client's lifetime, so a restart reuses these values along with the object. + this->ws_server_ = std::make_unique(); this->ws_server_->set_port(this->client_->config_.server_port); this->ws_server_->set_max_connections(this->client_->config_.server_max_connections); this->ws_server_->set_ctrl_port(this->client_->config_.httpd_ctrl_port); - if (!first_start) { - return; - } // Graceful rejection needs transport headroom: the manager can hold one established inbound // connection plus NURSERY_CAPACITY unproven ones, and rejecting a surplus peer with a @@ -295,7 +293,7 @@ void ConnectionManager::stop(SendspinGoodbyeReason reason) { std::vector> to_goodbye; { std::lock_guard lock(this->conn_ptr_mutex_); - this->accepting_.store(false, std::memory_order_release); + this->accepting_ = false; if (this->current_connection_ != nullptr) { this->current_connection_->disable_message_dispatch(); to_goodbye.push_back(std::move(this->current_connection_)); @@ -667,7 +665,7 @@ void ConnectionManager::on_new_connection(std::shared_ptraccepting_.load(std::memory_order_acquire)) { + if (!this->accepting_) { // Delivered while stop() is tearing down (or before start()): the nursery is being // emptied, so the newcomer gets a goodbye and a close instead of a slot. Same shape // as the nursery-full rejection below. diff --git a/src/connection_manager.h b/src/connection_manager.h index 172956f3..2fdd3363 100644 --- a/src/connection_manager.h +++ b/src/connection_manager.h @@ -209,9 +209,9 @@ class ConnectionManager { /// @brief Opens admission and creates the WebSocket server on first use /// - /// Server configuration is read from the client's config and applied on every call, so a - /// restart picks up the current values. loop() starts the server once the network provider - /// reports ready. Main-loop thread only. + /// Server configuration is read from the client's config when the server object is created; + /// a restart reuses the object. loop() starts the server once the network provider reports + /// ready. Main-loop thread only. void start(); /// @brief Synchronous teardown: goodbyes every managed connection, waits up to @@ -220,10 +220,11 @@ class ConnectionManager { /// /// Closes admission first, so a peer delivered during the wait is rejected with a goodbye. /// Blocks on the transports' own teardown as well as the flush bound: the host server joins - /// its connection threads (each waits up to IXWebSocket's 300 ms close handshake), the ESP - /// server waits for the httpd task to exit, and an outbound connection's transport stop is - /// synchronous (esp_websocket_client_stop() / ix::WebSocket::stop()). Client-state cleanup is - /// the caller's job: this only detaches connections. Main-loop thread only. + /// every accepted connection thread, including a raw socket that never completed its + /// WebSocket upgrade, which can hold the join for the full WS_HANDSHAKE_TIMEOUT_SECS (3 s); + /// the ESP server waits for the httpd task to exit, and an outbound connection's transport + /// stop is synchronous (esp_websocket_client_stop() / ix::WebSocket::stop()). Client-state + /// cleanup is the caller's job: this only detaches connections. Main-loop thread only. /// @param reason The goodbye reason sent to every connected peer. void stop(SendspinGoodbyeReason reason); @@ -449,6 +450,10 @@ class ConnectionManager { // 8-bit fields bool has_last_played_server_{false}; + /// True between start() and stop(). Written and read only under conn_ptr_mutex_ (the read is + /// on_new_connection(), on the network thread), so a peer delivered after stop() closed + /// admission is rejected rather than admitted into a nursery stop() has already emptied. + bool accepting_{false}; // Atomic fields (lock-free hints for loop() tick gating; ground truth remains the // mutex-protected containers/pointer above -- see the "Tick cost" note on loop()) @@ -476,11 +481,6 @@ class ConnectionManager { /// queue_deferred_release()) and after the drain swap in flush_deferred_releases(). Lets /// flush_deferred_releases() early-return without locking when nothing is queued. std::atomic deferred_size_{0}; - - /// True between start() and stop(). Written under conn_ptr_mutex_ and read under it by - /// on_new_connection() (network thread), so a peer delivered after stop() closed admission is - /// rejected rather than admitted into a nursery stop() has already emptied. - std::atomic accepting_{false}; }; } // namespace sendspin diff --git a/src/visualizer_role.cpp b/src/visualizer_role.cpp index cb93b0bc..09d9e9fb 100644 --- a/src/visualizer_role.cpp +++ b/src/visualizer_role.cpp @@ -188,22 +188,22 @@ bool VisualizerRole::Impl::start() { return true; } -void VisualizerRole::Impl::signal_stop() const { +bool VisualizerRole::Impl::signal_stop() const { if (!this->drain_task || !this->drain_task->drain_thread.joinable()) { - return; + return false; } // Set the flag before waking: the thread re-checks its command flags at the top of every // loop iteration, so this ordering guarantees it observes the stop no matter which wait // it was parked in (display-time flags wait or ring buffer receive). this->drain_task->event_flags.set(COMMAND_STOP); this->drain_task->ring_buffer.wake_receiver(); + return true; } void VisualizerRole::Impl::stop() const { - if (!this->drain_task || !this->drain_task->drain_thread.joinable()) { + if (!this->signal_stop()) { return; } - this->signal_stop(); this->drain_task->drain_thread.join(); // Joined, so this is the ring's only consumer (the single-consumer contract the ring diff --git a/src/visualizer_role_impl.h b/src/visualizer_role_impl.h index 1912c535..fc8d6af4 100644 --- a/src/visualizer_role_impl.h +++ b/src/visualizer_role_impl.h @@ -111,9 +111,10 @@ struct VisualizerRole::Impl { // Internal helpers // ======================================== - /// @brief Asks the drain thread to exit without waiting for it; stop() joins. No-op when - /// the thread is not running. Lets a caller overlap the thread's exit with other teardown. - void signal_stop() const; + /// @brief Asks the drain thread to exit without waiting for it; stop() joins. Lets a caller + /// overlap the thread's exit with other teardown. + /// @return true if a running thread was signalled, false if none was running. + bool signal_stop() const; void stop() const; void flush_ring_buffer() const; void signal_clear_marker() const; From cb7f9a7e3d2af521a753038a78e252fc3d5ea481 Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Tue, 15 Sep 2026 13:46:16 -0400 Subject: [PATCH 08/11] Keep ownership of the ESP server object across httpd_stop() httpd_stop() releases the global user context and, with a null free function, calls plain free() on it. The context is the SendspinWsServer itself, still owned by the ConnectionManager, so stop() went on to lock a mutex inside freed memory. Register a no-op free function so httpd never takes ownership. Latent until this branch's stop()/restart path became the first caller of stop() on a running server. --- src/esp/ws_server.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/esp/ws_server.cpp b/src/esp/ws_server.cpp index 607fe8b6..65639300 100644 --- a/src/esp/ws_server.cpp +++ b/src/esp/ws_server.cpp @@ -68,8 +68,12 @@ bool SendspinWsServer::start(SendspinClient* client, bool task_stack_in_psram, config.max_open_sockets = this->max_connections_; config.open_fn = SendspinWsServer::open_callback; config.close_fn = SendspinWsServer::close_callback; + // httpd_stop() releases the global user context: with a null free function it calls plain + // free() on the pointer (esp_http_server httpd_main.c), which would free this object while + // the ConnectionManager still owns it and stop() still runs on it. A no-op free function + // keeps ownership here. config.global_user_ctx = (void*)this; - config.global_user_ctx_free_fn = nullptr; + config.global_user_ctx_free_fn = [](void* /*ctx*/) {}; // Use the configured ctrl_port, or fall back to ESP_HTTPD_DEF_CTRL_PORT + 1 to avoid // conflict with the web_server component config.ctrl_port = (this->ctrl_port_ != 0) ? this->ctrl_port_ From 1d0bc104b8a506c3d8290183eb6d96df742a20c5 Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Tue, 15 Sep 2026 13:51:43 -0400 Subject: [PATCH 09/11] Plan the start_server() alias removal for v0.9.0 Removing the alias is a breaking change, so it lands with a minor version bump. --- include/sendspin/client.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/sendspin/client.h b/include/sendspin/client.h index 77ede130..e192c79f 100644 --- a/include/sendspin/client.h +++ b/include/sendspin/client.h @@ -250,7 +250,7 @@ class SendspinClient { /// @brief Starts the client /// @deprecated Use start(). Kept as an alias for existing consumers; removal is planned for - /// v0.8.0. + /// v0.9.0. /// @return See start(). [[deprecated("Use start()")]] bool start_server() { return this->start(); From de84989a8a676dcf43913f0f714cb4b436269f61 Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Tue, 15 Sep 2026 14:37:38 -0400 Subject: [PATCH 10/11] Address the Copilot review on the lifecycle branch Narrow the destructor's callback claim: it dispatches no teardown or clear callback, but a role-thread callback can still run until the role is joined, so listeners must outlive the client. The destructor test keeps its listener alive accordingly, and the sync thread's docs say it lives for one started session rather than the client's lifetime. Cover the artwork and visualizer stop/start paths: a stop discards the frames its thread never took and a start clears the stop command. The visualizer test reads the ring after the stop through the role's private impl, so the lifecycle test file is compiled with -fno-access-control instead of adding a seam. --- docs/integration-guide.md | 2 +- docs/internals.md | 2 +- src/client.cpp | 8 +-- src/sync_task.cpp | 2 +- tests/CMakeLists.txt | 4 ++ tests/test_artwork_role.cpp | 75 ++++++++++++++++++++++ tests/test_client_lifecycle.cpp | 106 +++++++++++++++++++++++++++++--- tests/test_support.h | 13 ++-- 8 files changed, 193 insertions(+), 19 deletions(-) diff --git a/docs/integration-guide.md b/docs/integration-guide.md index 82798361..0c8f1870 100644 --- a/docs/integration-guide.md +++ b/docs/integration-guide.md @@ -560,7 +560,7 @@ Listener callbacks fire from inside `stop()`, after every role and the group sta `on_request_high_performance()` and `on_release_high_performance()` can fire while the client holds an internal lock, so their bodies must only toggle the platform networking mode and must not call any client or role method. -Destroying a running client performs the transport half of `stop()` (goodbye, bounded wait, close, join) but delivers no listener callback, so a consumer that destroys its listeners before the client is never called into. Call `stop()` first when the clear callbacks matter. +Destroying a running client performs the transport half of `stop()` (goodbye, bounded wait, close, join) and dispatches no teardown or clear callback. Role-thread callbacks (`on_audio_write()`, `on_image_decode()`, visualizer deliveries) can still run until the destructor joins their role, so listeners must outlive the client as described in Step 5. Call `stop()` first when the clear callbacks matter. ## Sending Commands diff --git a/docs/internals.md b/docs/internals.md index e172a8c5..bbea5db3 100644 --- a/docs/internals.md +++ b/docs/internals.md @@ -47,7 +47,7 @@ On host builds, `platform_configure_thread()` is a no-op; threads use OS default 1. `SyncTask::start()` configures the thread and spawns it. 2. The caller blocks until the thread reaches IDLE state (`TASK_IDLE` event flag) or exits early due to an allocation failure (`TASK_STOPPED`). -3. The thread runs a persistent outer loop for the lifetime of the client. +3. The thread runs a persistent outer loop for one started session, until `stop()`. 4. `SyncTask::stop()` sets `COMMAND_STOP`, wakes the ring buffer receive via `wake_receiver()`, and joins the thread; after the join it clears `TASK_RUNNING` (a stop mid-stream leaves it set, and the player's sync-idle gate must read a stopped task as idle) and resets the encoded ring buffer, so a later `start()` begins with an empty ring. Called from `PlayerRole::Impl::stop()` (`SendspinClient::stop()` and the client destructor) and from `SyncTask`'s destructor, which is triggered by `sync_task_.reset()` in `PlayerRole::Impl`'s destructor. 5. `SyncTask::start()` clears every command and state flag before spawning, so a restart after `stop()` inherits nothing from the previous thread. diff --git a/src/client.cpp b/src/client.cpp index 3dced602..e3816044 100644 --- a/src/client.cpp +++ b/src/client.cpp @@ -80,10 +80,10 @@ SendspinClient::SendspinClient(SendspinClientConfig config) SendspinClient::~SendspinClient() { // Transport-only teardown: goodbye and close every peer in the same order as stop(), but - // deliver no listener callback. A consumer that destroys its listeners before the client - // (the natural declaration order when a listener needs a role reference) is never called - // into from here. The role threads are joined by the role resets below, whose destructors - // run the same stop() the explicit path would. + // dispatch no teardown or clear callback (nothing reaches the inbox and no drain runs). A + // role-thread callback can still run until its role is joined by the resets below, whose + // destructors run the same stop() the explicit path would, so listeners must outlive the + // client. if (this->lifecycle_.load(std::memory_order_relaxed) != LifecycleState::STOPPED) { this->close_transports(); } diff --git a/src/sync_task.cpp b/src/sync_task.cpp index bdd365ed..c4902b73 100644 --- a/src/sync_task.cpp +++ b/src/sync_task.cpp @@ -825,7 +825,7 @@ void SyncTask::thread_entry(void* params) { sync_context.bytes_per_frame = sync_context.current_stream_info.frames_to_bytes(1); sync_context.decoder = std::make_unique(); - // === OUTER LOOP: persists for the lifetime of the client === + // === OUTER LOOP: persists for one started session, until stop() === while (!(this_task->event_flags_.get() & COMMAND_STOP)) { // --- IDLE STATE --- this_task->event_flags_.clear( diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index d4189a7e..cd67a861 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -36,6 +36,10 @@ add_executable(sendspin_tests test_client_lifecycle.cpp ) +# test_client_lifecycle.cpp checks what a role's stop() left in its ring, which nothing on the +# public surface reports; it reads the private members directly instead of adding a seam. +set_source_files_properties(test_client_lifecycle.cpp PROPERTIES COMPILE_OPTIONS -fno-access-control) + # Reach the library's private headers (protocol_messages.h, time_filter.h, ...). # The public include/ dir and ArduinoJson propagate transitively from `sendspin`. # Use CMAKE_CURRENT_SOURCE_DIR (not CMAKE_SOURCE_DIR) so the path stays correct even diff --git a/tests/test_artwork_role.cpp b/tests/test_artwork_role.cpp index 43b679f9..545fae3e 100644 --- a/tests/test_artwork_role.cpp +++ b/tests/test_artwork_role.cpp @@ -702,6 +702,81 @@ TEST(ArtworkFrameDoneGate, RestartKeepsPresentedGate) { EXPECT_EQ(listener.decode_marker_at(1), 'B'); } +// ============================================================================ +// Impl stop()/start(): the decode thread is joined and restarted between sessions +// ============================================================================ + +namespace { + +// A RecordingListener whose on_image_decode() parks until release(), so a test can hold the +// decode thread inside a callback while it queues more work behind it. +class BlockingListener : public RecordingListener { +public: + void on_image_decode(uint8_t slot, const uint8_t* data, size_t length, + SendspinImageFormat format) override { + RecordingListener::on_image_decode(slot, data, length, format); + std::unique_lock lock(this->gate_mutex_); + this->gate_cv_.wait(lock, [this] { return this->released_; }); + } + + void release() { + { + std::lock_guard lock(this->gate_mutex_); + this->released_ = true; + } + this->gate_cv_.notify_all(); + } + +private: + std::mutex gate_mutex_; + std::condition_variable gate_cv_; + bool released_{false}; +}; + +// Two ungated slots, so a frame on each is decoded without an ack. +ArtworkRoleConfig make_two_ungated_slot_config() { + ArtworkRoleConfig config; + config.preferred_formats.push_back( + {SendspinImageSource::ALBUM, SendspinImageFormat::JPEG, 100, 100, false}); + config.preferred_formats.push_back( + {SendspinImageSource::ARTIST, SendspinImageFormat::JPEG, 100, 100, false}); + return config; +} + +} // namespace + +// stop() joins the decode thread and discards the notifications it never took, and start() +// clears the stop command, so a restarted role decodes fresh frames without replaying the +// previous session's. The thread is held inside frame A's decode while frame B is queued behind +// it and the stop is signalled; on release it exits at its command check without taking B. The +// stream is deliberately not restarted after start(): a stream restart bumps the epoch that +// would make a replayed B stale on its own, and this test is about the queue reset. +TEST(ArtworkRestart, StopDiscardsQueuedFramesAndStartDecodesNewOnes) { + BlockingListener listener; + auto impl = make_impl(make_two_ungated_slot_config()); + impl->listener = &listener; + ASSERT_TRUE(impl->start()); + impl->handle_stream_start(ServerArtworkStreamObject{}); + + send_frame(*impl, 0, 'A'); + listener.wait_until([&] { return listener.decodes.size() >= 1; }); // Thread parked in A + send_frame(*impl, 1, 'B'); // Queued behind A + + ASSERT_TRUE(impl->signal_stop()); + listener.release(); + impl->stop(); + EXPECT_EQ(listener.decode_count(), 1U); + + ASSERT_TRUE(impl->start()); + // B was discarded with the old session, not replayed by the new thread. + EXPECT_TRUE(listener.never_within([&] { return listener.decodes.size() >= 2; }, NEGATIVE_WINDOW)); + + // The new thread decodes: the stop command did not survive the restart. + send_frame(*impl, 1, 'C'); + listener.wait_until([&] { return listener.decodes.size() >= 2; }); + EXPECT_EQ(listener.decode_marker_at(1), 'C'); +} + // ============================================================================ // Reentrant frame_done() from inside on_image_display() // ============================================================================ diff --git a/tests/test_client_lifecycle.cpp b/tests/test_client_lifecycle.cpp index 55c746bf..2636bd0d 100644 --- a/tests/test_client_lifecycle.cpp +++ b/tests/test_client_lifecycle.cpp @@ -22,12 +22,14 @@ #include "connection_manager.h" // GoodbyeWait, GOODBYE_FLUSH_TIMEOUT_MS #include "platform/time.h" +#include "protocol_messages.h" // SENDSPIN_BINARY_VISUALIZER_LOUDNESS #include "sendspin/client.h" #include "sendspin/config.h" #include "sendspin/metadata_role.h" #include "sendspin/player_role.h" #include "sendspin/visualizer_role.h" #include "test_support.h" +#include "visualizer_role_impl.h" // Ring state after stop(); private access, see tests/CMakeLists.txt #include @@ -60,6 +62,7 @@ constexpr uint16_t CALLBACK_TEST_PORT = 18994; constexpr uint16_t DESTRUCTOR_TEST_PORT = 18995; constexpr uint16_t ROLLBACK_TEST_PORT = 18996; constexpr uint16_t HIGH_PERF_TEST_PORT = 18997; +constexpr uint16_t VISUALIZER_TEST_PORT = 18998; /// Reports whether anything is listening on the loopback port. bool port_accepts(uint16_t port) { @@ -338,24 +341,22 @@ TEST(ClientLifecycle, CallbackDuringStopCannotRecurse) { EXPECT_EQ(listener.clears, 2); } -// Destroying a running client goodbyes its peer like stop() does, but delivers no listener -// callback: the listener here is released before the client, the natural order for a consumer -// that never called stop(), and the sanitizer turns any callback into a use-after-free. +// Destroying a running client goodbyes its peer like stop() does, but dispatches no clear +// callback: the listener outlives the client, as the role contract requires, and fails the test +// if the destructor calls into it. TEST(ClientLifecycle, DestructorGoodbyesPeersWithoutCallbacks) { TestNetworkProvider network; FakeServer* server = nullptr; - auto listener = std::make_unique(); + ForbiddenMetadataListener listener; { SendspinClient client(make_config(DESTRUCTOR_TEST_PORT)); client.set_network_provider(&network); - client.add_metadata().set_listener(listener.get()); + client.add_metadata().set_listener(&listener); ASSERT_TRUE(client.start()); server = new FakeServer(server_url(DESTRUCTOR_TEST_PORT), "server-a"); pump_until(client, [&] { return client.is_connected(); }); - - listener.reset(); - // Client destroyed here while established, with its listener already gone. + // Client destroyed here while established. } wait_until([&] { return server->closed(); }); @@ -402,6 +403,95 @@ TEST(ClientLifecycle, FailedRoleStartRollsBackAndRetryStartsClean) { EXPECT_EQ(listener.stream_ends, 1); } +/// Counts loudness deliveries; they fire on the visualizer drain thread. +class CountingVisualizerListener : public VisualizerRoleListener { +public: + void on_loudness(int64_t /*client_timestamp*/, uint16_t /*loudness*/) override { + this->loudness.fetch_add(1); + } + + std::atomic loudness{0}; +}; + +std::string stream_start_visualizer_json() { + return R"({"type":"stream/start","payload":{"visualizer":{"types":["loudness"],"rate_max":30}}})"; +} + +VisualizerRoleConfig make_visualizer_config() { + VisualizerRoleConfig config; + config.support.types = {VisualizerDataType::LOUDNESS}; + config.support.buffer_capacity = 4096; + config.support.rate_max = 30; + return config; +} + +// Waits for a fresh peer that answers time messages to be established and synced: the drain +// thread delivers nothing until the client is time synced. +void pump_until_synced(SendspinClient& client) { + pump_until(client, [&] { return client.is_connected() && client.is_time_synced(); }); +} + +// Pumps until pred() holds, sending one loudness frame per iteration stamped `lead_us` ahead of +// the current time (the drain thread drops a frame whose display time is well past). A frame +// can be lost to the ring's documented wake race right after a stream/start (the drain thread +// may take the clear marker as a stray entry and then discard up to a marker that is gone), +// which production shrugs off because the next frame follows; so does this. +void send_loudness_until(SendspinClient& client, FakeServer& server, int64_t lead_us, + const std::function& pred) { + pump_until(client, [&] { + if (pred()) { + return true; + } + server.send_binary(SENDSPIN_BINARY_VISUALIZER_LOUDNESS, platform_time_us() + lead_us, + std::string("\x00\x10", 2)); + std::this_thread::sleep_for(std::chrono::milliseconds(20)); + return false; + }); +} + +// stop() joins the visualizer drain thread and flushes the frames it had buffered, and start() +// clears the stop command, so a restart begins with an empty ring and a thread that delivers. +// The old frames are stamped far into the future, so the first session's thread parks on the +// first one with the rest buffered behind it when stop() runs; the ring is read directly after +// the stop because the restarted thread would silently drop leftovers before the new peer is +// time synced, and the new session's stream/start would discard them at its clear marker. +TEST(ClientLifecycle, StopFlushesBufferedVisualizerFramesAndRestartDelivers) { + constexpr int64_t OLD_FRAME_LEAD_US = 5 * 1000 * 1000; + + TestNetworkProvider network; + CountingVisualizerListener listener; + auto config = make_config(VISUALIZER_TEST_PORT); + config.time_burst_interval_ms = 100; // Sync promptly after each (re)connect + SendspinClient client(std::move(config)); + client.set_network_provider(&network); + client.add_visualizer(make_visualizer_config()).set_listener(&listener); + + ASSERT_TRUE(client.start()); + { + FakeServer server(server_url(VISUALIZER_TEST_PORT), "server-a", + FakeServerOptions{.answer_time = true}); + pump_until_synced(client); + server.send_text(stream_start_visualizer_json()); + // The thread holds the first frame while it waits for its display time; the ones behind + // it are the ring content stop() must discard. + auto& ring = client.visualizer()->impl_->drain_task->ring_buffer; + send_loudness_until(client, server, OLD_FRAME_LEAD_US, + [&] { return ring.items_waiting() >= 2; }); + client.stop(); + EXPECT_TRUE(ring.is_empty()); + wait_until([&] { return server.closed(); }); + } + EXPECT_EQ(listener.loudness.load(), 0U); + + ASSERT_TRUE(client.start()); + FakeServer server(server_url(VISUALIZER_TEST_PORT), "server-b", + FakeServerOptions{.answer_time = true}); + pump_until_synced(client); + server.send_text(stream_start_visualizer_json()); + send_loudness_until(client, server, 0, [&] { return listener.loudness.load() >= 1; }); + client.stop(); +} + /// Counts high-performance requests and releases without touching the client, as the listener /// contract requires. class CountingClientListener : public SendspinClientListener { diff --git a/tests/test_support.h b/tests/test_support.h index f395f583..90419a3f 100644 --- a/tests/test_support.h +++ b/tests/test_support.h @@ -151,17 +151,22 @@ class FakeServer { this->ws_.send(text); } - /// Sends one player audio chunk: binary type 4, big-endian server timestamp, PCM payload. - void send_audio(int64_t timestamp_us, size_t payload_bytes) { + /// Sends one binary message: type byte, big-endian server timestamp, then the payload. + void send_binary(uint8_t binary_type, int64_t timestamp_us, const std::string& payload) { std::string frame; - frame.push_back(static_cast(4)); + frame.push_back(static_cast(binary_type)); for (int shift = 56; shift >= 0; shift -= 8) { frame.push_back(static_cast((timestamp_us >> shift) & 0xFF)); } - frame.append(payload_bytes, '\0'); + frame.append(payload); this->ws_.sendBinary(frame); } + /// Sends one player audio chunk: binary type 4 with a zeroed PCM payload. + void send_audio(int64_t timestamp_us, size_t payload_bytes) { + this->send_binary(4, timestamp_us, std::string(payload_bytes, '\0')); + } + bool closed() const { return this->closed_.load(); } From c726d2e6f129f0adcef8b12b0feecc03b00a23c0 Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Tue, 15 Sep 2026 14:55:22 -0400 Subject: [PATCH 11/11] Release every high-performance hold when a running client is destroyed The release sites for the time-burst and playback holds live in the connection cleanup, which the destructor does not run, so a client destroyed mid-burst or mid-playback left the platform in high-performance mode. --- src/client.cpp | 6 ++++++ tests/test_client_lifecycle.cpp | 25 ++++++++++++++++++++++++- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/src/client.cpp b/src/client.cpp index e3816044..e5f77d07 100644 --- a/src/client.cpp +++ b/src/client.cpp @@ -86,6 +86,12 @@ SendspinClient::~SendspinClient() { // client. if (this->lifecycle_.load(std::memory_order_relaxed) != LifecycleState::STOPPED) { this->close_transports(); + // Every high-performance hold ends with the client. The release sites for the time hold + // (cleanup_connection_state()) and the playback hold (the player's cleanup()) do not run + // here, and nothing can acquire once the network threads are gone. + while (this->high_performance_ref_count_.load() > 0) { + this->release_high_performance(); + } } // The network threads are gone (above, or never started), so the role threads are the only diff --git a/tests/test_client_lifecycle.cpp b/tests/test_client_lifecycle.cpp index 2636bd0d..f7bdbe9c 100644 --- a/tests/test_client_lifecycle.cpp +++ b/tests/test_client_lifecycle.cpp @@ -54,7 +54,7 @@ using namespace sendspin::test; // NOLINT(google-build-using-namespace): shared namespace { // Distinct ports per test so a lingering socket from one scenario cannot bleed into the next -// (and into test_connection_lifecycle.cpp, which uses 18941-18982). +// (and into test_connection_lifecycle.cpp, which uses 18941-18985). constexpr uint16_t RESTART_TEST_PORT = 18991; constexpr uint16_t NURSERY_GOODBYE_TEST_PORT = 18992; constexpr uint16_t STREAM_TEST_PORT = 18993; @@ -63,6 +63,7 @@ constexpr uint16_t DESTRUCTOR_TEST_PORT = 18995; constexpr uint16_t ROLLBACK_TEST_PORT = 18996; constexpr uint16_t HIGH_PERF_TEST_PORT = 18997; constexpr uint16_t VISUALIZER_TEST_PORT = 18998; +constexpr uint16_t DESTRUCTOR_HIGH_PERF_TEST_PORT = 18999; /// Reports whether anything is listening on the loopback port. bool port_accepts(uint16_t port) { @@ -537,4 +538,26 @@ TEST(ClientLifecycle, HighPerformanceRequestAndReleaseStayPaired) { EXPECT_EQ(listener.releases, 2); } +// Destroying a running client ends the hold too: the release sites stop() reaches through the +// connection cleanup do not run in the destructor, so it has to release on its own or the +// platform is left in high-performance mode after the client is gone. +TEST(ClientLifecycle, DestructorReleasesHighPerformanceHold) { + TestNetworkProvider network; + CountingClientListener listener; + { + auto config = make_config(DESTRUCTOR_HIGH_PERF_TEST_PORT); + config.time_burst_interval_ms = 50; + SendspinClient client(std::move(config)); + client.set_network_provider(&network); + client.set_listener(&listener); + ASSERT_TRUE(client.start()); + + FakeServer server(server_url(DESTRUCTOR_HIGH_PERF_TEST_PORT), "server-a"); + pump_until(client, [&] { return client.is_connected() && listener.requests == 1; }); + EXPECT_EQ(listener.releases, 0); + // Client destroyed here mid-burst, with the hold open. + } + EXPECT_EQ(listener.releases, 1); +} + } // namespace