diff --git a/AudioStreaming/Core/Helpers/DispatchTimerSource.swift b/AudioStreaming/Core/Helpers/DispatchTimerSource.swift index 2865384..18e10ca 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 = UnfairLock() + private var _state: SourceState = .suspended /// The state of the timer enum SourceState { @@ -19,6 +20,10 @@ 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.withLock { _state } } + var isRunning: Bool { state == .activated } @@ -39,8 +44,19 @@ 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. + let needsResume = lock.withLock { () -> Bool in + let wasSuspended = _state == .suspended + _state = .activated + return wasSuspended + } + + if needsResume { + timer.resume() + } } /// Adds an event handler to the timer. @@ -58,16 +74,20 @@ final class DispatchTimerSource { /// Activates the timer, if needed func activate() { - if state == .activated { return } - state = .activated - timer.resume() + lock.withLock { + guard _state == .suspended else { return } + _state = .activated + timer.resume() + } } /// Suspends the timer, if needed. func suspend() { - if state == .suspended { return } - state = .suspended - timer.suspend() + lock.withLock { + guard _state == .activated else { return } + _state = .suspended + timer.suspend() + } } func schedule(interval: DispatchTimeInterval, repeats: Bool) { 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")