Repository navigation
Document that callbacks never call loop() - #259
Merged
Merged
Conversation
loop() runs every main-loop callback and does not support being re-entered, so the public loop() doc, the integration guide's Thread Safety Summary and conventions.md now say no listener, role or provider callback calls it, and internals.md notes that the only drain nested beneath another is the one stop() runs. Closes #257.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues were identified for this documentation-only clarification of callback restrictions.
No open issues 🎉
What changed in this PR
This documentation-only PR clarifies that callbacks must never call SendspinClient::loop(), addressing #257 and documenting the non-reentrancy contract relied on by the scheduled-state drain from #258. No runtime behavior changes.
Changes:
- State the callback restriction in the public API and integration guide.
- Clarify that re-entrant teardown remains supported, with
stop()providing the only nested drain.
| File | Description |
|---|---|
| include/sendspin/client.h | Documents the callback restriction on loop(). |
| docs/internals.md | Clarifies why only stop() can run a nested drain. |
| docs/integration-guide.md | Explains the restriction for main-loop and background callbacks. |
| docs/conventions.md | Excludes loop() from permitted callback re-entry. |
Review effort: Balanced
kahrendt
added a commit
that referenced
this pull request
Nov 11, 2026
## Summary No listener, role or provider callback may call `start()` or `stop()`, the same rule #259 set for `loop()` and enforced the same way (documentation, no runtime check). With all three off-limits, no drain ever runs beneath another and no role's main-loop teardown half runs while one of its callbacks is on the stack. That removes the only reason for the re-entrancy guards in the drain, so they go. A consumer that wants to stop or restart in response to a callback records the request and acts on it between two `loop()` calls, which is what the ESPHome hub already does: it calls `stop()`/`start()` only from its own `loop()` and `on_shutdown()`. ## Change Removed, each of which only served a callback that calls `stop()` (and maybe `start()`) beneath the drain that fired it: - `EventState::drain_generation`, its bump in `stop()`, and every check in `drain_inbox()`: the event-ring skip and per-event abort, the pairing-note checks, the gates before each role drain and the group drain, and the check before the deprecated `on_group_update()`. - The pairing notes' move-out-and-back plus the `reserve()` a nested drain needed. The drain uses its buffer in place. The internals Memory row for the extra buffer goes too. - `start()`'s STOPPING branch and its warning, which only a callback inside `stop()` could reach. The STOPPING state stays: other threads, and `connect_to()`/`disconnect()` from callbacks inside `stop()`, still need `is_started()` to read false. - The `accepts(generation)` checks after player, artwork and controller callbacks and at the head of their drains and `drain_scheduled_state()`, then `RoleTeardown::accepts()` itself. - Between a drain's catch-up and those head checks only the clear callback runs, so they only caught a callback calling `stop()`. - A protocol-task teardown in that window now gives "state, then clear on the next drain". That is the outcome it already gave when it landed just after the check. - The player's indexed loop, `teardown_reentered` and the erase clamp. The awaiting events change only from the ring drain and the catch-up, neither of which can run inside the loop. - Metadata's `TeardownTracker::completed()` check before the deprecated `on_metadata()`, and `completed()` itself. The protocol task's teardowns stay caught by the generation stamps (`keep_current_payload()`, `event_is_current()`, `ScheduledStateSlot`'s sequence check), which are unchanged. One take did depend on the removed checks: the player's STREAM_START took whatever stream parameters were pending and dropped them on a stamp mismatch. A teardown during an earlier callback in the same drain, plus the next connection's `stream/start`, left the next START reading the previous stream's parameters. The new `GenerationSlot::take_stamped()` takes a payload only under the given stamp and leaves a later one pending. The player takes its parameters under the START's stamp, and the visualizer its config the same way. Docs: `client.h` (`start()`, `stop()`, `LifecycleState`), integration guide (Stopping and Restarting, Thread Safety Summary, string_view section), conventions.md, the house-patterns checklist, and internals.md (the re-entrant section becomes "Callbacks During Dispatch", plus `stop()` steps 0 and 6). Also removed, from what #261 added for pairing: the skip before each deprecated pairing callback and the nested-drain comments on its text buffer. ## Tests - Deleted `StopFromInsideADrainCallbackIsSafe`, `StopFromInsideAPairingCallbackDismissesThePromptOnce` and `StopFromTheStoppedCallbackFindsTheHoldReleased`. - Dropped the stop/start rows from `MetadataSlot`, the "stop() from ..." rows from the metadata and group listener-form tables, and the "stop() from the first view form" row from `PairingCallbacksReachBothForms`. - `CallbackDuringStopCannotRecurse` becomes `CallbackDuringStopSeesTheStoppedClient`: it keeps the stopped-state reads and now checks the request slot, so a `connect_to()` or `disconnect()` the callback posted fails it (mutant: drop `connect_to()`'s running check). - New `TeardownReorder.AStreamStartTakesOnlyItsOwnStreamParams`: a teardown plus the next stream's `stream/start` during `on_volume_changed()`, with a control row. A mutant restoring the unconditional take makes the next stream's `on_stream_start()` read no sample rate. - The metadata-form row where the protocol task tears down during `on_track_metadata()` stays as the control: the deprecated form still follows. - ASan, TSan and the allocation tests run clean, and an ESP-IDF 5.5 esp32s3 build of `tests/esp_idf` compiles the library without warnings.
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.
Summary
Documents that no callback may call
loop(). Every main-loop callback already runs inside one of the client's own calls (loop(),start(),stop()or its destructor), andloop()does not support being re-entered. The scheduled-state drain from #258 relies on this: a callback that re-enteredloop()between its peek and commit could commit a stale decision. Closes #257.Change
include/sendspin/client.h: theloop()doc states the rule and points at the integration guide.docs/integration-guide.md, Thread Safety Summary: the rule and why, including callbacks on other threads.docs/conventions.md: the re-entrant-teardown rule namesloop()as the one client method a callback does not call.docs/internals.md, "Re-entrant Teardown During Callback Dispatch": the only drain nested beneath another is the onestop()runs.Tests
Documentation only; no behaviour changes, and nothing enforces the rule at runtime (conventions.md "Contracts are documented and enforced by documentation, not by defensive code").