fix(moq-net): release cached state when a producer is aborted or dropped#1715
Conversation
Aborting a producer only flipped the kio close flag and set the abort error; the cached collections in the shared state were left intact. Since kio keeps the state value alive as long as any handle exists, a stale Consumer that never drained would pin the entire cache forever: Track.groups -> GroupProducer -> Group.frames -> FrameProducer -> the heap-allocated frame buffers. Drop the cache as part of abort so the buffers are freed immediately regardless of lingering consumers: - TrackProducer::abort clears groups and duplicates. - GroupProducer::abort clears frames and resets cache. - BroadcastProducer / BroadcastDynamic::abort clear the track lookup (in addition to draining the owned dynamic requests). Consumers that haven't pulled yet now surface the abort error instead of the leftover cache. Child consumers that already pulled a handle (GroupConsumer / FrameConsumer) own independent state and can still finish reading.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThis PR extends cache clearing across the model hierarchy by adding it to both abort and drop paths. A new 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
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 |
…pped The abort() cleanup only ran on an explicit abort. Dropping all producers without aborting (e.g. a publisher task ending) just flipped kio's closed flag and left the cache intact, so a stale Consumer (or Weak, which pins the state via an Arc clone) kept the buffers alive forever, the same leak. Add Drop impls that treat the last producer dropping without a clean finish like an abort: clear the cache. A cleanly finished producer keeps its cache so consumers can still drain it. - TrackProducer::drop clears groups + duplicates when not finished. - GroupProducer::drop clears frames + resets cache when not finished. - BroadcastProducer::drop / BroadcastDynamic::drop clear the track lookup. Gated on a new kio Producer::is_last() so cleanup only runs for the final producer, not a dropped clone.
kixelated
changed the title
2 participants
Summary
Tearing down a moq-net producer left its cached state intact, so a stale
Consumercould pin the buffers in memory forever.kiokeeps the shared state value alive as long as any handle exists, and that includeskio::Weak(it skips bumping the producer/consumer counts but still holds anArcclone of the state lock, so it pins the value). A late/abandonedTrackConsumer/GroupConsumer, or aWeakheld in the broadcast track lookup, kept the whole cached tree alive:This happened on both teardown paths:
abort()) only flipped kio's close flag and stored the error; it never dropped the cache.Dropimpls doing any moq-net-level cleanup.Fix
Release the cache on teardown, treating an abrupt drop like an abort while preserving a clean finish:
On
abort():TrackProducer::abortclearsgroups+duplicates.GroupProducer::abortclearsframes+ resetscache.BroadcastProducer/BroadcastDynamic::abortclear the track lookup (on top of draining owned dynamic requests).On drop of the last producer, when not cleanly finished:
TrackProducer::dropclearsgroups+duplicates(kept iffinish()/finish_at()was called).GroupProducer::dropclearsframes+ resetscache(kept iffinish()was called).BroadcastProducer::drop/BroadcastDynamic::dropclear the track lookup.A new
kio::Producer::is_last()gates the drop cleanup so it only runs for the final producer, not a dropped clone.The frame layer needs no change: its payload lives in an
Arc<FrameBuf>held directly by the producer/consumer handles, not in the sharedkiostate, so it is released by refcount when the handles drop.Behavior
After abort, or after the last producer drops without a clean finish, a consumer that hasn't pulled yet surfaces the terminal error (the abort error, or
Error::Dropped) instead of draining the leftover cache. This matches teardown semantics and avoids serving a truncated stream as if complete. A cleanly finished producer keeps its cache so consumers can still drain it (e.g. a relay forwarding a finished track), and child consumers that already pulled a handle (GroupConsumer/FrameConsumer) own independent state and finish reading what they hold.Public API signatures and wire behavior are unchanged;
kio::Producer::is_last()is additive. Targetingmain; happy to retargetdevif reviewers consider the drop/abort drain behavior change disruptive.Test plan
cargo test -p kio -p moq-net(347 passed)cargo clippy -p kio -p moq-net --all-targetscleancargo test -p hang(unaffected)producer::is_last_tracks_producer_counttrack::abort_clears_cached_groups,group::abort_clears_cached_frames,broadcast::abort_clears_track_lookuptrack::{drop_unfinished_clears_cached_groups, drop_finished_keeps_cached_groups},group::{drop_unfinished_clears_cached_frames, drop_finished_keeps_cached_frames},broadcast::drop_clears_track_lookup(Written by Claude)