Move RTMP publish/play authorization off the poll thread - #229
Draft
claude[bot] wants to merge 8 commits into
Draft
claude[bot] wants to merge 8 commits into
claude[bot] wants to merge 8 commits into
Conversation
authorize_publish/authorize_play do blocking SQLite work (and, for publish, Raft ownership acquisition when clustering is enabled). They used to run directly inside librtmp2's publish/play callbacks on the single RTMP poll thread, so one slow authorization -- lock contention, a busy disk, a Raft round trip -- stalled every other connection's handshake, publish, and play until it returned. auth_worker.rs adds a dedicated OS thread that owns this work instead. The publish/play callbacks (rtmp_publish_auth_cb/rtmp_play_auth_cb) submit a request and return librtmp2's new AuthorizationResult::Pending immediately; the worker calls the existing authorize_publish/authorize_play unchanged and reports the result on a completion channel, which the RTMP poll loop drains once per tick (drain_auth_completions, called from process_server_connections so both the production server and the test harness pick it up) via Server::complete_publish_authorization/ complete_play_authorization. A single worker thread keeps SQLite access serialized exactly as it already was under Db's connection mutex; the request queue is bounded (512) so a stuck DB fails new requests closed instead of queuing without limit. Existing DB/cluster authorization semantics, connection limits, and ownership checks are unchanged -- only where that work runs moved. The now-unused synchronous rtmp_publish_cb/rtmp_play_cb are removed. librtmp2's Cargo dependency is temporarily pinned to the branch adding AuthorizationResult/Pending (OpenRTMP/librtmp2#318) until that PR merges and a release containing it is published. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLht6yc8cp3bfPYYXo58k6
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Replace the sequential viewer_find_by_play_key() + stream_get() lookups in the play-authorization hot path with viewer_and_stream_by_play_key(), a single JOIN query. Both lookups were already indexed (stream_viewers. play_key is UNIQUE, streams.id is the primary key), so this removes a round trip and a lock acquisition rather than adding an index. Authorization semantics are unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLht6yc8cp3bfPYYXo58k6
Replace the three SELECT COUNT(*) FROM players WHERE viewer_id=? AND active=1 queries (in player_try_acquire, player_update's reactivation check, and player_active_count_for_viewer) with an in-memory counter (Db::active_player_counts) mutated in the same conn-lock critical section as the matching SQLite write, so it can never observably disagree with the players table. This removes a DB round trip from every viewer join and from the per-join cluster heartbeat check, without changing the per-viewer connection-cap semantics. SQLite remains the durable record: players rows are still inserted and updated exactly as before, and the counter starts empty on every Db::open, matching the existing startup reset of stale active rows (no session survives a process restart either way). players_deactivate_for_viewer now also zeroes the counter, which already covers the DeleteViewer cluster command. The Raft snapshot install path can drop an active player row outright (skipped rather than reinserted, when its stream or viewer didn't survive the snapshot) without ever calling player_update — install_app_snapshot now tracks those and resyncs the counter for their viewers after commit, so a concurrent viewer deletion during a snapshot install can't leave a stale +1 in the counter. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLht6yc8cp3bfPYYXo58k6
process_server_connections now reports whether it just applied a publish/play authorization completion from the auth worker (drain_auth_completions returns bool instead of ()). The poll loop uses that, alongside the existing "still negotiating" check, to pick the fast poll interval for the next tick -- so a client that sends its next protocol step (first media frame, a play command) right after receiving the onStatus reply doesn't wait out a full idle-interval sleep before the server reads it. Also drop POLL_INTERVAL_FAST_MS from 5ms to 1ms: it's already scoped to brief negotiation/join bursts (never steady-state traffic), so tightening it lowers per-step latency during those bursts without adding steady-state CPU cost. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLht6yc8cp3bfPYYXo58k6
Contributor
There was a problem hiding this comment.
dependencies -> librtmp2 Version set on Release
Contributor
Author
There was a problem hiding this comment.
That's intentional for now, not an oversight — this PR needs AuthorizationResult, on_publish_auth_cb/on_play_auth_cb, and complete_publish_authorization/complete_play_authorization from librtmp2#318, which hasn't shipped a crates.io release yet. I pinned to that PR's branch as a stopgap (see the comment above the dependency line) so this PR can build and be reviewed in the meantime.
Once #318 merges and a release containing it is published, I'll revert this to a normal version pin — I've noted that as a required follow-up. If you'd rather I hold off merging this PR until that release exists, or if you want to cut the librtmp2 release yourself once #318 lands, let me know; otherwise I'll handle the revert as soon as it's available.
Generated by Claude Code
Documents the async publish/play authorization work already on this branch: the dedicated auth worker thread, the single-JOIN play auth query, the in-memory viewer connection counter, and the immediate fast poll tick after an auth completion resolves. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLht6yc8cp3bfPYYXo58k6
Re-ran scripts/run_rtmp_benchmarks.sh (librtmp2-server vs nginx-rtmp vs MediaMTX) against the async-auth-worker-thread architecture from 0.4.0: handshake latency improved (avg 61.3ms -> 49.9ms), and the 100-viewer join-latency gap to nginx-rtmp narrowed from ~12.6ms to ~4ms, consistent with authorization moving off the RTMP poll thread and the per-viewer connection cap reading an in-memory counter instead of a per-join SELECT COUNT(*). Also documents a benign setup-phase rate-limit quirk found while re-running the script. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLht6yc8cp3bfPYYXo58k6
|
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
2 participants
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.
<!-- ccr-projects-attribution: {"github_login":"AlexanderWagnerDev"} -->
Requested by Alex · project thread
Depends on OpenRTMP/librtmp2#318, which is not yet merged. This PR's
Cargo.tomltemporarily pinslibrtmp2to that PR's branch so CI can build against the new API; it needs to be switched back to a crates.io version pin once #318 merges and is released. Marked draft for that reason, independent of review status here.Where synchronous blocking happened
authorize_publish/authorize_playonDbRtmpBridgeran directly inside librtmp2'spublish/playcommand callbacks (on_publish_cb/on_play_cb), which fire synchronously on the single RTMP poll thread. Both do blocking SQLite work (viewer_find_by_play_key,stream_get,player_try_acquire/publisher_update, all serialized behindDb's single-connection mutex), and publish additionally acquires Raft stream ownership when clustering is enabled. One slow authorization — lock contention, a busy disk, a Raft round trip to another node — stalled every other connection's handshake, publish, and play on the same server until it returned.What moved off that thread, and how it comes back
This PR does not change what
authorize_publish/authorize_playcheck — same DB lookups, same connection-limit enforcement, same cluster ownership/fencing rules, sameplayers/publisherspersistence. It changes where that work runs.src/auth_worker.rsis a new dedicated OS thread that owns exactly one job: pull a publish/play request off a bounded channel (capacity 512), call the existingauthorize_publish/authorize_playunchanged, and push the boolean result onto a completion channel. One thread keeps SQLite access serialized exactly as it already was underDb's connection mutex — a pool of workers would just queue on that same mutex — while guaranteeing the work never blocks the RTMP thread. A full queue fails the request closed (Deny) immediately rather than growing without bound or blocking the caller.The RTMP-side
publish/playcallbacks (rtmp_publish_auth_cb/rtmp_play_auth_cbinserver.rs) now submit to that worker and return librtmp2's newAuthorizationResult::Pendingimmediately (see #318) instead of blocking on the DB call inline.drain_auth_completions, called once per poll tick fromprocess_server_connections(shared by the production server and the test harness), applies every completion the worker has finished viaServer::complete_publish_authorization/complete_play_authorization— this resumes librtmp2's own publish/play state machine and sends the client'sonStatusreply. The now-unused synchronousrtmp_publish_cb/rtmp_play_cbare removed rather than left dead.A pending connection sends and receives no media in the meantime — librtmp2's
Pendingstate holds off the state-machine transition that would enable relay, so this falls out of the existing protocol layer rather than needing new gating here.Verified
cargo test --features test-support -- --test-threads=1(142 lib tests) andcargo test --test rtmp_http_e2e --features test-support -- --test-threads=1(5 tests, including a real RTMP publish+play round trip through the new async path) both pass unchanged, as doescargo test --lib --features cluster,test-support -- --test-threads=1(175 tests, covering cluster ownership through the worker).cargo clippy --all-targets --features test-support -- -D warningsandcargo fmt --checkare clean.🤖 Generated with Claude Code
https://claude.ai/code/session_01RLht6yc8cp3bfPYYXo58k6
Generated by Claude Code