(fix): show alternative using atomic property for thread safe notification center (#304)

Author: thomaszurkan-optimizely

Sources/Implementation/DefaultNotificationCenter.swift

@@ -17,33 +17,60 @@
 import Foundation
 
 public class DefaultNotificationCenter: OPTNotificationCenter {
-    public var notificationId: Int = 1
-    var notificationListeners = [Int: (Int, GenericListener)]()
+    public var notificationId: Int {
+        get {
+            return notifications.notificationId
+        }
+        set {
+            // don't do it.
+        }
+    }
+    
+    class Notifications {
+        var notificationId: Int = 1
+        var notificationListeners = [Int: (Int, GenericListener)]()
+        func incrementNotificationId() -> Int {
+            let returnValue = notificationId
+            notificationId += 1
+            return returnValue
+        }
+    }
+    
+    private var _listeners : AtomicProperty<Notifications> = AtomicProperty<Notifications>()
+    
+    var notifications : Notifications {
+        get {
+            return _listeners.property!
+        }
+        set {
+            _listeners.property = newValue;
+        }
+    }
 
     var observerLogEvent: NSObjectProtocol?
 
     required public init() {
         addInternalNotificationListners()
+        notifications = Notifications()
     }
 
     deinit {
         removeInternalNotificationListners()
     }
 
-    internal func incrementNotificationId() -> Int {
-        let returnValue = notificationId
-        notificationId += 1
-        return returnValue
-    }
-
     public func addGenericNotificationListener(notificationType: Int, listener: @escaping GenericListener) -> Int? {
-        notificationListeners[notificationId] = (notificationType, listener)
 
-        return incrementNotificationId()
+        var id = 0
+        _listeners.performAtomic { (notifications) in
+            notifications.notificationListeners[notifications.notificationId] = (notificationType, listener)
+            
+            id = notifications.incrementNotificationId()
+        }
+        return id
     }
 
     public func addActivateNotificationListener(activateListener: @escaping (OptimizelyExperimentData, String, OptimizelyAttributes?, OptimizelyVariationData, [String: Any]) -> Void) -> Int? {
-        notificationListeners[notificationId] = (NotificationType.activate.rawValue, { (args: Any...) in
+        let listener = { (args: Any...) in
             guard let myArgs = args[0] as? [Any?] else {
                 return
             }
@@ -62,13 +89,13 @@ public class DefaultNotificationCenter: OPTNotificationCenter {
 
                 activateListener(experimentData, userId, attributes, variationData, event)
             }
-        })
+        }
 
-        return incrementNotificationId()
+        return addGenericNotificationListener(notificationType: NotificationType.activate.rawValue, listener: listener)
     }
 
     public func addTrackNotificationListener(trackListener: @escaping (String, String, OptimizelyAttributes?, [String: Any]?, [String: Any]) -> Void) -> Int? {
-        notificationListeners[notificationId] = (NotificationType.track.rawValue, { (args: Any...) in
+        let listener = { (args: Any...) in
             guard let myArgs = args[0] as? [Any?] else {
                 return
             }
@@ -82,13 +109,13 @@ public class DefaultNotificationCenter: OPTNotificationCenter {
                 let event = myArgs[4] as? [String: Any] {
                 trackListener(eventKey, userId, attributes, eventTags, event)
             }
-        })
+        }
 
-        return incrementNotificationId()
+        return addGenericNotificationListener(notificationType: NotificationType.track.rawValue, listener: listener)
     }
 
     public func addDecisionNotificationListener(decisionListener: @escaping (String, String, OptimizelyAttributes?, [String: Any]) -> Void) -> Int? {
-        notificationListeners[notificationId] = (NotificationType.decision.rawValue, { (args: Any...) in
+        let listener = { (args: Any...) in
             guard let myArgs = args[0] as? [Any?] else {
                 return
             }
@@ -101,13 +128,13 @@ public class DefaultNotificationCenter: OPTNotificationCenter {
                 let decisionInfo = myArgs[3] as? [String: Any] {
                 decisionListener(type, userId, attributes, decisionInfo)
             }
-        })
+        }
 
-        return incrementNotificationId()
+        return addGenericNotificationListener(notificationType: NotificationType.decision.rawValue, listener: listener)
     }
 
     public func addDatafileChangeNotificationListener(datafileListener: @escaping DatafileChangeListener) -> Int? {
-        notificationListeners[notificationId] = (NotificationType.datafileChange.rawValue, { (args: Any...) in
+        let listener = { (args: Any...) in
             guard let myArgs = args[0] as? [Any?] else {
                 return
             }
@@ -117,13 +144,13 @@ public class DefaultNotificationCenter: OPTNotificationCenter {
             if let data = myArgs[0] as? Data {
                 datafileListener(data)
             }
-        })
+        }
 
-        return incrementNotificationId()
+        return addGenericNotificationListener(notificationType: NotificationType.datafileChange.rawValue, listener: listener)
     }
 
     public func addLogEventNotificationListener(logEventListener: @escaping LogEventListener) -> Int? {
-        notificationListeners[notificationId] = (NotificationType.logEvent.rawValue, { (args: Any...) in
+        let listener = { (args: Any...) in
             guard let myArgs = args[0] as? [Any?] else {
                 return
             }
@@ -134,26 +161,37 @@ public class DefaultNotificationCenter: OPTNotificationCenter {
                 let event = myArgs[1] as? [String: Any] {
                 logEventListener(url, event)
             }
-        })
+        }
 
-        return incrementNotificationId()
+        return addGenericNotificationListener(notificationType: NotificationType.logEvent.rawValue, listener: listener)
     }
 
     public func removeNotificationListener(notificationId: Int) {
-        self.notificationListeners.removeValue(forKey: notificationId)
+        _listeners.performAtomic { (listeners) in
+            listeners.notificationListeners.removeValue(forKey: notificationId)
+        }
     }
 
     public func clearNotificationListeners(type: NotificationType) {
-        self.notificationListeners = self.notificationListeners.filter({$1.0 != type.rawValue})
+        _listeners.performAtomic { (listeners) in
+            listeners.notificationListeners = listeners.notificationListeners.filter({$1.0 != type.rawValue})
+        }
     }
 
     public func clearAllNotificationListeners() {
-        self.notificationListeners.removeAll()
+        _listeners.performAtomic { (listeners) in
+            listeners.notificationListeners.removeAll()
+        }
     }
 
     public func sendNotifications(type: Int, args: [Any?]) {
-        for values in notificationListeners.values where values.0 == type {
-            values.1(args)
+        var selected = [GenericListener]()
+        _listeners.performAtomic { (listeners) in
+            selected = listeners.notificationListeners.values.filter{ $0.0 == type }.map{ $0.1 }
+        }
+
+        for listener in selected {
+            listener(args)
         }
     }
 

Tests/OptimizelyTests-Common/NotificationCenterTests.swift

@@ -18,7 +18,7 @@ import XCTest
 
 class NotificationCenterTests: XCTestCase {
 
-    let notificationCenter: DefaultNotificationCenter = DefaultNotificationCenter()
+    var notificationCenter: DefaultNotificationCenter!
     var experiment: Experiment?
     var variation: Variation?
     var called = false
@@ -38,20 +38,22 @@ class NotificationCenterTests: XCTestCase {
                                             "forcedVariations": ["12345": "1234567890"]]
 
     override func setUp() {
-        // Put setup code here. This method is called before the invocation of each test method in the class.
+         super.setUp()
 
         let data: [String: Any] = NotificationCenterTests.sampleExperiment
 
         experiment = try! OTUtils.model(from: data)
 
         variation = experiment!.variations[0]
 
+        notificationCenter = DefaultNotificationCenter()
         notificationCenter.clearAllNotificationListeners()
     }
 
     override func tearDown() {
-        // Put teardown code here. This method is called after the invocation of each test method in the class.
         notificationCenter.clearAllNotificationListeners()
+        notificationCenter = nil  // deinit immediately after each test
+        super.tearDown()
     }
 
     func sendActivate() {
@@ -241,13 +243,126 @@ class NotificationCenterTests: XCTestCase {
         XCTAssertTrue(called)
     }
 
+    func testNotificationCenterThreadSafe() {
+        let numConcurrency = 5
+        
+        let exp = expectation(description: "x")
+        exp.expectedFulfillmentCount = numConcurrency
+
+        DispatchQueue.global().asyncAfter(deadline: .now() + .microseconds(0)) {
+            _ = self.addActivateListener()
+            for _ in 0..<100000 { self.sendActivate() }
+            exp.fulfill()
+        }
+        DispatchQueue.global().asyncAfter(deadline: .now() + .microseconds(100)) {
+            _ = self.addTrackListener()
+            for _ in 0..<10000 { self.sendTrack() }
+            exp.fulfill()
+        }
+        DispatchQueue.global().asyncAfter(deadline: .now() + .microseconds(200)) {
+            _ = self.addDecisionListener()
+            for _ in 0..<1000 { self.sendDecision() }
+            exp.fulfill()
+        }
+        DispatchQueue.global().asyncAfter(deadline: .now() + .microseconds(10)) {
+            _ = self.addDatafileChangeListener()
+            exp.fulfill()
+        }
+        DispatchQueue.global().asyncAfter(deadline: .now() + .microseconds(20)) {
+            _ = self.addLogEventListener()
+            exp.fulfill()
+        }
 
+        wait(for: [exp], timeout: 10.0)
+        XCTAssertEqual(notificationCenter.notificationId - 1, numConcurrency)
+    }
+    
+    func testNotificationCenterThreadSafe_Remove() {
+        let numConcurrency = 5
+        
+        let exp = expectation(description: "x")
+        exp.expectedFulfillmentCount = numConcurrency
 
-    func testPerformanceExample() {
-        // This is an example of a performance test case.
-        self.measure {
-            // Put the code you want to measure the time of here.
+        DispatchQueue.global().asyncAfter(deadline: .now() + .microseconds(0)) {
+            _ = self.addActivateListener()
+            for _ in 0..<100 { self.sendActivate() }
+            self.notificationCenter.clearNotificationListeners(type: .activate)
+            exp.fulfill()
         }
+        DispatchQueue.global().asyncAfter(deadline: .now() + .microseconds(10)) {
+            _ = self.addTrackListener()
+            for _ in 0..<100 { self.sendTrack() }
+            self.notificationCenter.clearNotificationListeners(type: .track)
+            exp.fulfill()
+        }
+        DispatchQueue.global().asyncAfter(deadline: .now() + .microseconds(10)) {
+            _ = self.addDecisionListener()!
+            for _ in 0..<100 { self.sendDecision() }
+            self.notificationCenter.clearNotificationListeners(type: .decision)
+            exp.fulfill()
+        }
+        DispatchQueue.global().asyncAfter(deadline: .now() + .microseconds(10)) {
+            let id = self.addDatafileChangeListener()!
+            for _ in 0..<100 { self.sendDatafileChange() }
+            self.notificationCenter.removeNotificationListener(notificationId: id)
+            exp.fulfill()
+        }
+        DispatchQueue.global().asyncAfter(deadline: .now() + .microseconds(10)) {
+            let id = self.addLogEventListener()!
+            for _ in 0..<100 { self.sendLogEvent() }
+            self.notificationCenter.removeNotificationListener(notificationId: id)
+            exp.fulfill()
+        }
+
+        wait(for: [exp], timeout: 10.0)
+        
+        self.called = false
+        
+        sendActivate()
+        sendTrack()
+        sendDecision()
+        sendDatafileChange()
+        sendLogEvent()
+
+        XCTAssertFalse(called)
     }
+    
+    func testNotificationCenterThreadSafe_AddRemove() {
+        let numConcurrency = 3
+        
+        let exp = expectation(description: "x")
+        exp.expectedFulfillmentCount = numConcurrency
 
+        DispatchQueue.global().asyncAfter(deadline: .now() + .microseconds(0)) {
+            for _ in 0..<1000 {
+                let id = self.addActivateListener()!
+                self.notificationCenter.removeNotificationListener(notificationId: id)
+            }
+            exp.fulfill()
+        }
+        DispatchQueue.global().asyncAfter(deadline: .now() + .microseconds(10)) {
+            for _ in 0..<1000 {
+                let id = self.addTrackListener()!
+                self.notificationCenter.removeNotificationListener(notificationId: id)
+            }
+            exp.fulfill()
+        }
+        DispatchQueue.global().asyncAfter(deadline: .now() + .microseconds(10)) {
+            for _ in 0..<1000 {
+                let id = self.addDecisionListener()!
+                self.notificationCenter.removeNotificationListener(notificationId: id)
+            }
+            exp.fulfill()
+        }
+
+        wait(for: [exp], timeout: 10.0)
+
+        self.called = false
+        
+        sendActivate()
+        sendTrack()
+
+        XCTAssertFalse(called)
+    }
+    
 }