Turn ForegroundPOSCatalogSyncDispatcher into actor to guard against threading issues

staskus · staskus · commit 3be4a8b5839a · 2025-10-16T14:56:13.000+03:00
diff --git a/WooCommerce/Classes/Tools/BackgroundTasks/ForegroundPOSCatalogSyncDispatcher.swift b/WooCommerce/Classes/Tools/BackgroundTasks/ForegroundPOSCatalogSyncDispatcher.swift
@@ -7,7 +7,7 @@ import Experiments
 /// Periodically syncs POS catalog while the app is in the foreground.
 /// Triggers `catalogCoordinator.performSmartSync()` every hour when active.
 ///
-final class ForegroundPOSCatalogSyncDispatcher {
+final actor ForegroundPOSCatalogSyncDispatcher {
     private enum Constants {
         static let syncInterval: TimeInterval = 60 * 60 // 1 hour
         static let initialSyncDelay: TimeInterval = 5 // 5 seconds after becoming active
@@ -19,7 +19,7 @@ final class ForegroundPOSCatalogSyncDispatcher {
     private let timerProvider: DispatchTimerProviding
     private let featureFlagService: FeatureFlagService
     private let stores: StoresManager
-    private let isAppActive: () -> Bool
+    private let isAppActive: () async -> Bool
     private var observers: [NSObjectProtocol] = []
     private var timer: DispatchTimerProtocol?
 
@@ -33,7 +33,7 @@ final class ForegroundPOSCatalogSyncDispatcher {
          timerProvider: DispatchTimerProviding = DefaultDispatchTimerProvider(),
          featureFlagService: FeatureFlagService = ServiceLocator.featureFlagService,
          stores: StoresManager = ServiceLocator.stores,
-         isAppActive: @escaping () -> Bool = { UIApplication.shared.applicationState == .active }) {
+         isAppActive: @escaping () async -> Bool = UIApplication.isApplicationActive) {
         self.interval = interval
         self.notificationCenter = notificationCenter
         self.timerProvider = timerProvider
@@ -42,11 +42,7 @@ final class ForegroundPOSCatalogSyncDispatcher {
         self.isAppActive = isAppActive
     }
 
-    deinit {
-        stop()
-    }
-
-    func start() {
+    func start() async {
         guard featureFlagService.isFeatureFlagEnabled(.pointOfSaleLocalCatalogi1) else {
             return
         }
@@ -65,28 +61,34 @@ final class ForegroundPOSCatalogSyncDispatcher {
             object: nil,
             queue: .main
         ) { [weak self] _ in
-            self?.startTimer()
+            Task {
+                await self?.startTimer()
+            }
         }
 
         let backgroundObserver = notificationCenter.addObserver(
             forName: UIApplication.didEnterBackgroundNotification,
             object: nil,
             queue: .main
         ) { [weak self] _ in
-            self?.stopTimer()
+            Task {
+                await self?.stopTimer()
+            }
         }
 
         let defaultSiteObserver = notificationCenter.addObserver(
             forName: .StoresManagerDidUpdateDefaultSite,
             object: nil,
             queue: .main
         ) { [weak self] _ in
-            self?.stop()
+            Task {
+                await self?.stop()
+            }
         }
 
         observers = [activeObserver, backgroundObserver, defaultSiteObserver]
 
-        if isAppActive() {
+        if await isAppActive() {
             startTimer()
         }
 
@@ -112,7 +114,9 @@ final class ForegroundPOSCatalogSyncDispatcher {
         let timer = timerProvider.makeTimer(queue: queue)
         timer.schedule(deadline: .now() + Constants.initialSyncDelay, repeating: interval, leeway: .seconds(Constants.leeway))
         timer.setEventHandler { [weak self] in
-            self?.performSync()
+            Task {
+                await self?.performSync()
+            }
         }
         timer.resume()
         self.timer = timer
@@ -211,3 +215,9 @@ struct DefaultDispatchTimerProvider: DispatchTimerProviding {
         DispatchSource.makeTimerSource(queue: queue).asTimer()
     }
 }
+
+extension UIApplication {
+    static func isApplicationActive() async -> Bool {
+        shared.applicationState == .active
+    }
+}
diff --git a/WooCommerce/Classes/ViewRelated/MainTabBarController.swift b/WooCommerce/Classes/ViewRelated/MainTabBarController.swift
@@ -722,7 +722,7 @@ private extension MainTabBarController {
             viewModel.loadHubMenuTabBadge()
 
             // Begin foreground synchronization if POS tab becomes visible
-            isPOSTabVisible ? posSyncDispatcher.start() : posSyncDispatcher.stop()
+            await isPOSTabVisible ? posSyncDispatcher.start() : posSyncDispatcher.stop()
         }
     }
 
diff --git a/WooCommerce/WooCommerceTests/Tools/ForegroundPOSCatalogSyncDispatcherTests.swift b/WooCommerce/WooCommerceTests/Tools/ForegroundPOSCatalogSyncDispatcherTests.swift
@@ -2,6 +2,7 @@ import Testing
 import Foundation
 import Yosemite
 @testable import WooCommerce
+import UIKit
 
 @MainActor
 struct ForegroundPOSCatalogSyncDispatcherTests {
@@ -29,7 +30,7 @@ struct ForegroundPOSCatalogSyncDispatcherTests {
     @Test
     func start_whenFeatureFlagEnabled_createsTimer() async throws {
         // When
-        sut.start()
+        await sut.start()
 
         // Then
         #expect(timerProvider.createdTimers.count == 1)
@@ -39,18 +40,10 @@ struct ForegroundPOSCatalogSyncDispatcherTests {
     @Test
     func start_whenFeatureFlagDisabled_doesNotCreateTimer() async throws {
         // Given
-        let disabledFlags = MockFeatureFlagService()
-        disabledFlags.isFeatureFlagEnabledReturnValue[.pointOfSaleLocalCatalogi1] = false
-        let dispatcher = ForegroundPOSCatalogSyncDispatcher(
-            notificationCenter: notificationCenter,
-            timerProvider: timerProvider,
-            featureFlagService: disabledFlags,
-            stores: stores,
-            isAppActive: { true }
-        )
+        featureFlags.isFeatureFlagEnabledReturnValue[.pointOfSaleLocalCatalogi1] = false
 
         // When
-        dispatcher.start()
+        await sut.start()
 
         // Then
         #expect(timerProvider.createdTimers.isEmpty)
@@ -62,7 +55,7 @@ struct ForegroundPOSCatalogSyncDispatcherTests {
         let coordinator = MockPOSCatalogSyncCoordinator()
         stores.testPOSCatalogSyncCoordinator = coordinator
 
-        sut.start()
+        await sut.start()
         let timer = try #require(timerProvider.createdTimers.first)
 
         // When - fire timer and wait for sync to be called
@@ -82,19 +75,10 @@ struct ForegroundPOSCatalogSyncDispatcherTests {
     func timerFires_whenNoDefaultStore_skipsSync() async throws {
         // Given
         let coordinator = MockPOSCatalogSyncCoordinator()
-        let noStoreSession = SessionManager.testingInstance
-        noStoreSession.setStoreId(nil)
-        let noStoreStores = MockStoresManager(sessionManager: noStoreSession)
-        noStoreStores.testPOSCatalogSyncCoordinator = coordinator
-        let dispatcher = ForegroundPOSCatalogSyncDispatcher(
-            notificationCenter: notificationCenter,
-            timerProvider: timerProvider,
-            featureFlagService: featureFlags,
-            stores: noStoreStores,
-            isAppActive: { true }
-        )
+        sessionManager.setStoreId(nil)
+        stores.testPOSCatalogSyncCoordinator = coordinator
 
-        dispatcher.start()
+        await sut.start()
         let timer = try #require(timerProvider.createdTimers.first)
 
         // When
@@ -111,7 +95,7 @@ struct ForegroundPOSCatalogSyncDispatcherTests {
         coordinator.performSmartSyncResult = .failure(POSCatalogSyncError.syncAlreadyInProgress(siteID: 123))
         stores.testPOSCatalogSyncCoordinator = coordinator
 
-        sut.start()
+        await sut.start()
         let timer = try #require(timerProvider.createdTimers.first)
 
         // When - fire timer and wait for sync to be called
@@ -129,13 +113,13 @@ struct ForegroundPOSCatalogSyncDispatcherTests {
     @Test
     func start_whenSiteChanges_resetsAndRestartsSync() async throws {
         // Given - start with site 123
-        sut.start()
+        await sut.start()
         let firstTimer = try #require(timerProvider.createdTimers.first)
         #expect(timerProvider.createdTimers.count == 1)
 
         // When - change site to 456 and start again
         sessionManager.setStoreId(456)
-        sut.start()
+        await sut.start()
 
         // Then - old timer cancelled, new timer created
         #expect(firstTimer.isCancelled == true)
@@ -160,18 +144,76 @@ struct ForegroundPOSCatalogSyncDispatcherTests {
     @Test
     func defaultSiteNotification_stopsDispatcher() async throws {
         // Given
-        sut.start()
+        await sut.start()
         let timer = try #require(timerProvider.createdTimers.first)
         #expect(timer.isResumed == true)
 
         // When - default site changes
-        notificationCenter.post(name: .StoresManagerDidUpdateDefaultSite, object: nil)
+        await withCheckedContinuation { continuation in
+            timer.onCancelled = { continuation.resume() }
 
-        // Then - dispatcher stops
-        #expect(timer.isCancelled == true)
+            notificationCenter.post(name: .StoresManagerDidUpdateDefaultSite, object: nil)
+        }
 
         // Verify dispatcher can restart after notification
-        sut.start()
+        await sut.start()
+        #expect(timerProvider.createdTimers.count == 2)
+        #expect(timerProvider.createdTimers.last?.isResumed == true)
+    }
+
+    @Test
+    func didBecomeActiveNotification_startsTimer() async throws {
+        // Given - dispatcher started but app not active initially
+        let inactiveDispatcher = ForegroundPOSCatalogSyncDispatcher(
+            notificationCenter: notificationCenter,
+            timerProvider: timerProvider,
+            featureFlagService: featureFlags,
+            stores: stores,
+            isAppActive: { false }
+        )
+        await inactiveDispatcher.start()
+
+        // Then - no timer created yet
+        #expect(timerProvider.createdTimers.isEmpty)
+
+        // When - app becomes active, wait for timer to be created
+        await withCheckedContinuation { continuation in
+            timerProvider.onTimerCreated = { continuation.resume() }
+
+            notificationCenter.post(name: UIApplication.didBecomeActiveNotification, object: nil)
+        }
+
+        // Then - timer should be created and started
+        #expect(timerProvider.createdTimers.count == 1)
+        #expect(timerProvider.createdTimers.first?.isResumed == true)
+    }
+
+    @Test
+    func didEnterBackgroundNotification_stopsTimer() async throws {
+        // Given
+        await sut.start()
+        let timer = try #require(timerProvider.createdTimers.first)
+        #expect(timer.isResumed == true)
+        #expect(timer.isCancelled == false)
+
+        // When - app enters background
+        await withCheckedContinuation { continuation in
+            timer.onCancelled = { continuation.resume() }
+
+            notificationCenter.post(name: UIApplication.didEnterBackgroundNotification, object: nil)
+        }
+
+        // Then - timer should be cancelled
+        #expect(timer.isCancelled == true)
+
+        // When - app becomes active again, wait for new timer
+        await withCheckedContinuation { continuation in
+            timerProvider.onTimerCreated = { continuation.resume() }
+
+            notificationCenter.post(name: UIApplication.didBecomeActiveNotification, object: nil)
+        }
+
+        // Then - new timer should be created
         #expect(timerProvider.createdTimers.count == 2)
         #expect(timerProvider.createdTimers.last?.isResumed == true)
     }
@@ -182,6 +224,7 @@ struct ForegroundPOSCatalogSyncDispatcherTests {
 private final class MockDispatchTimer: DispatchTimerProtocol {
     private(set) var isResumed = false
     private(set) var isCancelled = false
+    var onCancelled: () -> Void = { }
     private var eventHandler: (() -> Void)?
 
     func schedule(deadline: DispatchTime, repeating: Double, leeway: DispatchTimeInterval) {
@@ -198,6 +241,7 @@ private final class MockDispatchTimer: DispatchTimerProtocol {
 
     func cancel() {
         isCancelled = true
+        onCancelled()
     }
 
     /// Test helper to manually fire the timer
@@ -208,10 +252,12 @@ private final class MockDispatchTimer: DispatchTimerProtocol {
 
 private final class MockDispatchTimerProvider: DispatchTimerProviding {
     private(set) var createdTimers: [MockDispatchTimer] = []
+    var onTimerCreated: () -> Void = { }
 
     func makeTimer(queue: DispatchQueue) -> DispatchTimerProtocol {
         let timer = MockDispatchTimer()
         createdTimers.append(timer)
+        onTimerCreated()
         return timer
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -722,7 +722,7 @@ private extension MainTabBarController {`
`722`	`722`	`viewModel.loadHubMenuTabBadge()`
`723`	`723`
`724`	`724`	`// Begin foreground synchronization if POS tab becomes visible`
`725`		`- isPOSTabVisible ? posSyncDispatcher.start() : posSyncDispatcher.stop()`
	`725`	`+ await isPOSTabVisible ? posSyncDispatcher.start() : posSyncDispatcher.stop()`
`726`	`726`	`}`
`727`	`727`	`}`
`728`	`728`