Add an outbound binary send primitive to all transports - #113
Conversation
ea0ad1c to
2d30ccf
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The ESP hot path still allocates, includes a shutdown leak, and lacks synchronized exactly-once verification.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds outbound binary messaging across all connection transports to support the upcoming source role.
Changes:
- Adds
send_binary_message()to the connection interface and four transports. - Implements a reusable single-in-flight ESP server send slot.
- Adds host transport delivery tests and
NOT_FINISHED.
File summaries
| File | Description |
|---|---|
src/connection.h |
Defines the binary-send contract. |
src/platform/types.h |
Adds the busy-operation error code. |
src/host/client_connection.h |
Declares host-client binary sending. |
src/host/client_connection.cpp |
Implements host-client binary sending. |
src/host/server_connection.h |
Declares host-server binary sending. |
src/host/server_connection.cpp |
Implements host-server binary sending. |
src/esp/client_connection.h |
Declares ESP-client binary sending. |
src/esp/client_connection.cpp |
Implements synchronous ESP-client sending. |
src/esp/server_connection.h |
Defines ESP-server send-slot state. |
src/esp/server_connection.cpp |
Implements queued ESP-server binary sending. |
tests/test_connection_lifecycle.cpp |
Tests host binary-frame delivery. |
Review details
Suppressed comments (2)
src/esp/server_connection.cpp:243
- The first binary chunk also allocates the lookup control block in the per-chunk path, so pre-sizing the payload alone would still violate the allocation-free hot-path rule in
docs/conventions.md:67-68. Create this reusable context during connection setup (with an explicit allocation-failure path) rather than lazily insend_binary_message().
if (this->binary_send_lookup_ == nullptr) {
this->binary_send_lookup_ = std::make_shared<BinarySendLookup>();
this->binary_send_lookup_->conn =
std::static_pointer_cast<SendspinServerConnection>(this->shared_from_this());
tests/test_connection_lifecycle.cpp:768
- Like the client-side test, this asserts immediately after the first frame and can miss a duplicate delivered just afterward. Synchronize with a fully drained/stopped peer before asserting the final frame count so the test actually protects the exactly-once behavior.
{
const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(5);
while (binary_frames.load() == 0 && std::chrono::steady_clock::now() < deadline) {
std::this_thread::sleep_for(std::chrono::milliseconds(5));
}
}
ASSERT_EQ(binary_frames.load(), 1);
- Files reviewed: 11/11 changed files
- Comments generated: 5
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// While a work item is queued, `self` keeps the block alive independently of the connection; | ||
| /// the worker moves `self` into a local before resolving `conn`, so teardown with work in | ||
| /// flight makes the worker a clean no-op. If httpd stops and discards queued work, the engaged | ||
| /// `self` cycle leaks this small block (bounded by client shutdowns); the destructor still | ||
| /// fails the pending completion. |
There was a problem hiding this comment.
Fixed in 914e365: engaged lookup blocks are tracked in a registry the worker leaves on entry, and reclaim_orphaned_binary_send_work() breaks the remaining cycles from the ws server's stop path once httpd_stop guarantees no queued worker can run.
| // Grow-only buffer sized by the first payload: chunks are near-constant size, so steady | ||
| // state allocates nothing and any growth is loud. SPIRAM-preferred like the receive buffer. | ||
| if (this->binary_send_payload_.size() < len) { | ||
| bool grown; | ||
| if (this->binary_send_payload_.data() == nullptr) { | ||
| grown = this->binary_send_payload_.allocate(len, MemoryLocation::PREFER_EXTERNAL); | ||
| } else { | ||
| SS_LOGW(TAG, "Growing binary send slot %zu -> %zu bytes", | ||
| this->binary_send_payload_.size(), len); | ||
| grown = this->binary_send_payload_.realloc(len); |
There was a problem hiding this comment.
Declining the pre-sized-at-setup ask: the connection is generic transport and cannot know a role's chunk size, and lazy first-use sizing with loud growth is this library's established pattern — the player's decode buffer is allocated at the first codec header and reallocated on Opus growth (sync_task.cpp). Steady state allocates nothing; the once-per-connection lookup block now moves to the constructor in 914e365.
| { | ||
| const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(5); | ||
| while (binary_frames.load() == 0 && std::chrono::steady_clock::now() < deadline) { | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(5)); | ||
| } | ||
| } | ||
| ASSERT_EQ(binary_frames.load(), 1); |
There was a problem hiding this comment.
Fixed in 914e365: after the first frame both tests close the sender and drain until the peer observes the close before re-asserting the exactly-once count.
| /// @param data Pointer to the message bytes (type byte first). | ||
| /// @param len Length of the message in bytes. | ||
| /// @param cb Callback invoked with the send result. |
There was a problem hiding this comment.
Fixed in 914e365: the interface doc now enumerates the callback's execution contexts (inline, httpd worker, destructor thread) and forbids thread-affine work.
| void SendspinServerConnection::async_send_binary(void* arg) { | ||
| auto* lookup = static_cast<BinarySendLookup*>(arg); | ||
| // Take the keep-alive back first; a successful lock() then blocks destruction until return | ||
| std::shared_ptr<BinarySendLookup> keep = std::move(lookup->self); | ||
| auto conn = lookup->conn.lock(); |
There was a problem hiding this comment.
Fixed in 842d16a (part 6, where the stack's documentation lands): the queued-worker section now covers async_send_binary, the reusable slot, its completion contract, and the reclamation path.
Track engaged binary send lookups in a registry reclaimed after httpd_stop so discarded work cannot strand keep-alive cycles across server restarts, allocate the lookup block at connection construction instead of on the first send, document the completion callback's execution contexts on the interface, and drain the peer's close before the wire tests re-assert their exactly-once counts.
Part 2/6 of the source@v1 stack (tracker: #95).
What it adds:
send_binary_message()on the connection interface and all four transports (ESP client/server, host client/server). The ESP server transport gets a single-in-flight send slot that is allocation-free in steady state; the completion callback fires exactly once per call.How it's used: part 3's source task calls it once per audio chunk. No callers yet in this part, so no behavior changes.