Improve AppKit NSEvent observation handling

source/Observatory/Observer/Event/Observer.Event.Handler.swift

@@ -18,37 +18,58 @@ extension EventObserver.Handler {
             public typealias Handler = (global: GlobalSignature?, local: LocalSignature?)
             public typealias Monitor = (global: Any?, local: Any?)
 
-            init(mask: NSEvent.EventTypeMask, handler: Handler) {
-                self.mask = mask
-                self.handler = handler
-            }
-
             deinit {
                 self.deactivate()
             }
 
+            internal init(monitoring: Monitoring? = nil, mask: NSEvent.EventTypeMask, handler: Handler) {
+                self.mask = mask
+                self.handler = handler
+                self.monitoring = monitoring ?? Monitoring.default
+            }
+
+            internal var monitoring: Monitoring
             public let mask: NSEvent.EventTypeMask
             public let handler: Handler
 
-            open private(set) var monitor: Monitor?
+            private let lock: NSRecursiveLock = NSRecursiveLock()
+            private var token: UInt = 0
 
+            open private(set) var monitor: Monitor?
             open private(set) var isActive: Bool = false
 
             @discardableResult open func activate(_ newValue: Bool = true) -> Self {
+                self.lock.lock()
+                defer { self.lock.unlock() }
+
                 if newValue == self.isActive { return self }
 
-                if newValue {
-                    self.monitor = Monitor(
-                        global: self.handler.global.map({ NSEvent.addGlobalMonitorForEvents(matching: self.mask, handler: $0) as Any }),
-                        local: self.handler.local.map({ NSEvent.addLocalMonitorForEvents(matching: self.mask, handler: $0) as Any })
-                    )
-                } else if let monitor = self.monitor {
-                    monitor.local.map({ NSEvent.removeMonitor($0) })
-                    monitor.global.map({ NSEvent.removeMonitor($0) })
-                    self.monitor = nil
-                }
+                self.token = self.token &+ 1
+                let token: UInt = self.token
 
+                // Assign here to support reentrancy.
                 self.isActive = newValue
+
+                let oldMonitor = self.monitor
+                let newMonitor = newValue ? Monitor(
+                    global: self.handler.global.flatMap({ handler in
+                        self.monitoring.observe(global: self.mask, { [weak self] in if let self, self.lock.withLock({ self.token }) == token { handler($0) } })
+                    }),
+                    local: self.handler.local.flatMap({ handler in
+                        self.monitoring.observe(local: self.mask, { [weak self] in if let self, self.lock.withLock({ self.token }) == token { handler($0) } else { $0 } })
+                    })
+                ) : nil
+
+                // If by some miracle a reentry happens – not 100% sure, but a few crashes point at this – this checks if the activation
+                // is stale and cleans up the redundant monitors in favor of recent ones.
+                if token != self.token {
+                    self.monitoring.unobserve(newMonitor)
+                    return self
+                }
+
+                self.monitor = newMonitor
+                self.monitoring.unobserve(oldMonitor)
+
                 return self
             }
 
@@ -59,6 +80,30 @@ extension EventObserver.Handler {
     }
 }
 
+extension EventObserver.Handler.AppKit.Definition {
+    /// This is a proxy for testing.
+    internal class Monitoring {
+        internal static let `default`: Monitoring = Monitoring()
+        internal init() {}
+        internal func observe(global mask: NSEvent.EventTypeMask, _ handler: @escaping (NSEvent) -> Void) -> Any? {
+            NSEvent.addGlobalMonitorForEvents(matching: mask, handler: { [weak self] in self?.receive(global: $0, handler: handler) })
+        }
+        internal func observe(local mask: NSEvent.EventTypeMask, _ handler: @escaping (NSEvent) -> NSEvent?) -> Any? {
+            NSEvent.addLocalMonitorForEvents(matching: mask, handler: { [weak self] in self?.receive(local: $0, handler: handler) })
+        }
+        internal func unobserve(_ monitor: Any) { NSEvent.removeMonitor(monitor) }
+        internal func receive(global event: NSEvent, handler: @escaping (NSEvent) -> Void){ handler(event) }
+        internal func receive(local event: NSEvent, handler: @escaping (NSEvent) -> NSEvent?) -> NSEvent? { handler(event) }
+    }
+}
+
+extension EventObserver.Handler.AppKit.Definition.Monitoring {
+    fileprivate func unobserve(_ monitor: EventObserver.Handler.AppKit.Definition.Monitor?) {
+        if let local = monitor?.local { self.unobserve(local) }
+        if let global = monitor?.global { self.unobserve(global) }
+    }
+}
+
 /// Convenience initializers.
 extension EventObserver.Handler.AppKit.Definition {
 

source/Observatory/Test/Observer/Test.Observer.Event.swift

@@ -1,53 +1,149 @@
+@testable import Observatory
 import AppKit
 import Carbon
 import Foundation
 import Nimble
-import Observatory
 import Quick
 
-/// Wow, this turned out to be a serious pain in the ass – testing events is not a joke… Doing it properly requires running
-/// a loop, as far as I understand there's no way testing global event dispatch, because, quoting, handler will not be called
-/// for events that are sent to your own application. Instead, we check that observers sets everything up correctly.
 internal class AppKitEventObserverSpec: Spec {
     override internal class func spec() {
-        it("can observe events in active state") {
-            let observer: EventObserver = EventObserver(active: true)
+        typealias Definition = EventObserver.Handler.AppKit.Definition
 
-            observer.add(mask: NSEvent.EventTypeMask.any, handler: {})
-            expect(observer.appKitDefinitions[0].handler.global) != nil
-            expect(observer.appKitDefinitions[0].handler.local) != nil
-            expect(observer.appKitDefinitions[0].monitor) != nil
+        it("can activate and deactivate definitions") {
+            let observer = EventObserver(active: false)
+            let globalMonitoring = AppKitSpecMonitoring()
+            let localMonitoring = AppKitSpecMonitoring()
+            let globalDefinition = Definition(monitoring: globalMonitoring, mask: .any, handler: (global: { _ in }, local: nil))
+            let localDefinition = Definition(monitoring: localMonitoring, mask: .any, handler: (global: nil, local: { event in event }))
 
-            observer.add(mask: NSEvent.EventTypeMask.any, global: {})
-            expect(observer.appKitDefinitions[1].handler.global) != nil
-            expect(observer.appKitDefinitions[1].handler.local) == nil
-            expect(observer.appKitDefinitions[1].monitor) != nil
+            observer.add(definition: globalDefinition)
+            observer.add(definition: localDefinition)
+            expect(globalDefinition.monitor) == nil
+            expect(localDefinition.monitor) == nil
 
-            observer.add(mask: NSEvent.EventTypeMask.any, local: {})
-            expect(observer.appKitDefinitions[2].handler.global) == nil
-            expect(observer.appKitDefinitions[2].handler.local) != nil
-            expect(observer.appKitDefinitions[2].monitor) != nil
+            observer.activate()
+            expect(globalMonitoring.count) == Count((local: 0, global: 1), 0)
+            expect(localMonitoring.count) == Count((local: 1, global: 0), 0)
+            expect(globalDefinition.monitor) != nil
+            expect(localDefinition.monitor) != nil
 
-            observer.isActive = false
+            observer.activate() // Repetitive activation does nothing…
+            expect(globalMonitoring.count) == Count((local: 0, global: 1), 0)
+            expect(localMonitoring.count) == Count((local: 1, global: 0), 0)
+            expect(globalDefinition.monitor) != nil
+            expect(localDefinition.monitor) != nil
+
+            observer.deactivate()
+            expect(globalMonitoring.count) == Count((local: 0, global: 1), 1)
+            expect(localMonitoring.count) == Count((local: 1, global: 0), 1)
+            expect(globalDefinition.monitor) == nil
+            expect(localDefinition.monitor) == nil
+
+            observer.deactivate() // Repetitive deactivation does nothing…
+            expect(globalMonitoring.count) == Count((local: 0, global: 1), 1)
+            expect(localMonitoring.count) == Count((local: 1, global: 0), 1)
+            expect(globalDefinition.monitor) == nil
+            expect(localDefinition.monitor) == nil
+        }
+
+        it("can ignore callbacks when deactivated") {
+            let monitoring = AppKitSpecMonitoring()
+            let globalObservation = Observation()
+            let localObservation = Observation()
+            let definition = Definition(monitoring: monitoring, mask: .leftMouseDown, handler: (
+                global: { _ in globalObservation.make() },
+                local: { event in localObservation.make(); return event }
+            ))
+
+            definition.activate(true)
+
+            let event = NSEvent.fake(type: .leftMouseDown)
+            _ = monitoring.localHandler?(event)
+            monitoring.globalHandler?(event)
+            globalObservation.assert(count: 1)
+            localObservation.assert(count: 1)
+
+            definition.activate(false)
+            let localResult = monitoring.localHandler?(event)
+            monitoring.globalHandler?(event)
+
+            expect(localResult === event) == true
+            globalObservation.assert(count: 0)
+            localObservation.assert(count: 0)
+        }
+
+        it("can deactivate on deinit") {
+            let monitoring = AppKitSpecMonitoring()
+            autoreleasepool {
+                var definition: Definition? = Definition(monitoring: monitoring, mask: .any, handler: (global: { _ in }, local: { event in event }))
+                definition?.activate(true)
+                expect(monitoring.count) == .init((local: 1, global: 1), 0)
+                definition = nil
+            }
+            expect(monitoring.count) == .init((local: 1, global: 1), 2)
+        }
+
+        it("can handle recursive activations and deactivations") {
+            let monitoring = AppKitSpecMonitoring()
+            let definition = Definition(monitoring: monitoring, mask: .appKitDefined, handler: (global: { _ in }, local: { event in event }))
+            var observeReentryCount = 0
+            var unobserveReentryCount = 0
+
+            monitoring.observeCallback = {
+                observeReentryCount += 1
+                definition.activate(true)
+            }
+
+            monitoring.unobserveCallback = {
+                unobserveReentryCount += 1
+                definition.activate(false)
+            }
+
+            definition.activate(true)
+            expect(definition.isActive) == true
+            expect(definition.monitor) != nil
+            expect(monitoring.count) == Count((local: 1, global: 1), 0)
+            definition.activate(false)
+
+            expect(observeReentryCount) == 2
+            expect(unobserveReentryCount) == 2
+            expect(monitoring.count) == Count((local: 1, global: 1), 2)
+            expect(definition.isActive) == false
+            expect(definition.monitor) == nil
+        }
+
+        it("can handle concurrent activations and deactivations") {
+            let monitoring = AppKitSpecMonitoring()
+            let definition = Definition(monitoring: monitoring, mask: .any, handler: (global: { _ in }, local: { event in event }))
+            let queue = DispatchQueue(label: "\(Self.self).mixed.concurrent", attributes: .concurrent)
+            let start = DispatchSemaphore(value: 0)
+            let group = DispatchGroup()
 
-            expect(observer.appKitDefinitions[0].monitor) == nil
-            expect(observer.appKitDefinitions[1].monitor) == nil
-            expect(observer.appKitDefinitions[2].monitor) == nil
+            for index in 0..<16 {
+                group.enter()
+                queue.async {
+                    start.wait()
+                    definition.activate(index % 2 == 0)
+                    group.leave()
+                }
+            }
 
-            observer.isActive = true
+            for _ in 0..<16 { start.signal() }
+            group.wait()
 
-            expect(observer.appKitDefinitions[0].monitor) != nil
-            expect(observer.appKitDefinitions[1].monitor) != nil
-            expect(observer.appKitDefinitions[2].monitor) != nil
+            definition.activate(false)
+            expect(definition.isActive) == false
+            expect(definition.monitor) == nil
         }
     }
+
 }
 
 internal class CarbonEventObserverSpec: Spec {
     override internal class func spec() {
         it("can observe events in active state") {
-            let observer: EventObserver = EventObserver(active: true)
-            let observation: Observation = Observation()
+            let observer = EventObserver(active: true)
+            let observation = Observation()
 
             observer.add(mask: NSEvent.EventTypeMask.leftMouseDown.union(.rightMouseDown).rawValue, handler: { observation.make() })
             Event.postMouseEvent(type: CGEventType.leftMouseDown)
@@ -63,3 +159,49 @@ internal class CarbonEventObserverSpec: Spec {
         }
     }
 }
+
+fileprivate struct Count: Equatable {
+    fileprivate init(_ observe: (local: Int, global: Int) = (0, 0), _ unobserve: Int = 0) {
+        self.observe = observe
+        self.unobserve = unobserve
+    }
+    fileprivate var observe: (local: Int, global: Int) = (0, 0)
+    fileprivate var unobserve: Int = 0
+    fileprivate static func == (lhs: Count, rhs: Count) -> Bool { lhs.observe == rhs.observe && lhs.unobserve == rhs.unobserve }
+}
+
+private final class AppKitSpecMonitoring: EventObserver.Handler.AppKit.Definition.Monitoring {
+    internal var count: Count = Count()
+
+    internal var globalHandler: ((NSEvent) -> Void)?
+    internal var localHandler: ((NSEvent) -> NSEvent?)?
+    internal var observeCallback: (() -> Void)?
+    internal var unobserveCallback: (() -> Void)?
+
+    override internal func observe(global mask: NSEvent.EventTypeMask, _ handler: @escaping (NSEvent) -> Void) -> Any? {
+        self.count.observe.global += 1
+        self.globalHandler = handler
+        self.observeCallback?()
+        return NSObject()
+    }
+
+    override internal func observe(local mask: NSEvent.EventTypeMask, _ handler: @escaping (NSEvent) -> NSEvent?) -> Any? {
+        self.count.observe.local += 1
+        self.localHandler = handler
+        self.observeCallback?()
+        return NSObject()
+    }
+
+    override internal func unobserve(_ monitor: Any) {
+        self.count.unobserve += 1
+        self.unobserveCallback?()
+    }
+}
+
+extension NSEvent {
+    fileprivate static func fake(type: CGEventType) -> NSEvent {
+        guard let event = CGEvent(mouseEventSource: nil, mouseType: type, mouseCursorPosition: CGPoint(x: -10000, y: -10000), mouseButton: .center) else { preconditionFailure("Cannot construct fake CGEvent.") }
+        guard let event = NSEvent(cgEvent: event) else { preconditionFailure("Cannot construct fake NSEvent.") }
+        return event
+    }
+}