diff --git a/.claude/skills/house-patterns/SKILL.md b/.claude/skills/house-patterns/SKILL.md index dc455111..3a168d5b 100644 --- a/.claude/skills/house-patterns/SKILL.md +++ b/.claude/skills/house-patterns/SKILL.md @@ -69,8 +69,9 @@ the descriptions in `docs/internals.md`: `InboxSlot`; two non-main-loop threads communicate via `ShadowSlot`. - Payload validation and decoding on the consuming thread, never the transport thread. -- Callback dispatch tolerates re-entrant teardown (a listener may call back - into the client mid-callback). +- No callback calls `loop()`, `start()` or `stop()`, so callback dispatch + carries no re-entrancy guard; a new guard for a callback that does is a + finding. - Silent drops on bounded queues are findings; drop sites log a warning. ## Validation posture diff --git a/docs/conventions.md b/docs/conventions.md index de0a435e..32451fef 100644 --- a/docs/conventions.md +++ b/docs/conventions.md @@ -77,10 +77,10 @@ checklists in `.claude/skills/` apply these standards to a diff. than relying on the ring or the slot being reset: a teardown that leaves other producers running cannot reset it, and a drain can take a slot on either side of a teardown. -- Callback dispatch must tolerate re-entrant teardown: a listener callback may - call back into the client, except `loop()` (see `client.h`). See "Re-entrant - Teardown During Callback Dispatch" in `docs/internals.md` for the guard - patterns in use. +- No callback calls `loop()`, `start()` or `stop()` (see `client.h`), so no + teardown runs on the main loop beneath the drain that fired a callback, and + callback dispatch carries no re-entrancy guard. See "Callbacks During + Dispatch" in `docs/internals.md`. - A message handler on the protocol task does the connection work the message calls for in place (an activation's admissibility check, admission and role ownership, a pairing step, a `noise/handshake` re-handshake, the RAM commit diff --git a/docs/integration-guide.md b/docs/integration-guide.md index 6f41701d..b1b5cd06 100644 --- a/docs/integration-guide.md +++ b/docs/integration-guide.md @@ -829,7 +829,7 @@ Restarting is `start()` again; start, stop, and start again can be repeated inde A pairing attempt in flight is cut short the same way: `on_clear_pairing_code()` and `on_close_pairing_window()` fire from inside `stop()` for a prompt that was still showing, and every provider write still owed (a long-term record a `server/pair-finalize` had just committed, for example) is performed before `stop()` returns. The identity and record store survive the stop, so a restarted client keeps its `client_id`, its pairing token, and every record. -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. +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 `connect_to()` or `disconnect()` is ignored. `is_started()` reads `false` throughout and is safe to call from any thread. Call `start()` and `stop()` only from the main loop thread, and never from a callback (see [Thread Safety Summary](#thread-safety-summary)). `on_request_high_performance()` and `on_release_high_performance()` fire from the main loop with no internal lock held, and their bodies should only toggle the platform networking mode rather than calling back into the client or a role. A time burst waits for the request to have been delivered before it sends its first time message, so a main loop that stalls delays the next clock measurement rather than measuring it in power-save mode; a release is never delayed that way. @@ -1217,7 +1217,7 @@ The view forms hand the metadata, group, pairing and server text over as `std::s | `ServerInformationObject` | `ServerInformation` | | `SendspinClient::get_server_information()` | `SendspinClient::server_information()`, main loop only | -The fields and arguments keep their meaning; only the text type changes. A view handed to a callback is valid for the duration of the callback, and the views `group_state()` and `server_information()` return until control next returns to `loop()` (from inside a callback: until the callback returns) or until `stop()`: copy what is needed beyond that. A view, like a pairing token's array, is not guaranteed to be NUL-terminated: print it with `%.*s` (its size, then its data) or copy it into a `std::string` before passing it to an API that expects a C string. `server_information()` reads a copy `loop()` refreshes as the server changes, so it is main loop only and can read the previous server, or `nullopt`, for one `loop()` after `is_connected()` turns true: read it on each `loop()` rather than only when `is_connected()` rises; `get_server_information()` reads from any thread, and code off the main loop has no replacement and keeps calling it until the removal. Until the removal the library calls the new callback and then the deprecated one with the same content, so a listener overrides one of the two; one that overrides both gets both, the new one first. A `stop()` from the new callback ends the delivery there. +The fields and arguments keep their meaning; only the text type changes. A view handed to a callback is valid for the duration of the callback, and the views `group_state()` and `server_information()` return until control next returns to `loop()` (from inside a callback: until the callback returns) or until `stop()`: copy what is needed beyond that. A view, like a pairing token's array, is not guaranteed to be NUL-terminated: print it with `%.*s` (its size, then its data) or copy it into a `std::string` before passing it to an API that expects a C string. `server_information()` reads a copy `loop()` refreshes as the server changes, so it is main loop only and can read the previous server, or `nullopt`, for one `loop()` after `is_connected()` turns true: read it on each `loop()` rather than only when `is_connected()` rises; `get_server_information()` reads from any thread, and code off the main loop has no replacement and keeps calling it until the removal. Until the removal the library calls the new callback and then the deprecated one with the same content, so a listener overrides one of the two; one that overrides both gets both, the new one first. ## Thread Safety Summary @@ -1236,7 +1236,7 @@ Most listener callbacks fire on the main loop thread (the thread calling `client | `SendspinClientConfig::message_trace` (built with `SENDSPIN_ENABLE_MESSAGE_TRACE`) | Protocol task | | All other listener methods | Main loop thread | -Never call `loop()` from a listener, role or provider callback. A callback on the main loop already runs inside one of the client's own calls (`loop()`, `start()`, `stop()` or its destructor), and `loop()` does not support being re-entered; a callback the table lists on another thread is off the main loop, where `loop()` never runs. +Never call `loop()`, `start()` or `stop()` from a listener, role or provider callback. A callback on the main loop already runs inside one of the client's own calls (`loop()`, `start()`, `stop()` or its destructor), none of which supports being re-entered; a callback the table lists on another thread is off the main loop, where these calls never run. To stop or restart the client in response to a callback, record the request in the callback and act on it from the main loop between two `loop()` calls. `PlayerRole::notify_audio_played()` is thread-safe and is designed to be called from an audio output callback thread. diff --git a/docs/internals.md b/docs/internals.md index 66ecd31b..1d963142 100644 --- a/docs/internals.md +++ b/docs/internals.md @@ -200,7 +200,7 @@ PLAYER_STREAM ring events → on_stream_ring_event() → awaiting_sync_idle_even │ Sync task idle → fire on_stream_end() if a stream is open, continue │ └─ STREAM_START: - Take stream_params_slot (applied if its stamp is current) + Take stream_params_slot only if stamped with the START's generation Mark stream active, fire on_stream_start() Acknowledge the event's ordinal to the sync task ``` @@ -227,14 +227,9 @@ When a connection leaves the admitted array, `detach_inbound()` runs first. It d The source's `cleanup()` closes the stream gate without a message and clears the authorization; it recalls nothing, since the source holds no inbound ring item. The player's, the visualizer's and the artwork role's `cleanup()` also return what the sync task, the visualizer drain thread or the artwork decode thread has not taken, right after the generation bump and on the thread that hands the items over (`InboundConsumer::recall()`), so every item recalled carries an older stamp; a consumer that takes such an item between the bump and the recall returns it, by its stamp, without acting on it. The visualizer's `cleanup()` also moves its boundary sequence on (see the `stream/end` row under [Dispatch](#dispatch-protocol-task)), so its drain thread returns a frame it holds for its delivery time. The artwork decode thread, woken by its `cleanup()`, also drops the images it assembles or parks once it sees the generation move on. A role removed by a `server/activate` takes the same path. In `stop()` the role threads are joined and their lists returned and unbound (`InboundConsumer::unbind()`) before the main loop runs the roles' `cleanup()`, whose recall then finds no ring bound. -### Re-entrant Teardown During Callback Dispatch +### Callbacks During Dispatch -Requests a listener callback makes (`connect_to()`, `disconnect()`, ...) are queued or posted to the protocol task, so no callback tears a connection down under the drain that fired it. `stop()` is the exception: called from a callback, it joins the protocol task, tears every role down on the main loop and runs a drain of its own beneath the one that fired the callback, and a `start()` that follows it in the same callback begins a new run. No other drain runs beneath one, since a callback never calls `loop()` (`client.h`). Two generation counters keep the outer drain from acting on what it already took: - -- `drain_generation` (on the client's `EventState`) is bumped by `stop()` before its teardown. The outer drain snapshots it once and stops as soon as it changes: the event ring between events, the pairing-note dispatch between notes, and the role drains and the group update before each starts. -- Each role's `cleanup_generation` is bumped by its `cleanup()` and stamped onto every event and slot payload it queues afterwards; the ring drain drops stream events whose stamp no longer matches (`event_is_current()`). Inside a role drain the same counter catches a callback that re-entered teardown: the player checks it after each server-command callback and around each `on_stream_start()` and `on_stream_end()`, metadata and color by ending their drain once the teardown's catch-up has forgotten the pending deadline (`ScheduledStateSlot::pending_at()`), and artwork after each display or clear, and each abandons the rest of its work when it moved on. - -A state handed to the listener in two forms, the view and the deprecated `std::string` copy, is not handed over in the second once the first re-entered teardown: the group update checks the drain generation between `on_group_state()` and `on_group_update()`, the pairing-note dispatch between each text-carrying callback and its deprecated form (`on_pairing_attempt()` and `on_pairing_started()`, ...), and the metadata role checks between `on_track_metadata()` and `on_metadata()` whether its `TeardownTracker` completed a teardown, which on the main loop only `stop()` can do inside a callback. +No listener, role or provider callback calls `loop()`, `start()` or `stop()` (`client.h`), and the requests a callback may make (`connect_to()`, `disconnect()`, a controller command, ...) are queued or posted to the protocol task. So no drain runs beneath another, and no role's main-loop half runs while one of its callbacks is on the stack: each drain delivers everything it took. A teardown the protocol task runs during a callback is caught up, through the stamps described under [Cleanup](#cleanup), the next time a drain reaches that role, later in the same drain or in the next one, always after the callback returns. A payload a drain takes after one of its callbacks has run is taken only under its own stamp, so one a later teardown's next connection wrote meanwhile stays pending: the player takes its stream parameters with the STREAM_START's stamp (`GenerationSlot::take_stamped()`). ## Message Flow @@ -415,9 +410,8 @@ Each getter reads the slot once, so a caller that checks `is_time_synced()` and `SendspinClient::stop()` is synchronous and ordered so that every producer is gone before any state is reset: ```api -0. lifecycle_ = STOPPING (loop() becomes a no-op; start/stop and every request that queues a - command or posts a request are refused or ignored, so a callback fired below cannot recurse - into the teardown) +0. lifecycle_ = STOPPING (loop() becomes a no-op, and every request that queues a command or + posts a request is ignored, from another thread or from a callback fired below) 1. Signal the visualizer and artwork threads to stop, without joining. The player is not signalled yet: it keeps returning the ring items it plays, so a transport waiting for ring space is not parked behind a stopped consumer @@ -434,9 +428,8 @@ Each getter reads the slot once, so a caller that checks `is_time_synced()` and connecting stays parked); then drop any command still queued and any request still posted 5. Join all role threads; each then returns its items to the ring, and the emptied inbound ring is released; the source destroys its outbound ring, every item back since step 4 -6. Bump drain_generation, cleanup_connection_state() for every role (the protocol-task halves - run here, on the main loop), then queue the pairing-UI dismissals the step 3 snapshot calls - for +6. cleanup_connection_state() for every role (the protocol-task halves run here, on the main + loop), then queue the pairing-UI dismissals the step 3 snapshot calls for 7. drain_inbox() runs each role's main-loop half (its clear; the player's on_stream_end(), the source's on_streaming_stopped()), the high-performance releases the shutdown pass queued, the owed provider writes, and the pairing notes step 6 queued, each exactly once: a dismissal step 6 queues beside one an @@ -472,7 +465,6 @@ A metadata state's text, only the fields `MetadataRoleConfig` keeps, each cut to | What | When | |------|------| | The pairing notes for the main loop beyond the room both of their buffers keep | A batch of more than `PAIRING_NOTE_CAPACITY` notes between two drains, on the protocol task; the grown room is kept | -| Another drain buffer for the pairing notes, with room for `PAIRING_NOTE_CAPACITY` | A pairing listener callback that calls `stop()` while a prompt shows: the drain `stop()` runs finds the buffer lent to the dispatch that called it, and reserves one before it takes the dismissals, on the main loop | | The player's stream events held for the sync task's idle | Events beyond the list's kept capacity, on the main loop | The `std::function`s built after `start()` allocate nothing because each capture is one pointer, which fits both standard libraries' small buffer: the Noise transport's frame sinks, set as each connection is constructed; the frame write hook a time send builds, which ESP copies into each send; and the handshake's send function, built for each handshake frame. A change to one of these captures is checked against that buffer, since a callable that does not fit it allocates on every copy. diff --git a/include/sendspin/client.h b/include/sendspin/client.h index c1e840eb..33f4633d 100644 --- a/include/sendspin/client.h +++ b/include/sendspin/client.h @@ -363,7 +363,8 @@ class SendspinClient { /// start, the roles that did start are stopped again so a corrected retry begins from the /// stopped state. A rejected configured pairing secret (SendspinClientConfig::pairing_psk or /// static_pairing_code) fails every start() of this instance, since the config is fixed at - /// construction. Main-loop thread only. + /// construction. Main-loop thread only, and never from a listener, role or provider callback + /// (docs/integration-guide.md, Thread Safety Summary). /// @return true if the client is running (including when it already was), false on failure bool start(); @@ -386,10 +387,9 @@ class SendspinClient { /// bounds are described in docs/integration-guide.md (Stopping and Restarting). /// /// 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. + /// state they observe through the getters is the stopped state; one that calls connect_to() + /// or disconnect() is ignored. Main-loop thread only, and never from a listener, role or + /// provider callback (docs/integration-guide.md, Thread Safety Summary). void stop(); /// @brief Returns true between a successful start() and stop() @@ -857,8 +857,7 @@ class SendspinClient { /// @brief Drains the inbox: high-performance requests, the time-sync report, lifecycle events /// (each role's teardown half caught up ahead of its stamped events), provider writes, /// pairing notes, role slots, and group updates, dispatching listener callbacks on the - /// calling (main-loop) thread. Shared by loop() and stop(). A callback that calls stop() - /// abandons the rest of this drain (EventState::drain_generation). + /// calling (main-loop) thread. Shared by loop() and stop(). void drain_inbox(); /// @brief Applies the high-performance edges the protocol task queued: every acquire, then @@ -1169,8 +1168,8 @@ class SendspinClient { std::atomic network_ready_{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. + /// is_started() reads false, loop() is a no-op, and connect_to(), disconnect() and every + /// request that queues a command are ignored while it is set. enum class LifecycleState : uint8_t { STOPPED, RUNNING, STOPPING }; std::atomic lifecycle_{LifecycleState::STOPPED}; }; diff --git a/src/artwork_role.cpp b/src/artwork_role.cpp index 38e01662..5c64e4d8 100644 --- a/src/artwork_role.cpp +++ b/src/artwork_role.cpp @@ -644,11 +644,8 @@ void ArtworkRole::Impl::drain_events() { // before this call, so it has already cleared held_display_mask before we get here. ArtworkDisplayUpdate update{}; bool have_update = false; - const uint32_t generation = take_current_payload(*this, this->event_state->display_slot, update, - have_update, TAG, "artwork displays"); - if (!this->accepts(generation)) { - return; - } + take_current_payload(*this, this->event_state->display_slot, update, have_update, TAG, + "artwork displays"); if (have_update) { for (uint8_t slot = 0; slot < ARTWORK_MAX_SLOTS; ++slot) { const uint8_t bit = static_cast(1U << slot); @@ -739,11 +736,6 @@ void ArtworkRole::Impl::drain_events() { this->listener->on_image_display(slot, display_lateness_ms(client_ts, overdue_us)); } } - // The callback may re-enter teardown (a listener calling stop()), whose own drain already - // dropped the holds. - if (!this->accepts(generation)) { - return; - } } } diff --git a/src/client.cpp b/src/client.cpp index cbb61b75..8dec2181 100644 --- a/src/client.cpp +++ b/src/client.cpp @@ -278,8 +278,7 @@ struct SendspinClient::EventState { std::vector drained_pairing_notes{reserved_pairing_notes()}; /// The std::string the deprecated pairing callbacks are handed, assigned in place from each /// note's text and reserved for the longest (PairingNote::text), so no delivery allocates. - /// Main loop only. A drain nested in a callback (its stop()) delivers only the dismissals, - /// which carry no text, so it never reassigns the string a callback is reading. + /// Main loop only. std::string deprecated_pairing_text{reserved_string(decltype(PairingNote::text)::capacity())}; /// The primary connection's server information, written by the protocol task each time the /// primary changes (ConnectionManager::refresh_published_state()), latest-wins. @@ -287,10 +286,6 @@ struct SendspinClient::EventState { /// The server information the drain last took from server_slot, which server_information() /// views. Main loop only. std::optional server_information; - /// Bumped by stop() before its teardown so a drain frame already on the stack (a listener - /// callback that called stop()) abandons what it has not delivered yet: the events it copied - /// out, the rest of the pairing notes, the role drains and the group update. Main loop only. - uint32_t drain_generation{0}; /// @brief Appends `note` to the pairing slot. Every SendspinClient::note_*() method funnels /// through this so the append lives once. PairingNote is anonymous-namespace-private to this @@ -461,14 +456,8 @@ int64_t SendspinClient::now_us() { // ============================================================================ bool SendspinClient::start() { - 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; + if (this->is_started()) { + return true; } // Fail closed, like a failed identity: a Pairing PSK any peer could hold admits every server @@ -656,10 +645,9 @@ void SendspinClient::stop() { if (this->lifecycle_.load(std::memory_order_relaxed) != LifecycleState::RUNNING) { return; } - // From here the client reads as stopped: is_started() is false, loop() is a no-op, and - // start()/stop() and every request that queues a command or posts a request are refused, so - // a listener callback fired below cannot recurse into the teardown, restart the server, or - // reach a connection. + // From here the client reads as stopped: is_started() is false, and every request that queues + // a command or posts a request is refused, so a call from another thread or from a listener + // callback fired below cannot reach a connection. this->lifecycle_.store(LifecycleState::STOPPING, std::memory_order_release); // 1-4. Signal the drain roles, close admission, join the protocol task (its final tick @@ -675,12 +663,9 @@ void SendspinClient::stop() { // 6. Reset per-connection and role state exactly as a lost connection does, here on the main // loop since every other thread is joined. With every producer gone, the state this - // leaves behind is the state a restart begins from. The drain generation bump abandons - // the events a drain frame further up the stack (a listener that called stop()) copied - // out before this teardown. The group slot is reset too, so the drain below cannot - // repopulate the group state from a delta that arrived before the stop, and the server - // slot with the copy server_information() views. - ++this->event_state_->drain_generation; + // leaves behind is the state a restart begins from. The group slot is reset too, so the + // drain below cannot repopulate the group state from a delta that arrived before the stop, + // and the server slot with the copy server_information() views. this->cleanup_connection_state(ALL_ROLES_MASK); this->task_state_->client_state.reset(); this->event_state_->clear_group_state(); @@ -956,10 +941,6 @@ void SendspinClient::drain_inbox() { // be written by a producer between this snapshot and that one. The provider writes poll for // themselves right before the pairing notes. auto& es = *this->event_state_; - // A listener callback below may call stop(), which tears every role down and runs a drain of - // its own; everything this frame has not delivered by then is abandoned (see - // EventState::drain_generation). - const uint32_t drain_generation = es.drain_generation; // Catches each role up on a teardown: with `cleared` null, to the role's current generation; // otherwise only the role a *_CLEARED event names, to the generation it carries. @@ -1043,9 +1024,7 @@ void SendspinClient::drain_inbox() { } // --- Lifecycle events --- - // Skipped once a callback above called stop(): its own drain delivered the ring, and a take - // here would pull off what a start() from the same callback queued for the next drain. - if ((inbox_bits & INBOX_TOPIC_EVENTS) && es.drain_generation == drain_generation) { + if (inbox_bits & INBOX_TOPIC_EVENTS) { // Drain in small batches to bound the stack cost on the shared main-loop task. A batch that // comes back partial means the ring is empty, ending the loop; events pushed mid-drain are // still delivered this tick as long as full batches keep arriving. Sized as a fraction of @@ -1054,17 +1033,9 @@ void SendspinClient::drain_inbox() { constexpr size_t EVENT_DRAIN_BATCH_SIZE = Inbox::EVENT_CAPACITY / 4; InboxEvent events[EVENT_DRAIN_BATCH_SIZE]; size_t event_count = 0; - // A dispatched event below can run a listener callback that calls stop(), whose teardown - // bumps the drain generation and wipes the ring. Events already copied into the local - // batch are stale at that point and must be dropped, exactly as the ring reset intended. - bool drain_aborted = false; do { event_count = es.inbox.take_events(events, EVENT_DRAIN_BATCH_SIZE); for (size_t i = 0; i < event_count; ++i) { - if (es.drain_generation != drain_generation) { - drain_aborted = true; - break; - } const InboxEvent& event = events[i]; // Every event is stamped with its role's teardown generation. The role catches up // on that teardown before the event is acted on (catch_up_teardown()), so the @@ -1152,15 +1123,7 @@ void SendspinClient::drain_inbox() { } } } - // A stop() that re-entered on the final event of a full batch bumps the drain - // generation but leaves drain_aborted false: the check at the top of the inner loop - // never runs again because there is no next iteration. Re-check here so the loop stops - // instead of calling take_events() again and destructively pulling the cleanup's - // freshly re-pushed *_CLEARED events off the live ring (dropping them). - if (es.drain_generation != drain_generation) { - drain_aborted = true; - } - } while (!drain_aborted && event_count == EVENT_DRAIN_BATCH_SIZE); + } while (event_count == EVENT_DRAIN_BATCH_SIZE); } // Second snapshot: catches topic bits a producer set while the ring drain above was running. @@ -1175,21 +1138,11 @@ void SendspinClient::drain_inbox() { // the protocol task requests the write before it queues the PAIRING_SUCCEEDED note, so the // flush (which polls for itself) runs after the notes are taken and before they are // dispatched, and every note taken finds its write performed. - // Moved out for the dispatch and back after it, so a listener callback that re-enters the - // drain (a stop() whose dismissals it drains) cannot touch the batch this frame is delivering. - // That nested drain finds the buffer gone and, only when it has notes to take, reserves - // another before trading it to the slot, which therefore keeps its room; the one this frame - // moves back, emptied, replaces the nested drain's. - std::vector notes = std::move(es.drained_pairing_notes); - bool notes_taken = false; - if ((slot_bits & INBOX_TOPIC_PAIRING) != 0 && es.drain_generation == drain_generation) { - if (notes.capacity() < PAIRING_NOTE_CAPACITY) { - notes.reserve(PAIRING_NOTE_CAPACITY); - } - notes_taken = es.pairing_slot.exchange(notes); - } + std::vector& notes = es.drained_pairing_notes; + const bool notes_taken = + (slot_bits & INBOX_TOPIC_PAIRING) != 0 && es.pairing_slot.exchange(notes); this->flush_pending_persistence(); - if (notes_taken && es.drain_generation == drain_generation) { + if (notes_taken) { if (this->listener_ != nullptr) { // In queue order, skipping a coalesced type (is_coalesced_note()) that repeats its // prompt's last delivered note. The listener is set before start() and must outlive @@ -1206,8 +1159,6 @@ void SendspinClient::drain_inbox() { } last_note = note.type; } - // The view form views the note in `notes`, which a drain nested in the callback - // cannot reach. switch (note.type) { case PairingNoteType::PAIRING_STARTED: this->listener_->on_pairing_attempt(note.text.view()); @@ -1231,58 +1182,45 @@ void SendspinClient::drain_inbox() { this->listener_->on_close_pairing_window(); break; } - // A stop() from the view form ended the delivery, so the deprecated form of the - // note must not follow. - if (carries_text(note.type) && es.drain_generation == drain_generation) { + if (carries_text(note.type)) { es.deliver_deprecated_pairing_note(*this->listener_, note); } - // A re-entrant stop() bumped the generation: abandon the rest of the batch. - if (es.drain_generation != drain_generation) { - break; - } } } } notes.clear(); - es.drained_pairing_notes = std::move(notes); // --- Role events: bit-gated so an idle tick performs zero inbox mutex acquisitions here --- // Each role drain takes its slot first and catches the role up on its teardowns before - // applying anything (see TeardownTracker), and abandons the rest of its own work when one of - // its callbacks re-entered teardown (its generation moved on). Between role drains, and - // before the group update, the drain generation decides whether this frame was overtaken by - // a stop() a callback made. - const auto overtaken = [&es, drain_generation] { - return es.drain_generation != drain_generation; - }; + // applying anything (see TeardownTracker). #ifdef SENDSPIN_ENABLE_PLAYER - if (!overtaken() && this->player_ && this->player_->impl_->needs_drain(slot_bits)) { + if (this->player_ && this->player_->impl_->needs_drain(slot_bits)) { this->player_->impl_->drain_events(); } #endif #ifdef SENDSPIN_ENABLE_CONTROLLER - if (!overtaken() && this->controller_ && this->controller_->impl_->needs_drain(slot_bits)) { + if (this->controller_ && this->controller_->impl_->needs_drain(slot_bits)) { this->controller_->impl_->drain_events(); } #endif #ifdef SENDSPIN_ENABLE_METADATA - if (!overtaken() && this->metadata_ && this->metadata_->impl_->needs_drain(slot_bits)) { + if (this->metadata_ && this->metadata_->impl_->needs_drain(slot_bits)) { this->metadata_->impl_->drain_events(); } #endif #ifdef SENDSPIN_ENABLE_COLOR - if (!overtaken() && this->color_ && this->color_->impl_->needs_drain(slot_bits)) { + if (this->color_ && this->color_->impl_->needs_drain(slot_bits)) { this->color_->impl_->drain_events(); } #endif #ifdef SENDSPIN_ENABLE_ARTWORK - if (!overtaken() && this->artwork_ && this->artwork_->impl_->needs_drain(slot_bits)) { + if (this->artwork_ && this->artwork_->impl_->needs_drain(slot_bits)) { this->artwork_->impl_->drain_events(); } #endif // --- Group update events --- - if (!overtaken() && (slot_bits & INBOX_TOPIC_GROUP) != 0) { + if ((slot_bits & INBOX_TOPIC_GROUP) != 0) { GroupUpdatePayload payload; if (es.group_slot.take(payload)) { es.apply_group_update(payload); @@ -1299,11 +1237,7 @@ void SendspinClient::drain_inbox() { if (this->listener_) { this->listener_->on_group_state(group_state_of(payload)); - // A stop() from on_group_state() cleared the group state, so the deprecated form - // of the update must not follow. - if (!overtaken()) { - es.deliver_group_update(*this->listener_, payload); - } + es.deliver_group_update(*this->listener_, payload); } } } diff --git a/src/controller_role.cpp b/src/controller_role.cpp index 33446afe..0557bbf5 100644 --- a/src/controller_role.cpp +++ b/src/controller_role.cpp @@ -129,7 +129,7 @@ void ControllerRole::Impl::drain_events() { bool have_state = false; const uint32_t generation = take_current_payload(*this, this->event_state->slot, state, have_state, TAG, "controller state"); - if (!have_state || !this->accepts(generation)) { + if (!have_state) { return; } this->apply_state(state); diff --git a/src/inbox.h b/src/inbox.h index 2e7024a9..afd9e291 100644 --- a/src/inbox.h +++ b/src/inbox.h @@ -551,8 +551,9 @@ class InboxSlot { * Every write and merge keeps the newest generation's content: a delta stamped with a later * generation than the pending content replaces it, and one stamped with an earlier generation * than the pending content is dropped, so a producer that lost a race with a teardown cannot - * revive the content the teardown discarded. Lives on the shared Inbox mutex like every - * InboxSlot; no lock of its own. + * revive the content the teardown discarded. A drain that takes a payload belonging to an event it + * already holds, rather than at its head, takes it with take_stamped() under that event's stamp. + * Lives on the shared Inbox mutex like every InboxSlot; no lock of its own. */ template class GenerationSlot { @@ -603,6 +604,29 @@ class GenerationSlot { return true; } + /// @brief Moves the pending payload out only when it is stamped `generation`, for a payload + /// that belongs to something the drain already holds (the player's stream parameters, taken + /// with their STREAM_START): one stamped with a later generation stays pending for its own + /// taker, and one stamped with an earlier generation is discarded + /// @return false when no payload stamped `generation` was pending. + bool take_stamped(T& out, uint32_t generation) { + bool taken = false; + this->slot_.edit([&out, &taken, generation](Stamped& current) { + if (current.generation != generation) { + if (count_after(current.generation, generation)) { + return true; // A later generation's: left for its own taker + } + current = Stamped{}; + return false; + } + out = std::move(current.value); + current = Stamped{}; + taken = true; + return false; + }); + return taken; + } + /// @brief Discards the pending payload void reset() { this->slot_.reset(); diff --git a/src/metadata_role.cpp b/src/metadata_role.cpp index e87ab599..2f242c36 100644 --- a/src/metadata_role.cpp +++ b/src/metadata_role.cpp @@ -260,16 +260,7 @@ void MetadataRole::Impl::apply_current_state() { if (this->listener == nullptr) { return; } - // Moves only when the main loop runs a teardown's half, which during the callback means the - // listener called stop() (a callback never calls loop()). Not accepts(): the protocol task may - // bump the generation during the callback, and the deprecated form must still follow then; - // the clear that teardown queued comes on a later drain. - const uint32_t completed = this->teardown.completed(); this->listener->on_track_metadata(this->track_metadata()); - // The teardown delivered its clear: the deprecated form of the state must not follow it. - if (this->teardown.completed() != completed) { - return; - } this->fill_deprecated_state(); #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wdeprecated-declarations" diff --git a/src/metadata_role_impl.h b/src/metadata_role_impl.h index cf0d97d2..d1cd1c84 100644 --- a/src/metadata_role_impl.h +++ b/src/metadata_role_impl.h @@ -111,7 +111,7 @@ struct MetadataRole::Impl : RoleTeardown { /// Whether a state's server-clock deadline has passed on the synchronized client clock. bool state_is_due(int64_t timestamp) const; /// @brief Hands the current state to on_track_metadata(), then to the deprecated - /// on_metadata() unless the first call re-entered teardown. Main loop, with no lock held. + /// on_metadata(). Main loop, with no lock held. void apply_current_state(); /// @brief The public view of the current state, its text viewing the current block. Main loop. TrackMetadata track_metadata() const; diff --git a/src/player_role.cpp b/src/player_role.cpp index b5bb7997..ff8e2f78 100644 --- a/src/player_role.cpp +++ b/src/player_role.cpp @@ -518,15 +518,13 @@ void PlayerRole::Impl::drain_events() { (void)this->event_state->sync_idle_slot.take(sync_idle_note); ServerCommandMessage cmd_msg{}; bool have_command = false; - const uint32_t generation = take_current_payload(*this, this->event_state->command_slot, - cmd_msg, have_command, TAG, "server command"); + take_current_payload(*this, this->event_state->command_slot, cmd_msg, have_command, TAG, + "server command"); // --- Server command events (volume, mute, output delay) --- // Check each field independently since multiple command types may have been merged into one - // inbox slot between drain ticks. Each callback may re-enter teardown (a listener calling - // stop()), whose own drain already reset this role, so the rest is abandoned when the - // generation moved on. roles/player/v1.md "server/command player object": a command absent - // from the current supported_commands is ignored. + // inbox slot between drain ticks. roles/player/v1.md "server/command player object": a + // command absent from the current supported_commands is ignored. if (have_command && cmd_msg.player.has_value()) { const ServerPlayerCommandObject& player_cmd = cmd_msg.player.value(); @@ -541,7 +539,7 @@ void PlayerRole::Impl::drain_events() { if (player_cmd.mute.has_value() && !this->mute_adjustable) { SS_LOGD(TAG, "Ignoring mute: not in supported_commands"); - } else if (player_cmd.mute.has_value() && this->accepts(generation)) { + } else if (player_cmd.mute.has_value()) { this->update_muted(player_cmd.mute.value()); if (this->listener) { this->listener->on_mute_changed(player_cmd.mute.value()); @@ -551,7 +549,7 @@ void PlayerRole::Impl::drain_events() { const bool delay_advertised = this->output_delay_adjustable.load(std::memory_order_relaxed); if (player_cmd.output_delay_ms.has_value() && !delay_advertised) { SS_LOGD(TAG, "Ignoring set_output_delay: not in supported_commands"); - } else if (player_cmd.output_delay_ms.has_value() && this->accepts(generation)) { + } else if (player_cmd.output_delay_ms.has_value()) { this->update_output_delay(player_cmd.output_delay_ms.value()); if (this->listener) { this->listener->on_output_delay_changed( @@ -559,9 +557,6 @@ void PlayerRole::Impl::drain_events() { } } } - if (!this->accepts(generation)) { - return; - } // --- Process awaiting sync idle events --- // Stream lifecycle arrivals (STREAM_START/STREAM_END) are appended directly to @@ -572,15 +567,7 @@ void PlayerRole::Impl::drain_events() { if (!this->awaiting_sync_idle_events.empty()) { bool sync_idle = !this->sync_task->is_running(); size_t processed = 0; - bool teardown_reentered = false; - - // Indexed with a fresh size() check per iteration (not a range-for): the listener - // callbacks below may re-enter connection teardown, whose own drain clears this vector - // mid-loop. Cached range-for iterators would dangle; re-checking size() ends the loop - // and the clamp before the erase below keeps the range valid. - // NOLINTNEXTLINE(modernize-loop-convert): body mutates the vector, see above - for (size_t idx = 0; idx < this->awaiting_sync_idle_events.size(); ++idx) { - const InboxEvent event = this->awaiting_sync_idle_events[idx]; + for (const InboxEvent& event : this->awaiting_sync_idle_events) { const auto type = static_cast(event.code); if (type == PlayerStreamCallbackType::STREAM_END && !sync_idle) { // Wait for the sync task to go idle before firing this and anything after it. The @@ -594,8 +581,7 @@ void PlayerRole::Impl::drain_events() { // Only fire the callback when a stream is actually open: every teardown owes // a STREAM_END (complete_teardown()), and a failed stream start enqueues one // with no preceding START, so gating here keeps on_stream_end() paired 1:1 - // with on_stream_start(). Cleared before the callback, which may re-enter - // teardown and owe a STREAM_END of its own: that one finds the stream closed. + // with on_stream_start(). if (this->stream_active) { this->stream_active = false; if (this->listener) { @@ -606,9 +592,6 @@ void PlayerRole::Impl::drain_events() { this->client->release_high_performance(); this->high_performance_requested_for_playback = false; } - if (!this->accepts(generation)) { - teardown_reentered = true; - } break; case PlayerStreamCallbackType::STREAM_START: { // Request high-performance networking for playback (deferred from the @@ -618,36 +601,17 @@ void PlayerRole::Impl::drain_events() { this->client->acquire_high_performance(); this->high_performance_requested_for_playback = true; } - // The params handle_stream_start() wrote with this START. A teardown since - // then was caught up above and removed the START from this vector, so a stamp - // that is not current belongs to an older stream whose START was dropped. + // The params handle_stream_start() wrote with this START, taken under the + // START's own stamp: params a later teardown's next stream wrote while a + // callback above ran stay pending for that stream's START. ServerPlayerStreamObject stream_params; - uint32_t params_stamp = 0; - if (this->event_state->stream_params_slot.take(stream_params, params_stamp)) { - if (params_stamp == generation) { - this->current_stream_params = stream_params; - } else { - SS_LOGD(TAG, "Dropping stream params queued before the role was torn " - "down"); - } + if (this->event_state->stream_params_slot.take_stamped(stream_params, + event.epoch)) { + this->current_stream_params = stream_params; } - // Mark the stream active before invoking the listener. on_stream_start() may - // re-enter teardown, and the STREAM_END the teardown owes fires - // on_stream_end() only when stream_active is set (see the gate above). Setting - // it first keeps start/end paired even when the batch is abandoned below. this->stream_active = true; if (this->listener) { this->listener->on_stream_start(); - // on_stream_start() may re-enter connection teardown, whose own drain - // already ended the stream and replaced this vector's content. - // Re-arming the sync task below would resurrect the dead stream, so - // abandon the batch instead (the clamp below keeps the erase in range). - // stream_active stays true so a STREAM_END the teardown still owes - // delivers a paired on_stream_end(). - if (!this->accepts(generation)) { - teardown_reentered = true; - break; - } } this->sync_task->signal_stream_start(event.serial); // The sync task sets TASK_RUNNING asynchronously after this signal, so the @@ -657,17 +621,9 @@ void PlayerRole::Impl::drain_events() { break; } } - if (teardown_reentered) { - break; - } ++processed; } - // Clamp: a re-entrant teardown may have cleared the vector mid-loop, so processed can - // exceed the current size. - if (processed > this->awaiting_sync_idle_events.size()) { - processed = this->awaiting_sync_idle_events.size(); - } if (processed > 0) { this->awaiting_sync_idle_events.erase( this->awaiting_sync_idle_events.begin(), @@ -677,10 +633,8 @@ void PlayerRole::Impl::drain_events() { } void PlayerRole::Impl::cleanup() { - // Flag the teardown before anything else: it tells a drain_events() frame that may be on the - // call stack right now (a listener callback re-entering teardown) that the stream is gone, - // and it stamps every event queued from here on, so a START this teardown just invalidated is - // discarded (see cleanup_generation). + // Flag the teardown before anything else: it stamps every event queued from here on, so a + // START this teardown just invalidated is discarded (see cleanup_generation). const uint32_t generation = this->cleanup_generation.fetch_add(1, std::memory_order_acq_rel) + 1; diff --git a/src/scheduled_state_slot.h b/src/scheduled_state_slot.h index cd9a9fac..fa9f21b2 100644 --- a/src/scheduled_state_slot.h +++ b/src/scheduled_state_slot.h @@ -265,13 +265,7 @@ void drain_scheduled_state(RoleImpl& role, ScheduledStateSlot& slot, Deli for (;;) { const typename Slot::Peek peek = slot.peek(); bool current = peek.oldest.has_value(); - const uint32_t generation = keep_current_payload(role, current, peek.generation, tag, what); - // Also catches a catch-up whose clear callback re-entered teardown (a listener calling - // stop()): its cleanup() moved the generation on, and its own drain already reset this - // role. - if (!role.accepts(generation)) { - return; - } + keep_current_payload(role, current, peek.generation, tag, what); // States stamped before the teardown are decided away: none delivered, none pending. const typename Slot::Decision decision = current ? Slot::decide(peek, is_due) : typename Slot::Decision{}; @@ -282,9 +276,6 @@ void drain_scheduled_state(RoleImpl& role, ScheduledStateSlot& slot, Deli return; } apply(); - // A callback that re-entered teardown (a listener calling stop()) reset the slot, and the - // drain stop() ran forgot pending_at() (the role's complete_teardown()), so this ends the - // loop too. const std::optional pending_at = slot.pending_at(); if (!pending_at.has_value() || !is_due(*pending_at)) { return; diff --git a/src/source_role.cpp b/src/source_role.cpp index 8ed9e437..4863cbab 100644 --- a/src/source_role.cpp +++ b/src/source_role.cpp @@ -373,10 +373,6 @@ void SourceRole::Impl::handle_stream_ring_event(SourceStreamEventType event) { if (started == this->streaming_active) { return; // Already reported } - // Flipped before the callback, which may re-enter teardown: its catch-up then finds the - // state the callback reports. The hold changes before the callback too: a stop() from - // on_streaming_stopped() catches up through complete_teardown(), which finds streaming_active - // already clear and releases nothing. this->streaming_active = started; if (started) { // An open stream holds high-performance networking, as playback does diff --git a/src/teardown_tracker.h b/src/teardown_tracker.h index c29cc2b1..5c2c9702 100644 --- a/src/teardown_tracker.h +++ b/src/teardown_tracker.h @@ -62,19 +62,14 @@ class TeardownTracker { return true; } - /// @brief The latest teardown generation whose main-loop half has run - uint32_t completed() const { - return this->completed_; - } - private: /// The latest teardown generation whose main-loop half has run. uint32_t completed_{0}; }; /** - * @brief A role's teardown state, shared by every role's Impl: its teardown generation, the - * check each drain makes against it, and its TeardownTracker + * @brief A role's teardown state, shared by every role's Impl: its teardown generation and its + * TeardownTracker * * cleanup() bumps cleanup_generation and stamps everything the role queues from then on with the * new value: its events, its slot payloads, the items it hands a role thread. Each protocol-task @@ -82,17 +77,10 @@ class TeardownTracker { * on the protocol task between two handlers, or from stop() once that task is joined, so the * generation stays current for the whole handler and the handler does not check it. The stamp * protects the threads that consume what the handler queued: the drains check a payload's stamp - * with accepts() (event_is_current(), GenerationSlot), and a role thread an item's - * (InboundConsumer::take()), so a teardown that runs before they take it discards it; a drain also - * uses accepts() to detect a listener callback that re-entered teardown. + * (event_is_current(), keep_current_payload()), and a role thread an item's + * (InboundConsumer::take()), so a teardown that runs before they take it discards it. */ struct RoleTeardown { - /// @brief Whether an effect stamped with `generation` may still be applied - /// @param generation The counter value the effect was stamped with. - bool accepts(uint32_t generation) const { - return generation == this->cleanup_generation.load(std::memory_order_acquire); - } - TeardownTracker teardown; ///< Main loop only. /// Written by cleanup() on the protocol task, or on the main loop in SendspinClient::stop() /// once the task is joined; read on the main loop, the protocol task, the role's own thread @@ -134,9 +122,7 @@ uint32_t keep_current_payload(RoleImpl& role, bool& have, uint32_t stamp, const /// /// Taken before the catch-up: a teardown that ran before the take is caught up here (its clear /// fires first) and drops a payload stamped before it; one that runs after the take is caught up -/// by the next drain, behind what this drain applies. The caller then checks -/// `role.accepts(generation)`, which also catches a clear callback that re-entered teardown (a -/// listener calling stop()). Main loop only. +/// by the next drain, behind what this drain applies. Main loop only. /// @param[out] have Whether `out` holds a payload stamped with the returned generation. /// @param what What the payload is, for the drop's log line. /// @return The role's generation the catch-up ran to. diff --git a/src/visualizer_role.cpp b/src/visualizer_role.cpp index 41e19957..e33c6b41 100644 --- a/src/visualizer_role.cpp +++ b/src/visualizer_role.cpp @@ -386,8 +386,9 @@ void VisualizerRole::Impl::handle_stream_start(const VisualizerStreamPayload& st // Write the config to the inbox slot for the main thread, then push the event. Both lock the // same shared Inbox mutex, in this order, so a consumer that later takes the START event is - // guaranteed to observe this config (see config_slot.take() in handle_stream_ring_event()). - // Both carry `generation`, so a config left over from a torn-down stream is never applied. + // guaranteed to observe this config (see config_slot.take_stamped() in + // handle_stream_ring_event()). Both carry `generation`, so a config left over from a torn-down + // stream is never applied. this->event_state->config_slot.write(VisualizerStreamPayload{admitted}, generation); this->enqueue_stream_event(VisualizerEventType::STREAM_START, generation); } @@ -433,12 +434,7 @@ void VisualizerRole::Impl::handle_stream_ring_event(VisualizerEventType event, switch (event) { case VisualizerEventType::STREAM_START: { VisualizerStreamPayload config{}; - uint32_t stamp = 0; - if (!this->event_state->config_slot.take(config, stamp)) { - break; - } - if (stamp != generation) { - SS_LOGD(TAG, "Dropping a visualizer config queued before the role was torn down"); + if (!this->event_state->config_slot.take_stamped(config, generation)) { break; } ServerVisualizerStreamObject& out = this->stream_object; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 15de695a..c11065ae 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -150,19 +150,22 @@ endif() # old stamp, and reads cleanup_generation and the applied state (controller_state, the metadata # role's current, color, the player's stream_active, current_stream_params and volume); its player runs with the # sync thread stopped and the item list bound by hand (SyncTask's stop() and inbound_), so a -# STREAM_END never waits for a thread to go idle. The high-performance grant tables +# STREAM_END never waits for a thread to go idle. The stream-params test runs on the same player, +# calling its cleanup() and handle_stream_start() from inside a listener callback, the protocol +# task's work landing while a drain runs. The stop-callback test reads the protocol task's request +# slot (take_requests()) after stop(), since start() drops what waits there and no peer sees the +# difference. The high-performance grant tables # play the protocol task's run_time_sync() on a stand-in in the admitted slot (find_admitted(), # its high_performance_held) and read the stand-in's time_frame_sent_us_, the one trace of a time # frame written without a Noise session that another thread may read; the wake test restarts the # protocol task (protocol_task_->start()) with liveness off (liveness_timeout_us_) so the grant is # the only wake, and polls the client's Inbox through a role's impl_->inbox, which also lets the -# reorder table fill the event ring. The re-entrant pairing table queues pairing notes as the -# protocol task's handlers do (SendspinClient's note_open_pairing_window(), -# note_display_pairing_code() and note_clear_pairing_code()), since no public call shows a prompt, -# and from inside its listener runs the final tick stop() would join (close_admission(), -# protocol_tick()), so the shutdown pass takes the prompt flags it set on its stand-in's -# pairing_session() (window_shown and code_emitted). The pairing forms table queues every -# text-carrying note the same way (note_pairing_started(), note_pairing_succeeded(), +# reorder table fill the event ring. The pairing teardown table queues pairing notes as the +# protocol task's handlers do (SendspinClient's note_display_pairing_code(), +# note_open_pairing_window(), note_clear_pairing_code() and note_close_pairing_window()), since no +# public call shows a prompt, and runs the teardown cleanup_connection_state() performs on a +# connection loss directly, before the main loop drains what was queued. The pairing forms table +# queues every text-carrying note the same way (note_pairing_started(), note_pairing_succeeded(), # note_pairing_failed() and note_display_pairing_code()) with the protocol task stopped # (protocol_task_->stop()), since no public call produces each abort reason and code format on # demand. The server information table hands the main loop two servers between drains as @@ -170,13 +173,12 @@ endif() # call changes the primary twice inside one main-loop tick. A second test also queues the metadata # role's teardown (its cleanup()) beside the server going to none, so its clear callback reads # server_information() as the drain delivers it, since no public call stages both in one tick. -# The stream-events table drives the -# player's handle_stream_start() and handle_stream_end() as the task would, and stages one -# interleaving by waiting on the sync task's item list and the player's sync-idle topic bit, and its -# clear row calls handle_stream_clear() the same way. The next-stream header table plays the sync -# task itself on the test thread, because no public call holds the task between its ACTIVE load -# and its next IDLE wait: it calls SyncTask's wait_for_codec_header() and load_next_chunk() and -# returns items through its inbound_, +# The stream-events table drives the player's handle_stream_start() and handle_stream_end() as the +# task would, and stages one interleaving by waiting on the sync task's item list and the player's +# sync-idle topic bit, and its clear row calls handle_stream_clear() the same way. The next-stream +# header table plays the sync task itself on the test thread, because no public call holds the +# task between its ACTIVE load and its next IDLE wait: it calls SyncTask's wait_for_codec_header() +# and load_next_chunk() and returns items through its inbound_, # resets the flags with event_flags_.clear_all() as start() would, sets and reads SyncContext's # active_ordinal and next_header, and reads PlayerRole::Impl::stream_ordinal. The metadata slot # table plays the protocol task's half of the metadata role (handle_server_state(), and for a diff --git a/tests/README.md b/tests/README.md index e50a93f8..3f5b4a37 100644 --- a/tests/README.md +++ b/tests/README.md @@ -116,9 +116,9 @@ Each `test_*.cpp` file covers one unit of cross-platform logic: stamp it reads, persistence written from the main loop, controller command validation, and the metadata fields the listener is handed. - `test_client_lifecycle.cpp`: `start()`/`stop()`/restart: goodbyes, clear callbacks delivered - inside `stop()`, re-entrancy from callbacks, role start rollback, accepts refused at `stop()`, - the command queue's refusals and the lifecycle requests it never refuses, the protocol task's - next deadline, the teardown reorder guarantee for every role with main-loop state, stream + inside `stop()`, what a callback inside `stop()` sees, role start rollback, accepts refused at + `stop()`, the command queue's refusals and the lifecycle requests it never refuses, the protocol + task's next deadline, the teardown reorder guarantee for every role with main-loop state, stream starts acknowledged by number, the high-performance grant, the time filter slot, the inbound ring's size, the pairing callbacks in each listener form, and the metadata role's scheduled-state slot, whose three text blocks carry every state to the listener in order. diff --git a/tests/test_client_lifecycle.cpp b/tests/test_client_lifecycle.cpp index cf9d6b99..f246cef4 100644 --- a/tests/test_client_lifecycle.cpp +++ b/tests/test_client_lifecycle.cpp @@ -14,7 +14,7 @@ /// SendspinClient start/stop/restart: stop() goodbyes every peer, leaves every role and slot /// 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. +/// again, and a callback fired from inside stop() sees the stopped client. /// /// The client is driven on loopback ports like test_connection_lifecycle.cpp: a /// FakeEncryptedServer plays the Sendspin server over the real Noise transport and the test @@ -190,17 +190,16 @@ class SilentListener { uint16_t port_{0}; }; -/// Records on_metadata_clear() and, from inside it, tries to drive the lifecycle re-entrantly. -class ReentrantMetadataListener : public MetadataRoleListener { +/// Records on_metadata_clear() and, from inside it, reads the client and makes the connection +/// requests a callback may make. +class ClearReadingMetadataListener : public MetadataRoleListener { public: - explicit ReentrantMetadataListener(SendspinClient& client) : client_(client) {} + explicit ClearReadingMetadataListener(SendspinClient& client) : client_(client) {} void on_metadata_clear() override { ++this->clears; this->started_during_clear = this->client_.is_started(); this->group_had_state_during_clear = this->client_.group_state().playback_state.has_value(); - this->start_result_during_clear = this->client_.start(); - 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 } @@ -208,7 +207,6 @@ class ReentrantMetadataListener : public MetadataRoleListener { int clears{0}; bool started_during_clear{true}; bool group_had_state_during_clear{true}; - bool start_result_during_clear{true}; private: SendspinClient& client_; @@ -410,14 +408,13 @@ TEST(ClientLifecycle, OpusStreamStartIsRefusedWithoutTheOpusDecoder) { } #endif -// A listener callback fired from inside stop() cannot re-enter the lifecycle: start() reports -// 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) { +// A listener callback fired from inside stop() sees the client (its started flag and its group +// state) already stopped, and its disconnect() and connect_to() are ignored rather than reaching +// a connection. Afterwards the client restarts normally. +TEST(ClientLifecycle, CallbackDuringStopSeesTheStoppedClient) { PairedClientBundle bundle(make_config(CALLBACK_TEST_PORT)); SendspinClient& client = bundle.client(); - ReentrantMetadataListener listener(client); + ClearReadingMetadataListener listener(client); client.add_metadata().set_listener(&listener); ASSERT_TRUE(bundle.start()); @@ -433,12 +430,15 @@ TEST(ClientLifecycle, CallbackDuringStopCannotRecurse) { 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()); + // Read from the request slot: the next start() drops whatever waits there, so nothing a + // peer sees tells an ignored request from one posted and dropped. + LifecycleRequests requests; + EXPECT_FALSE(client.protocol_task_->take_requests(requests)) + << "connect_to() or disconnect() inside stop() posted a request"; wait_until([&] { return server->closed(); }); } - // The refused start() inside the callback left the client stopped; a real start() works. ASSERT_TRUE(bundle.start()); auto server = connect_paired_server(bundle.peer, CALLBACK_TEST_PORT); pump_until(client, [&] { return client.is_connected(); }); @@ -2763,112 +2763,60 @@ TEST(TeardownReorder, AClearAlwaysPrecedesTheNextConnectionsState) { } } -/// Calls stop(), and then start() when `restart` is set, from inside the first controller state -/// callback; records every state-role callback in `log`. -class StopFromCallbackListener : public StateRoleLog { +/// Logs as StateRoleLog does, then runs `during_volume`, once, from inside on_volume_changed(). +class VolumeHookLog : public StateRoleLog { public: - /// The callback that calls stop(): none, the controller's state or the player's stream end. - enum class Reenter : uint8_t { NONE, CONTROLLER_STATE, STREAM_END }; - - StopFromCallbackListener(SendspinClient*& client, Reenter reenter, bool restart) - : client_(client), reenter_(reenter), restart_(restart) {} - - void on_controller_state(const ServerStateControllerObject& state) override { - StateRoleLog::on_controller_state(state); - this->reenter_from(Reenter::CONTROLLER_STATE); - } - void on_stream_end() override { - StateRoleLog::on_stream_end(); - ++this->stream_ends; - this->reenter_from(Reenter::STREAM_END); - } - - bool restart_result{false}; - int stream_ends{0}; - -private: - void reenter_from(Reenter callback) { - if (this->reenter_ == callback && !this->reentered_) { - this->reentered_ = true; - this->client_->stop(); - if (this->restart_) { - this->restart_result = this->client_->start(); - } + void on_volume_changed(uint8_t volume) override { + StateRoleLog::on_volume_changed(volume); + if (this->during_volume) { + std::exchange(this->during_volume, nullptr)(); } } - SendspinClient*& client_; - Reenter reenter_; - bool restart_; - bool reentered_{false}; + std::function during_volume; }; -// A listener may call stop(), and start() after it, from inside a callback a loop() drain fires. -// stop() tears every role down and delivers each clear exactly once from its own drain; the drain -// that fired the callback then abandons the rest of its work, so the metadata state queued beside -// the controller's is never delivered, after its clear or at all. A stop() from on_stream_end() -// owes no second on_stream_end(): the stream it ends is already closed. Control: without the -// re-entry both states are delivered. -TEST(ClientLifecycle, StopFromInsideADrainCallbackIsSafe) { - using Reenter = StopFromCallbackListener::Reenter; +// A STREAM_START takes only the stream parameters stamped with its own generation. A drain that +// holds stream A's START runs a server command's callback first, and a teardown the protocol task +// runs meanwhile, followed by the next connection's stream/start for stream B, writes B's +// parameters before A's START is processed. A's START leaves them pending (it still fires, with no +// parameters of its own left to apply, and the teardown's owed end follows), so B's +// on_stream_start() reads B's sample rate. The test thread plays the protocol task +// from inside on_volume_changed(), since no public call lands a teardown and a stream/start +// inside one callback. Control: without the teardown A's START reads A's rate. +TEST(TeardownReorder, AStreamStartTakesOnlyItsOwnStreamParams) { struct Row { const char* name; - Reenter reenter; - bool restart; + bool teardown; std::vector expected_calls; - bool expected_started; - int expected_stream_ends; + uint32_t expected_rate; }; const Row rows[] = { - {"Control: no re-entry", Reenter::NONE, false, {"state 11", "state 5"}, true, 0}, - {"stop()", Reenter::CONTROLLER_STATE, false, {"state 11", "clear", "clear"}, false, 0}, - {"stop() then start()", Reenter::CONTROLLER_STATE, true, {"state 11", "clear", "clear"}, - true, 0}, - {"stop() from on_stream_end()", Reenter::STREAM_END, false, - {"state 48", "clear", "clear", "clear"}, false, 1}, + {"Control: no teardown, A's START reads A's rate", false, {"state 7", "state 48"}, 48000}, + {"a teardown and B's stream/start during the volume callback", + true, + {"state 7", "state 0", "clear", "state 44"}, + 44000}, }; for (const Row& row : rows) { SCOPED_TRACE(row.name); - SendspinClient* client_ref = nullptr; - StopFromCallbackListener log(client_ref, row.reenter, row.restart); - TestNetworkProvider network; - SendspinClient client(make_config(0)); - client_ref = &client; - client.set_network_provider(&network); - client.add_controller().set_listener(&log); - client.add_metadata().set_listener(&log); - PlayerRole& player = client.add_player(make_pcm_player_config()); - player.set_listener(&log); - log.player = &player; - ASSERT_TRUE(client.start()); - client.protocol_task_->stop(); - - if (row.reenter == Reenter::STREAM_END) { - PlayerRole::Impl& impl = *player.impl_; - ServerPlayerStreamObject params; - params.codec = SendspinCodecFormat::PCM; - params.sample_rate = 48000; - params.channels = 2; - params.bit_depth = 16; - impl.handle_stream_start({params}); - pump_until(client, [&] { return impl.sync_task->is_running(); }); - impl.handle_stream_end(); - pump_until(client, [&] { return !client.is_started(); }); - } else { - client.controller_->impl_->handle_server_state(controller_state_with(11)); - handle_metadata_state(client, 5); - client.loop(); + VolumeHookLog log; + StateRoleClient harness(log); + SendspinClient& client = harness.client; + PlayerRole::Impl& impl = *client.player_->impl_; + impl.handle_stream_start({player_stream_with(48)}); + impl.handle_server_command(player_volume_with(7)); + if (row.teardown) { + log.during_volume = [&impl] { + impl.cleanup(); + impl.handle_stream_start({player_stream_with(44)}); + }; } + client.loop(); + client.loop(); EXPECT_EQ(log.calls, row.expected_calls); - EXPECT_EQ(log.stream_ends, row.expected_stream_ends); - EXPECT_EQ(client.is_started(), row.expected_started); - if (row.restart) { - EXPECT_TRUE(log.restart_result) << "start() from inside the callback was refused"; - } - client.loop(); // A later loop() delivers nothing the re-entry abandoned. - EXPECT_EQ(log.calls, row.expected_calls); - client.stop(); + EXPECT_EQ(impl.current_stream_params.sample_rate.value_or(0), row.expected_rate); } } @@ -2882,10 +2830,10 @@ TEST(ClientLifecycle, StopFromInsideADrainCallbackIsSafe) { // test thread plays: same-window arrivals (a third replacing the scheduled second), a pending // state replaced by an arrival, a teardown with states pending, a state stamped before it, an // arrival between a drain's peek and its commit (from the clear its catch-up delivers), a pending -// state that falls due in the callback of the state before it, stop() from on_track_metadata(), -// and a restart. Reaches the role's private state because which block each place names and which -// state waits show nowhere else before a deadline the test never reaches; the primary clock is -// installed by hand, so a scheduled state waits for its deadline without a server. +// state that falls due in the callback of the state before it, and a restart. Reaches the role's +// private state because which block each place names and which state waits show nowhere else +// before a deadline the test never reaches; the primary clock is installed by hand, so a +// scheduled state waits for its deadline without a server. TEST(MetadataSlot, ThreeBlocksCarryEveryStateToTheListenerInOrder) { struct TitleLog : MetadataRoleListener { void on_track_metadata(const TrackMetadata& metadata) override { @@ -2913,9 +2861,7 @@ TEST(MetadataSlot, ThreeBlocksCarryEveryStateToTheListenerInOrder) { DRAIN_ARRIVING, // The role's drain, a scheduled state arriving from the clear it // catches up on, between its peek and its commit DRAIN_FALLING_DUE, // A tick whose first callback brings every scheduled state due - DRAIN_STOPPING, // A tick whose first callback stops the client CLEANUP, // The protocol task's teardown - START, RESTART, }; struct Row { @@ -2953,16 +2899,11 @@ TEST(MetadataSlot, ThreeBlocksCarryEveryStateToTheListenerInOrder) { {"a scheduled one behind it", Step::SCHEDULED, "O", {}, nullptr, 2}, {"falling due in the first callback, it follows in the same tick", Step::DRAIN_FALLING_DUE, nullptr, {"N", "O"}, nullptr, 2}, - {"a due state", Step::DUE, "P", {}, nullptr, 2}, - {"a scheduled one behind it", Step::SCHEDULED, "Q", {}, nullptr, 2}, - {"stop() from the first callback leaves nothing behind", Step::DRAIN_STOPPING, nullptr, - {"P"}, nullptr, 3}, - {"a start", Step::START, nullptr, {}, nullptr, 3}, - {"a scheduled state", Step::SCHEDULED, "R", {}, nullptr, 3}, - {"waits", Step::DRAIN, nullptr, {}, "R", 3}, - {"a restart drops it", Step::RESTART, nullptr, {}, nullptr, 4}, - {"a due state after it", Step::DUE, "S", {}, nullptr, 4}, - {"is applied", Step::DRAIN, nullptr, {"S"}, nullptr, 4}, + {"a scheduled state", Step::SCHEDULED, "R", {}, nullptr, 2}, + {"waits", Step::DRAIN, nullptr, {}, "R", 2}, + {"a restart drops it", Step::RESTART, nullptr, {}, nullptr, 3}, + {"a due state after it", Step::DUE, "S", {}, nullptr, 3}, + {"is applied", Step::DRAIN, nullptr, {"S"}, nullptr, 3}, }; TitleLog log; @@ -3037,19 +2978,10 @@ TEST(MetadataSlot, ThreeBlocksCarryEveryStateToTheListenerInOrder) { client.loop(); set_clock(0.0); break; - case Step::DRAIN_STOPPING: - log.on_title = [&client](const std::string&) { - client.stop(); - }; - client.loop(); - break; case Step::CLEANUP: before_teardown = impl.cleanup_generation.load(); impl.cleanup(); break; - case Step::START: - start(); - break; case Step::RESTART: client.stop(); start(); @@ -3176,121 +3108,15 @@ TEST(ClientLifecycle, AFullTeardownKeepsTheDismissalsOfShownPrompts) { } } -/// Calls stop(), and then start() when `restart` is set, from inside the first -/// on_pairing_code(). The test thread plays the protocol task, so the listener runs the -/// final tick stop() would join first: admission closes, then the shutdown pass takes the -/// admitted connection's prompt flags. -class StopFromPairingPromptListener : public PairingPromptLog { -public: - StopFromPairingPromptListener(SendspinClient*& client, bool reenter, bool restart) - : client_(client), reenter_(reenter), restart_(restart) {} - - void on_pairing_code(std::string_view code, SendspinPairingCodeFormat format) override { - PairingPromptLog::on_pairing_code(code, format); - if (this->reenter_ && !this->reentered_) { - this->reentered_ = true; - this->client_->connection_manager_->close_admission(); - (void) this->client_->protocol_tick(); - this->client_->stop(); - if (this->restart_) { - this->restart_result = this->client_->start(); - } - } - } - - bool restart_result{false}; - -private: - SendspinClient*& client_; - bool reenter_; - bool restart_; - bool reentered_{false}; -}; - -// A pairing listener may call stop(), and start() after it, from inside the callback that shows a -// code. stop() dismisses the code and the pairing window it was shown under exactly once, from its -// own drain; the drain that showed the code abandons the rest of its batch, so the next round's -// code queued behind it is never shown. After a restart the next batch drains as any other. -// Control: without the re-entry both codes are shown and nothing is dismissed. -TEST(ClientLifecycle, StopFromInsideAPairingCallbackDismissesThePromptOnce) { - struct Row { - const char* name; - bool reenter; - bool restart; - int expected_displays; - int expected_dismissals; - }; - const Row rows[] = { - {"Control: no re-entry", false, false, 2, 0}, - {"stop() with the next round's code queued behind", true, false, 1, 1}, - {"stop() then start()", true, true, 1, 1}, - }; - for (const Row& row : rows) { - SCOPED_TRACE(row.name); - SendspinClient* client_ref = nullptr; - StopFromPairingPromptListener log(client_ref, row.reenter, row.restart); - TestNetworkProvider network; - SendspinClient client(make_config(0)); - client_ref = &client; - client.set_network_provider(&network); - client.set_listener(&log); - ASSERT_TRUE(client.start()); - client.protocol_task_->stop(); - - // An attempt under a pairing window the operator was shown, with its code emitted. - auto conn = std::make_shared(); - conn->pairing_session().window_shown = true; - conn->pairing_session().code_emitted = true; - client.connection_manager_->install_admitted(conn, 0); - client.note_open_pairing_window(); - client.loop(); - ASSERT_EQ(log.opens, 1); - - client.note_display_pairing_code("111111", SendspinPairingCodeFormat::DIGITS); - client.note_display_pairing_code("222222", SendspinPairingCodeFormat::DIGITS); - client.loop(); - if (row.restart) { - ASSERT_TRUE(log.restart_result) << "start() from inside the callback was refused"; - client.protocol_task_->stop(); - } - - EXPECT_EQ(log.displays, row.expected_displays); - EXPECT_EQ(log.clears, row.expected_dismissals); - EXPECT_EQ(log.closes, row.expected_dismissals); - client.loop(); // A later loop() delivers nothing the re-entry abandoned. - EXPECT_EQ(log.displays, row.expected_displays); - EXPECT_EQ(log.clears, row.expected_dismissals); - - if (row.restart) { - client.note_display_pairing_code("333333", SendspinPairingCodeFormat::DIGITS); - client.note_clear_pairing_code(); - client.loop(); - EXPECT_EQ(log.displays, row.expected_displays + 1); - EXPECT_EQ(log.clears, row.expected_dismissals + 1); - } - // stop()'s order with the task played here, for a run the re-entry did not end. - if (client.is_started()) { - client.connection_manager_->close_admission(); - (void) client.protocol_tick(); - } - client.stop(); - } -} - /// Logs each pairing callback with text, in the form "
". Overrides /// neither form itself: ViewPairingForms and WithDeprecatedPairingForms add them. Main loop only. class PairingFormsLog : public SendspinClientListener { public: std::vector calls; - /// When set, the first view-form callback calls its stop(). - SendspinClient* stop_from_view_form{nullptr}; protected: void log_view(const std::string& what, std::string_view text) { this->calls.push_back("view " + what + " " + std::string(text)); - if (SendspinClient* client = std::exchange(this->stop_from_view_form, nullptr)) { - client->stop(); - } } void log_deprecated(const std::string& what, const std::string& text) { this->calls.push_back("deprecated " + what + " " + text); @@ -3341,8 +3167,7 @@ using BothPairingForms = WithDeprecatedPairingForms; // Each pairing callback with text, every abort reason and a code in each format, reaches a // listener in the forms it overrides: only the view form, only the deprecated std::string form, -// or both, the view form first with the same content. A stop() from the first view-form callback -// ends the delivery there, its deprecated form included. The test thread plays the protocol task, +// or both, the view form first with the same content. The test thread plays the protocol task, // queueing the notes as its handlers do. TEST(ClientLifecycle, PairingCallbacksReachBothForms) { const std::string server_id = "c2VuZHNwaW4tcGFpcmluZy1jYWxsYmFjay1mb3Jtcw0"; @@ -3382,18 +3207,15 @@ TEST(ClientLifecycle, PairingCallbacksReachBothForms) { ViewPairingForms view_only; DeprecatedPairingForms deprecated_only; BothPairingForms both; - BothPairingForms stopping; struct Row { const char* name; PairingFormsLog& log; - bool stop; std::vector expected; }; const Row rows[] = { - {"only the view forms", view_only, false, in_form({"view"})}, - {"only the deprecated forms", deprecated_only, false, in_form({"deprecated"})}, - {"both, the view form first", both, false, in_form({"view", "deprecated"})}, - {"stop() from the first view form", stopping, true, {"view " + delivered.front()}}, + {"only the view forms", view_only, in_form({"view"})}, + {"only the deprecated forms", deprecated_only, in_form({"deprecated"})}, + {"both, the view form first", both, in_form({"view", "deprecated"})}, }; for (const Row& row : rows) { SCOPED_TRACE(row.name); @@ -3403,9 +3225,6 @@ TEST(ClientLifecycle, PairingCallbacksReachBothForms) { client.set_listener(&row.log); ASSERT_TRUE(client.start()); client.protocol_task_->stop(); - if (row.stop) { - row.log.stop_from_view_form = &client; - } client.note_pairing_started(server_id); client.note_pairing_succeeded(server_id); diff --git a/tests/test_encrypted_lifecycle.cpp b/tests/test_encrypted_lifecycle.cpp index 9ccfc993..faf0f497 100644 --- a/tests/test_encrypted_lifecycle.cpp +++ b/tests/test_encrypted_lifecycle.cpp @@ -2848,18 +2848,15 @@ class BothGroupForms : public NewGroupForm { } // namespace // Each state reaches on_track_metadata() and then the deprecated on_metadata(), so a listener -// overriding either gets it and one overriding both gets both, the new form first. A stop() from -// on_track_metadata() delivers the clear in the callback, whose views still read the state, and -// the deprecated form of the state the clear replaced does not follow. A teardown the protocol -// task runs during on_track_metadata() (the test thread playing it, through the role's cleanup()) -// does not stop the deprecated form: its clear comes on the next drain. +// overriding either gets it and one overriding both gets both, the new form first. A teardown +// the protocol task runs during on_track_metadata() (the test thread playing it, through the +// role's cleanup()) does not stop the deprecated form: its clear comes on the next drain. TEST(EncryptedLifecycle, EachMetadataListenerFormGetsTheStateNewFormFirst) { NewMetadataForm new_form; OldMetadataForm old_form; BothMetadataForms both_forms; - BothMetadataForms both_forms_stopping; BothMetadataForms both_forms_torn_down; - enum class Interrupt : uint8_t { NONE, STOP, TEARDOWN }; + enum class Interrupt : uint8_t { NONE, TEARDOWN }; struct Row { const char* what; MetadataRoleListener* listener; @@ -2877,12 +2874,6 @@ TEST(EncryptedLifecycle, EachMetadataListenerFormGetsTheStateNewFormFirst) { Interrupt::NONE, {"new Song", "old Song"}, {}}, - {"stop() from on_track_metadata()", - &both_forms_stopping, - &both_forms_stopping, - Interrupt::STOP, - {"new Song", "clear", "read Song"}, - {}}, {"the protocol task's teardown during on_track_metadata()", &both_forms_torn_down, &both_forms_torn_down, @@ -2895,9 +2886,7 @@ TEST(EncryptedLifecycle, EachMetadataListenerFormGetsTheStateNewFormFirst) { DispatchTestClient bundle("Metadata Form Test Client"); SendspinClient& client = bundle.client_ref(); client.metadata()->set_listener(row.listener); - if (row.interrupt == Interrupt::STOP) { - row.log->interrupt = [&client] { client.stop(); }; - } else if (row.interrupt == Interrupt::TEARDOWN) { + if (row.interrupt == Interrupt::TEARDOWN) { row.log->interrupt = [&client] { client.metadata()->impl_->cleanup(); }; } DispatchTestConnection& conn = bundle.connection(); @@ -2914,49 +2903,36 @@ TEST(EncryptedLifecycle, EachMetadataListenerFormGetsTheStateNewFormFirst) { } // The group twin of EachMetadataListenerFormGetsTheStateNewFormFirst: each group/update reaches -// on_group_state() and then the deprecated on_group_update(), and a stop() from on_group_state() -// clears the group state with the deprecated form of the update not following. +// on_group_state() and then the deprecated on_group_update(). TEST(EncryptedLifecycle, EachGroupListenerFormGetsTheUpdateNewFormFirst) { NewGroupForm new_form; OldGroupForm old_form; BothGroupForms both_forms; - BothGroupForms both_forms_stopping; struct Row { const char* what; SendspinClientListener* listener; FormLog* log; - bool stop; std::vector calls; }; const Row rows[] = { - {"on_group_state() alone", &new_form, &new_form, false, {"new Kitchen"}}, - {"on_group_update() alone", &old_form, &old_form, false, {"old Kitchen"}}, + {"on_group_state() alone", &new_form, &new_form, {"new Kitchen"}}, + {"on_group_update() alone", &old_form, &old_form, {"old Kitchen"}}, {"Control: both, the new form first", &both_forms, &both_forms, - false, {"new Kitchen", "old Kitchen"}}, - {"stop() from on_group_state()", - &both_forms_stopping, - &both_forms_stopping, - true, - {"new Kitchen", "read Kitchen"}}, }; for (const Row& row : rows) { SCOPED_TRACE(row.what); DispatchTestClient bundle("Group Form Test Client"); SendspinClient& client = bundle.client_ref(); client.set_listener(row.listener); - if (row.stop) { - row.log->interrupt = [&client] { client.stop(); }; - } DispatchTestConnection& conn = bundle.connection(); bundle.admit(conn); bundle.deliver(conn, R"({"type":"group/update","payload":{"group_name":"Kitchen"}})"); bundle.pump(); EXPECT_EQ(row.log->calls, row.calls); - EXPECT_EQ(client.group_state().group_name, - row.stop ? std::nullopt : std::optional("Kitchen")); + EXPECT_EQ(client.group_state().group_name, std::optional("Kitchen")); } } diff --git a/tests/test_source_role.cpp b/tests/test_source_role.cpp index 9497ed3e..63e5a042 100644 --- a/tests/test_source_role.cpp +++ b/tests/test_source_role.cpp @@ -1261,48 +1261,6 @@ TEST(SourceStream, StopEndsAnOpenStreamAheadOfTheGoodbye) { EXPECT_FALSE(rig.source->write_audio(piece.data(), piece.size(), 0)) << "accepted when stopped"; } -// docs/internals.md "Re-entrant Teardown During Callback Dispatch": a listener that -// stops the client from on_streaming_stopped() finds the stream's hold released by the time -// stop() returns, since the nested teardown has no stream left to release. -TEST(SourceStream, StopFromTheStoppedCallbackFindsTheHoldReleased) { - class ReleaseCounter : public SendspinClientListener { - public: - void on_release_high_performance() override { - ++this->releases; - } - int releases{0}; - }; - class StoppingListener : public SourceRoleListener { - public: - void on_streaming_stopped() override { - this->client->stop(); - this->ref_count_after_stop = this->client->high_performance_ref_count_; - this->releases_after_stop = this->counter->releases; - } - SendspinClient* client{nullptr}; - ReleaseCounter* counter{nullptr}; - int ref_count_after_stop{-1}; - int releases_after_stop{-1}; - }; - ReleaseCounter counter; - StoppingListener stopping; - SourceStandIn rig; - rig.client->set_listener(&counter); - stopping.client = rig.client.get(); - stopping.counter = &counter; - rig.source->set_listener(&stopping); - rig.deliver(source_command_json("start")); - rig.tick(); - ASSERT_EQ(rig.client->high_performance_ref_count_, 1); - - rig.deliver(source_command_json("stop")); - rig.tick(); - EXPECT_FALSE(rig.client->is_started()); - EXPECT_EQ(stopping.ref_count_after_stop, 0); - EXPECT_EQ(stopping.releases_after_stop, 1); - EXPECT_EQ(counter.releases, 1); -} - // ============================================================================ // FLAC // ============================================================================