From 2b5b01ca9660c545b33775aaa9e4bdb4cb620b11 Mon Sep 17 00:00:00 2001 From: Roman Sorochak Date: Tue, 4 Aug 2026 17:57:04 +0200 Subject: [PATCH 1/2] Fix libdispatch over-resume crash in DispatchTimerSource MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../Core/Helpers/DispatchTimerSource.swift | 39 +++++++++++++++---- .../Core/DispatchTimerSourceTests.swift | 28 +++++++++++++ 2 files changed, 60 insertions(+), 7 deletions(-) diff --git a/AudioStreaming/Core/Helpers/DispatchTimerSource.swift b/AudioStreaming/Core/Helpers/DispatchTimerSource.swift index 2865384..ce3492d 100644 --- a/AudioStreaming/Core/Helpers/DispatchTimerSource.swift +++ b/AudioStreaming/Core/Helpers/DispatchTimerSource.swift @@ -11,7 +11,8 @@ import Foundation final class DispatchTimerSource { private var handler: (() -> Void)? private let timer: DispatchSourceTimer - var state: SourceState = .suspended + private let lock = NSLock() + private var _state: SourceState = .suspended /// The state of the timer enum SourceState { @@ -19,6 +20,14 @@ final class DispatchTimerSource { case suspended } + /// Read-only: the suspend/resume balance is only correct when the state check and the state + /// change happen together under the lock, which is what `activate()`/`suspend()` do. + var state: SourceState { + lock.lock() + defer { lock.unlock() } + return _state + } + var isRunning: Bool { state == .activated } @@ -39,8 +48,18 @@ final class DispatchTimerSource { deinit { timer.setEventHandler(handler: nil) timer.cancel() - // balance called of cancel/resume to avoid crashes - timer.resume() + // A suspended dispatch source traps if it is released without being resumed first — but + // resuming one that is already running is an over-resume, which traps just the same. The + // balancing resume therefore has to be conditional: this object is routinely deallocated + // while its timer is still activated, e.g. a pending retry whose owner goes away. + lock.lock() + let needsResume = _state == .suspended + _state = .activated + lock.unlock() + + if needsResume { + timer.resume() + } } /// Adds an event handler to the timer. @@ -58,15 +77,21 @@ final class DispatchTimerSource { /// Activates the timer, if needed func activate() { - if state == .activated { return } - state = .activated + lock.lock() + defer { lock.unlock() } + + guard _state == .suspended else { return } + _state = .activated timer.resume() } /// Suspends the timer, if needed. func suspend() { - if state == .suspended { return } - state = .suspended + lock.lock() + defer { lock.unlock() } + + guard _state == .activated else { return } + _state = .suspended timer.suspend() } diff --git a/AudioStreamingTests/Core/DispatchTimerSourceTests.swift b/AudioStreamingTests/Core/DispatchTimerSourceTests.swift index 6eef01a..5d272e7 100644 --- a/AudioStreamingTests/Core/DispatchTimerSourceTests.swift +++ b/AudioStreamingTests/Core/DispatchTimerSourceTests.swift @@ -66,6 +66,34 @@ class DispatchTimerSourceTests: XCTestCase { timerSource?.removeHandler() } + func test_DispatchTimerSource_Can_Be_Deallocated_While_Activated() { + // `deinit` used to resume unconditionally to balance a suspended source. When the source + // was still activated — a pending retry whose owner is torn down — that extra resume is an + // over-resume and libdispatch traps on it. Reaching the end of this test is the assertion. + var source: DispatchTimerSource? = DispatchTimerSource(interval: .milliseconds(100), queue: dispatchQueue) + source?.activate() + XCTAssertTrue(source!.isRunning) + + source = nil + XCTAssertNil(source) + } + + func test_DispatchTimerSource_Repeated_Activate_And_Suspend_Stay_Balanced() { + // Each activate/suspend must move the suspend count exactly once, no matter how often the + // no-op path is taken. + timerSource?.activate() + timerSource?.activate() + XCTAssertTrue(timerSource!.isRunning) + + timerSource?.suspend() + timerSource?.suspend() + XCTAssertFalse(timerSource!.isRunning) + + timerSource?.activate() + XCTAssertTrue(timerSource!.isRunning) + timerSource?.suspend() + } + func test_HandlerIsExecuted_On_The_Specified_Queue() { let expectaction = expectation(description: "fired") From c712f871accd16146b88520b6552e4d1ce098d7d Mon Sep 17 00:00:00 2001 From: Roman Sorochak Date: Mon, 7 Sep 2026 14:18:54 +0200 Subject: [PATCH 2/2] Use the library's UnfairLock instead of NSLock in DispatchTimerSource 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 --- .../Core/Helpers/DispatchTimerSource.swift | 39 ++++++++----------- 1 file changed, 17 insertions(+), 22 deletions(-) diff --git a/AudioStreaming/Core/Helpers/DispatchTimerSource.swift b/AudioStreaming/Core/Helpers/DispatchTimerSource.swift index ce3492d..18e10ca 100644 --- a/AudioStreaming/Core/Helpers/DispatchTimerSource.swift +++ b/AudioStreaming/Core/Helpers/DispatchTimerSource.swift @@ -11,7 +11,7 @@ import Foundation final class DispatchTimerSource { private var handler: (() -> Void)? private let timer: DispatchSourceTimer - private let lock = NSLock() + private let lock = UnfairLock() private var _state: SourceState = .suspended /// The state of the timer @@ -22,11 +22,7 @@ final class DispatchTimerSource { /// Read-only: the suspend/resume balance is only correct when the state check and the state /// change happen together under the lock, which is what `activate()`/`suspend()` do. - var state: SourceState { - lock.lock() - defer { lock.unlock() } - return _state - } + var state: SourceState { lock.withLock { _state } } var isRunning: Bool { state == .activated @@ -52,10 +48,11 @@ final class DispatchTimerSource { // resuming one that is already running is an over-resume, which traps just the same. The // balancing resume therefore has to be conditional: this object is routinely deallocated // while its timer is still activated, e.g. a pending retry whose owner goes away. - lock.lock() - let needsResume = _state == .suspended - _state = .activated - lock.unlock() + let needsResume = lock.withLock { () -> Bool in + let wasSuspended = _state == .suspended + _state = .activated + return wasSuspended + } if needsResume { timer.resume() @@ -77,22 +74,20 @@ final class DispatchTimerSource { /// Activates the timer, if needed func activate() { - lock.lock() - defer { lock.unlock() } - - guard _state == .suspended else { return } - _state = .activated - timer.resume() + lock.withLock { + guard _state == .suspended else { return } + _state = .activated + timer.resume() + } } /// Suspends the timer, if needed. func suspend() { - lock.lock() - defer { lock.unlock() } - - guard _state == .activated else { return } - _state = .suspended - timer.suspend() + lock.withLock { + guard _state == .activated else { return } + _state = .suspended + timer.suspend() + } } func schedule(interval: DispatchTimeInterval, repeats: Bool) {