Merge pull request #195 from davidstump/dr/heartbeat-timer-crash

Refactor HeartbeatTimer to guarantee thread safety

Sources/SwiftPhoenixClient/Defaults.swift

@@ -57,6 +57,9 @@ public class Defaults {
       else { return nil }
     return json
   }
+
+  public static let heartbeatQueue: DispatchQueue
+    = DispatchQueue(label: "com.phoenix.socket.heartbeat")
 }
 
 

Sources/SwiftPhoenixClient/HeartbeatTimer.swift

@@ -20,80 +20,104 @@
 
 import Foundation
 
-class HeartbeatTimer: Equatable {
-    let timeInterval: TimeInterval
-    let dispatchQueue: DispatchQueue
-    let id: String = UUID().uuidString
-    init(timeInterval: TimeInterval, dispatchQueue: DispatchQueue) {
-        self.timeInterval = timeInterval
-        self.dispatchQueue = dispatchQueue
-    }
-
-    private lazy var timer: DispatchSourceTimer = {
-        let t = DispatchSource.makeTimerSource(flags: [], queue: self.dispatchQueue)
-        t.schedule(deadline: .now() + self.timeInterval, repeating: self.timeInterval)
-        t.setEventHandler(handler: { [weak self] in
-            self?.eventHandler?()
-        })
-        return t
-    }()
-
-    var isValid: Bool {
-        return state == .resumed
-    }
-
-    private var eventHandler: (() -> Void)?
-
-    private enum State {
-        case suspended
-        case resumed
-    }
-    private var state: State = .suspended
-
-
-    func startTimerWithEvent(eventHandler: (() -> Void)?) {
-        self.eventHandler = eventHandler
-        resume()
-    }
-
-    func stopTimer() {
-        timer.setEventHandler {}
-        eventHandler = nil
-        suspend()
-    }
-
-    private func resume() {
-        if state == .resumed {
-            return
-        }
-        state = .resumed
-        timer.resume()
-    }
-
-    private func suspend() {
-        if state == .suspended {
-            return
-        }
-        state = .suspended
-        timer.suspend()
-    }
-
-    func fire() {
-        eventHandler?()
-    }
-
-    deinit {
-        timer.setEventHandler {}
-        timer.cancel()
-        /*
-         If the timer is suspended, calling cancel without resuming
-         triggers a crash. This is documented here https://forums.developer.apple.com/thread/15902
-         */
-        resume()
-        eventHandler = nil
+
+
+/**
+ Heartbeat Timer class which manages the lifecycle of the underlying
+ timer which triggers when a hearbeat should be fired. This heartbeat
+ runs on it's own Queue so that it does not interfere with the main
+ queue but guarantees thread safety.
+ */
+
+class HeartbeatTimer {
+
+  //----------------------------------------------------------------------
+  // MARK: - Dependencies
+  //----------------------------------------------------------------------
+  // The interval to wait before firing the Timer
+  let timeInterval: TimeInterval
+
+  // The DispatchQueue to schedule the timers on
+  let queue: DispatchQueue
+
+  // UUID which specifies the Timer instance. Verifies that timers are different
+  let uuid: String = UUID().uuidString
+
+  //----------------------------------------------------------------------
+  // MARK: - Properties
+  //----------------------------------------------------------------------
+  // The underlying, cancelable, resettable, timer.
+  private var temporaryTimer: DispatchSourceTimer? = nil
+  // The event handler that is called by the timer when it fires.
+  private var temporaryEventHandler: (() -> Void)? = nil
+
+  /**
+   Create a new HeartbeatTimer
+
+   - Parameter timeInterval: Interval to fire the timer. Repeats
+   - parameter queue: Queue to schedule the timer on
+   */
+  init(timeInterval: TimeInterval, queue: DispatchQueue) {
+    self.timeInterval = timeInterval
+    self.queue = queue
+  }
+
+  /**
+   Create a new HeartbeatTimer
+
+   - Parameter timeInterval: Interval to fire the timer. Repeats
+   */
+  convenience init(timeInterval: TimeInterval) {
+    self.init(timeInterval: timeInterval, queue: Defaults.heartbeatQueue)
+  }
+
+  func start(eventHandler: @escaping () -> Void) {
+    queue.sync {
+      // Create a new DispatchSourceTimer, passing the event handler
+      let timer = DispatchSource.makeTimerSource(flags: [], queue: queue)
+      timer.setEventHandler(handler: eventHandler)
+
+      // Schedule the timer to first fire in `timeInterval` and then
+      // repeat every `timeInterval`
+      timer.schedule(deadline: DispatchTime.now() + self.timeInterval,
+                     repeating: self.timeInterval)
+
+      // Start the timer
+      timer.resume()
+      self.temporaryEventHandler = eventHandler
+      self.temporaryTimer = timer
     }
-
-    static func == (lhs: HeartbeatTimer, rhs: HeartbeatTimer) -> Bool {
-        return lhs.id == rhs.id
+  }
+
+  func stop() {
+    // Must be queued synchronously to prevent threading issues.
+    queue.sync {
+      // DispatchSourceTimer will automatically cancel when released
+      temporaryTimer = nil
+      temporaryEventHandler = nil
     }
+  }
+
+  /**
+   True if the Timer exists and has not been cancelled. False otherwise
+   */
+  var isValid: Bool {
+    guard let timer = self.temporaryTimer else { return false }
+    return !timer.isCancelled
+  }
+
+  /**
+   Calls the Timer's event handler immediately. This method
+   is primarily used in tests (not ideal)
+   */
+  func fire() {
+    guard isValid else { return }
+    self.temporaryEventHandler?()
+  }
+}
+
+extension HeartbeatTimer: Equatable {
+  static func == (lhs: HeartbeatTimer, rhs: HeartbeatTimer) -> Bool {
+    return lhs.uuid == rhs.uuid
+  }
 }

Sources/SwiftPhoenixClient/Socket.swift

@@ -142,9 +142,6 @@ public class Socket: PhoenixTransportDelegate {
 
   /// Ref counter for messages
   var ref: UInt64 = UInt64.min // 0 (max: 18,446,744,073,709,551,615)
-
-  ///Queue to run heartbeat timer on
-  var heartbeatQueue: DispatchQueue = DispatchQueue(label: "com.phoenix.socket.heartbeat")
 
   /// Timer that triggers sending new Heartbeat messages
   var heartbeatTimer: HeartbeatTimer?
@@ -274,8 +271,7 @@ public class Socket: PhoenixTransportDelegate {
     self.connection = nil
 
     // The socket connection has been torndown, heartbeats are not needed
-    self.heartbeatTimer?.stopTimer()
-    self.heartbeatTimer = nil
+    self.heartbeatTimer?.stop()
 
     // Since the connection's delegate was nil'd out, inform all state
     // callbacks that the connection has closed
@@ -594,8 +590,7 @@ public class Socket: PhoenixTransportDelegate {
     self.triggerChannelError()
 
     // Prevent the heartbeat from triggering if the
-    self.heartbeatTimer?.stopTimer()
-    self.heartbeatTimer = nil
+    self.heartbeatTimer?.stop()
 
     // Only attempt to reconnect if the socket did not close normally
     if (!self.closeWasClean) {
@@ -712,15 +707,14 @@ public class Socket: PhoenixTransportDelegate {
   internal func resetHeartbeat() {
     // Clear anything related to the heartbeat
     self.pendingHeartbeatRef = nil
-    self.heartbeatTimer?.stopTimer()
-    self.heartbeatTimer = nil
-
+    self.heartbeatTimer?.stop()
+
     // Do not start up the heartbeat timer if skipHeartbeat is true
     guard !skipHeartbeat else { return }
 
-    self.heartbeatTimer = HeartbeatTimer(timeInterval: heartbeatInterval, dispatchQueue: heartbeatQueue)
-    self.heartbeatTimer?.startTimerWithEvent(eventHandler: { [weak self] in
-        self?.sendHeartbeat()
+    self.heartbeatTimer = HeartbeatTimer(timeInterval: heartbeatInterval)
+    self.heartbeatTimer?.start(eventHandler: { [weak self] in
+      self?.sendHeartbeat()
     })
   }
 

SwiftPhoenixClient.xcodeproj/project.pbxproj

@@ -48,6 +48,7 @@
 		6338892125D1E72000A05817 /* StarscreamTransport.swift in Sources */ = {isa = PBXBuildFile; fileRef = 631D1892259CE2A8005FAC0E /* StarscreamTransport.swift */; };
 		635669C4261631DC0068B665 /* URLSessionTransportSpec.swift in Sources */ = {isa = PBXBuildFile; fileRef = 635669C3261631DC0068B665 /* URLSessionTransportSpec.swift */; };
 		6397FE8A259967E9005E66C3 /* RxSwiftPhoenixClient.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 128143D425411361008615A7 /* RxSwiftPhoenixClient.framework */; };
+		63ACBE9426D53DF500171582 /* HeartbeatTimerSpec.swift in Sources */ = {isa = PBXBuildFile; fileRef = 63ACBE9326D53DF500171582 /* HeartbeatTimerSpec.swift */; };
 		63B526852656CF9600289719 /* Starscream.xcframework in Frameworks */ = {isa = PBXBuildFile; fileRef = 63B526842656CF9600289719 /* Starscream.xcframework */; };
 		63B5268E2656CFFE00289719 /* RxSwift.xcframework in Frameworks */ = {isa = PBXBuildFile; fileRef = 63B5268D2656CFFE00289719 /* RxSwift.xcframework */; };
 		63B526992656D05300289719 /* RxSwift.xcframework in Frameworks */ = {isa = PBXBuildFile; fileRef = 63B5268D2656CFFE00289719 /* RxSwift.xcframework */; };
@@ -189,6 +190,7 @@
 		631D1886259CE1D7005FAC0E /* StarscreamSwiftPhoenixClient.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = StarscreamSwiftPhoenixClient.framework; sourceTree = BUILT_PRODUCTS_DIR; };
 		631D1892259CE2A8005FAC0E /* StarscreamTransport.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = StarscreamTransport.swift; sourceTree = "<group>"; };
 		635669C3261631DC0068B665 /* URLSessionTransportSpec.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = URLSessionTransportSpec.swift; sourceTree = "<group>"; };
+		63ACBE9326D53DF500171582 /* HeartbeatTimerSpec.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = HeartbeatTimerSpec.swift; sourceTree = "<group>"; };
 		63B526842656CF9600289719 /* Starscream.xcframework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.xcframework; name = Starscream.xcframework; path = Carthage/Build/Starscream.xcframework; sourceTree = "<group>"; };
 		63B5268D2656CFFE00289719 /* RxSwift.xcframework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.xcframework; name = RxSwift.xcframework; path = Carthage/Build/RxSwift.xcframework; sourceTree = "<group>"; };
 		63B526DC2656D53700289719 /* Nimble.xcframework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.xcframework; name = Nimble.xcframework; path = Carthage/Build/Nimble.xcframework; sourceTree = "<group>"; };
@@ -390,6 +392,7 @@
 				12EF620225524B6800A6EE9B /* TimeoutTimerSpec.swift */,
 				12EF620325524B6800A6EE9B /* DefaultSerializerSpec.swift */,
 				635669C3261631DC0068B665 /* URLSessionTransportSpec.swift */,
+				63ACBE9326D53DF500171582 /* HeartbeatTimerSpec.swift */,
 			);
 			path = SwiftPhoenixClientTests;
 			sourceTree = "<group>";
@@ -677,6 +680,7 @@
 			buildActionMask = 2147483647;
 			files = (
 				12EF620825524B6800A6EE9B /* DefaultSerializerSpec.swift in Sources */,
+				63ACBE9426D53DF500171582 /* HeartbeatTimerSpec.swift in Sources */,
 				12991E11254C456F00BB8650 /* FakeTimerQueue.swift in Sources */,
 				12991E15254C45FC00BB8650 /* FakeTimerQueueSpec.swift in Sources */,
 				635669C4261631DC0068B665 /* URLSessionTransportSpec.swift in Sources */,

Tests/Mocks/MockableClass.generated.swift

@@ -645,11 +645,6 @@ class SocketMock: Socket {
         set(value) { underlyingRef = value }
     }
     var underlyingRef: (UInt64)!
-    override var heartbeatQueue: DispatchQueue {
-        get { return underlyingHeartbeatQueue }
-        set(value) { underlyingHeartbeatQueue = value }
-    }
-    var underlyingHeartbeatQueue: (DispatchQueue)!
     var heartbeatTimerSetCount: Int = 0
     var heartbeatTimerDidGetSet: Bool { return heartbeatTimerSetCount > 0 }
     override var heartbeatTimer: HeartbeatTimer? {

Tests/SwiftPhoenixClientTests/HeartbeatTimerSpec.swift

@@ -0,0 +1,74 @@
+//
+//  HeartbeatTimerSpec.swift
+//  SwiftPhoenixClientTests
+//
+//  Created by Daniel Rees on 8/24/21.
+//  Copyright © 2021 SwiftPhoenixClient. All rights reserved.
+//
+
+import Quick
+import Nimble
+@testable import SwiftPhoenixClient
+
+class HeartbeatTimerSpec: QuickSpec {
+
+  override func spec() {
+
+    let queue = DispatchQueue(label: "heartbeat.timer.spec")
+    var timer: HeartbeatTimer!
+
+
+    beforeEach {
+      timer = HeartbeatTimer(timeInterval: 10, queue: queue)
+    }
+
+    describe("isValid") {
+      it("returns false if is not started") {
+        expect(timer.isValid).to(beFalse())
+      }
+
+      it("returns true if the timer has started") {
+        timer.start { /* no-op */ }
+        expect(timer.isValid).to(beTrue())
+      }
+
+      it("returns false if timer has been stopped") {
+        timer.start { /* no-op */ }
+        timer.stop()
+        expect(timer.isValid).to(beFalse())
+      }
+    }
+
+    describe("fire") {
+      it("calls the event handler") {
+        var timerCalled = 0
+        timer.start { timerCalled += 1 }
+        expect(timerCalled).to(equal(0))
+
+        timer.fire()
+        expect(timerCalled).to(equal(1))
+      }
+
+      it("does not call event handler if stopped") {
+        var timerCalled = 0
+        timer.start { timerCalled += 1 }
+        expect(timerCalled).to(equal(0))
+
+        timer.stop()
+        timer.fire()
+        expect(timerCalled).to(equal(0))
+      }
+    }
+
+    describe("equatable") {
+      it("equates different timers correctly", closure: {
+        let timerA = HeartbeatTimer(timeInterval: 10, queue: queue)
+        let timerB = HeartbeatTimer(timeInterval: 10, queue: queue)
+
+        expect(timerA).to(equal(timerA))
+        expect(timerA).toNot(equal(timerB))
+
+      })
+    }
+  }
+}

Tests/SwiftPhoenixClientTests/SocketSpec.swift

@@ -318,18 +318,17 @@ class SocketSpec: QuickSpec {
 
       })
 
-      it("invalidates and releases the heartbeat timer", closure: {
+      it("invalidates and invalidates the heartbeat timer", closure: {
         var timerCalled = 0
-        let timer = HeartbeatTimer(timeInterval: 10, dispatchQueue: .main)
-
-        timer.startTimerWithEvent {
-          timerCalled += 1
-        }
-
+        let queue = DispatchQueue(label: "test.heartbeat")
+        let timer = HeartbeatTimer(timeInterval: 10, queue: queue)
+
+        timer.start { timerCalled += 1 }
+
         socket.heartbeatTimer = timer
 
         socket.disconnect()
-        expect(socket.heartbeatTimer).to(beNil())
+        expect(socket.heartbeatTimer?.isValid).to(beFalse())
         timer.fire()
         expect(timerCalled).to(equal(0))
       })
@@ -686,9 +685,11 @@ class SocketSpec: QuickSpec {
 
       it("should invalidate an old timer and create a new one", closure: {
         mockWebSocket.readyState = .open
+        let queue = DispatchQueue(label: "test.heartbeat")
+        let timer = HeartbeatTimer(timeInterval: 1000, queue: queue)
 
-        let timer = HeartbeatTimer(timeInterval: 1000, dispatchQueue: .main)
-        timer.startTimerWithEvent { }
+        var timerCalled = 0
+        timer.start { timerCalled += 1 }
         socket.heartbeatTimer = timer
 
         expect(timer.isValid).to(beTrue())