Fix libdispatch over-resume crash in DispatchTimerSource.deinit - #138
Open
romansorochak wants to merge 2 commits into
Open
Fix libdispatch over-resume crash in DispatchTimerSource.deinit#138romansorochak wants to merge 2 commits into
romansorochak wants to merge 2 commits into
Conversation
deinit resumed the timer unconditionally to balance a suspended source. When the source is still activated — a pending retry whose owner is torn down — that resume is an over-resume and libdispatch aborts the process. Make the balancing resume conditional on the source actually being suspended, and guard the state check and change with a lock so activate()/suspend() from different queues cannot tear the suspend count. Adds two regression tests; without the fix the deallocation test crashes the test runner at DispatchTimerSource.deinit.
The fix introduced an NSLock, but `Core/Helpers/Lock.swift` already provides `UnfairLock`, which is what the rest of the library uses to guard shared state (NetworkingClient, AudioPlayerContext, AudioRendererContext, FrameFilterProcessor, Atomic). Switch to it and express the critical sections with `withLock` instead of manual lock/unlock pairs. No behaviour change: the state check and change still happen together under the lock, and deinit still resumes only a suspended source. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3 tasks
KevinTCoughlin
added a commit
to cascadiacollections/shoutkit
that referenced
this pull request
Sep 7, 2026
handleMediaServicesReset() replaced `player` with a fresh AudioPlayer and let the old one go. AudioStreaming 1.4.4's `AudioPlayer.deinit` closes only `audioPlayingEntry`, never `audioReadingEntry`, so an abandoned reading entry takes its RemoteAudioSource — and that source's pending retry timer — with it. `DispatchTimerSource.deinit` resumes the timer unconditionally, which for a timer still activated by a pending retry is an over-resume that aborts the process inside libdispatch. A reset landing while a stream is mid-retry (a dead station, a timeout) crashes on the way out. `stop()` alone doesn't close it: AudioStreaming tears the reading entry down on its own source queue, capturing the player weakly, so reassigning `player` on the next line leaves that block with nil. Ask for the teardown, then hold the discarded player alive for a second while it runs. Exposure here is one path — the engine is a singleton and every ordinary path (station switch, stop, replay) already goes through play/stop, both of which cancel the retrier. Closing it is free, and the failure it produces is a hard crash rather than a degraded stream. Upstream fix: dimitris-c/AudioStreaming#138. Once a release carrying it is pinned, this reduces back to a bare reassignment. Signed-off-by: Kevin T. Coughlin <706967+KevinTCoughlin@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.
Problem
DispatchTimerSource.deinitresumes the timer unconditionally:That resume is necessary when the source is suspended — libdispatch traps when a
suspended source is released. But it is an over-resume when the source is still
activated, and libdispatch traps on that just the same.
The activated case is reached on a normal path in this library.
Retrierschedules aone-shot timer and calls
timeoutTimer.activate()ininternalRetry(); the timer staysin
.activateduntil someone callscancel(). Whenever a failing stream is torn downwith a retry pending, the
Retrier— and with it theDispatchTimerSource— isdeallocated while the source is still running, and the process aborts.
Crash signature:
For context on how often this path is hit: in a radio app built on AudioStreaming this
was 62% of all iOS crash events, affecting 1.3% of weekly active users — dead and
flaky stream URLs put
Retrierto work constantly, and every teardown with a pendingretry was a coin flip.
Fix
Make the balancing resume conditional on the source actually being suspended, and take a
lock around the check-and-change in
activate()/suspend()so the suspend count cannotbe torn when those are called from different queues.
statebecomes a lock-protectedread-only property;
isRunningis unchanged.Tests
Two regression tests in
DispatchTimerSourceTests:test_DispatchTimerSource_Can_Be_Deallocated_While_Activated— deallocates an activatedsource. Reaching the end of the test is the assertion: against the current
mainthistest does not fail, it takes the whole test runner down at
DispatchTimerSource.deinit.test_DispatchTimerSource_Repeated_Activate_And_Suspend_Stay_Balanced— repeatedactivate()/suspend()calls move the suspend count exactly once each, including theno-op paths.
Notes
The change is confined to
DispatchTimerSource; no public API changes. We have beenrunning this patch in production on a pinned fork revision, and would rather have it
upstream than carry the fork.