Add a synchronous start/stop/restart lifecycle to SendspinClient - #124
Merged
Merged
Conversation
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.
…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.
…::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.
… 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.
…e 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.
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.
- 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.
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.
Removing the alias is a breaking change, so it lands with a minor version bump.
kahrendt
force-pushed
the
sync-lifecycle
branch
from
September 15, 2026 17:56
f8168dd to
1d0bc10
Compare
kahrendt
marked this pull request as ready for review
September 15, 2026 17:58
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Listener-lifetime documentation is unsafe and several critical teardown and restart branches lack focused coverage.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds synchronous, restartable lifecycle management to SendspinClient.
Changes:
- Adds
start(),stop(),is_started(), and deprecatedstart_server()compatibility. - Implements bounded transport teardown and restart-safe role cleanup.
- Updates examples, documentation, and lifecycle tests.
File summaries
| File | Description |
|---|---|
include/sendspin/client.h |
Defines the lifecycle API and contracts. |
include/sendspin/config.h |
Updates startup documentation. |
src/client.cpp |
Implements start, stop, rollback, and reset orchestration. |
src/connection_manager.h |
Declares connection shutdown and goodbye coordination. |
src/connection_manager.cpp |
Implements admission control and synchronous teardown. |
src/platform/event_flags.h |
Adds full event-bit reset support. |
src/sync_task.h |
Exposes restart-safe sync-task stopping. |
src/sync_task.cpp |
Resets flags, buffers, and borrowed entries. |
src/player_role.cpp |
Restarts and stops the sync task. |
src/player_role_impl.h |
Declares player stop support. |
src/artwork_role.cpp |
Adds restart-safe decode-thread teardown. |
src/artwork_role_impl.h |
Declares asynchronous stop signaling. |
src/visualizer_role.cpp |
Adds restart-safe drain-thread teardown. |
src/visualizer_role_impl.h |
Declares asynchronous stop signaling. |
src/esp/ws_server.cpp |
Preserves WebSocket server context ownership. |
tests/test_support.h |
Adds shared loopback test scaffolding. |
tests/test_client_lifecycle.cpp |
Exercises lifecycle, rollback, and callback behavior. |
tests/test_connection_lifecycle.cpp |
Migrates connection tests to start(). |
tests/test_client_teardown.cpp |
Migrates destructor coverage to start(). |
tests/CMakeLists.txt |
Registers lifecycle tests. |
examples/basic_client/main.cpp |
Uses synchronous start and stop. |
examples/tui_client/main.cpp |
Uses synchronous start and stop. |
docs/internals.md |
Documents lifecycle teardown internals. |
docs/integration-guide.md |
Documents consumer-facing restart behavior. |
Review details
- Files reviewed: 24/24 changed files
- Comments generated: 9
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Destructor cleanup can leave high-performance networking enabled, and lifecycle tests contain a server-start race.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/client.cpp:89
- Destroying a running client while a time burst or playback hold is active never balances
on_request_high_performance(): this path only closes transports, while the two release sites are incleanup_connection_state()andPlayerRole::Impl::cleanup(), neither of which runs during destruction. On ESP this can leave Wi-Fi power saving disabled after the client is gone. Please join the roles and release these holds during destructor teardown without draining the queued clear events; the listener is documented to outlive the client.
tests/test_client_lifecycle.cpp:57 - The referenced test file now uses ports through 18985 (
LIVENESS_DISABLED_PORT), so this range is stale.
- Files reviewed: 25/25 changed files
- Comments generated: 2
- Review effort level: Balanced
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a synchronous
start()/stop()/ restart lifecycle toSendspinClient.start_server()stays as a deprecated alias. Replaces #110.Behavior
start()starts the role threads and arms the WebSocket server (the server itself comes up on the firstloop()tick after the network provider reports ready). A role that fails to start rolls back the roles that did, so a corrected retry begins from the stopped state.stop()sendsclient/goodbye(shutdown) to every peer, waits a bounded time for the sends to complete (GOODBYE_FLUSH_TIMEOUT_MSper goodbye), closes the server and every connection, joins the role threads, resets connection and role state, and delivers the roles' clear callbacks before returning. Start, stop, start can be repeated.is_started()is readable from any thread and reads false for the whole ofstop().connect_to(),disconnect(), andloop()are no-ops on a stopped client;start()andstop()are refused from a callback fired insidestop().stop()and dispatches no teardown or clear callback. Role-thread callbacks can still run until the destructor joins their role, so listeners must outlive the client.Listener contract
on_request_high_performance()andon_release_high_performance()can fire while the client holds an internal lock. Their bodies must only toggle the platform networking mode and must not call back into the client or a role. This replaces an earlier deferred-delivery mechanism on this branch.Fixes
httpd_stop()callsfree()on the global user context when no free function is registered. The context is theSendspinWsServeritself, sostop()ran on freed memory. A no-op free function keeps ownership with the manager. Latent onmain; this branch is the first caller ofstop()on a running server. Verified on hardware.COMMAND_STOPand other event-group bits are cleared on every role start (EventFlags::clear_all()), each role discards its ring or queue contents after its own join,TASK_RUNNINGis cleared after the sync thread joins so the stop-timeon_stream_end()fires, andgroup_state_/ client state are reset for the next session.stop()so a peer delivered mid-teardown is rejected with a goodbye rather than admitted into a nursery being emptied.Other changes
ConnectionManager::init_server()becomesstart()/stop();take_pending_events()replaces three copies of the pending-event drain.tests/test_support.hholds the loopbackFakeServerand pump helpers shared by the connection and client lifecycle tests.start()/stop().stop()sequence indocs/internals.md, "Stopping and Restarting" indocs/integration-guide.md. The hoststop()bound is documented as up to the 3 s handshake timeout for a raw socket that never completed its upgrade.Coverage gaps
stop()resets the ring. This is only observable on the FreeRTOS ring: the host ring re-reads a received but unreturned item, soreset()reclaims it and no host test can fail on that line.ConnectionManagerrejects a peer delivered whilestop()is tearing down. There is no deterministic way to holdstop()open while a peer arrives, so this branch is untested.Verification
Host: ASan/UBSan and TSan suites, 149 tests each. ESP32: start/stop/start cycles with the server running and a connected peer, on hardware.