Merge pull request #13 from bookingcom/gtarasov/fix_async_experiment

Avoid deallocations of the view controllers on background threads

Author: pilot34

PerformanceSuite/Sources/ViewControllerObserver.swift

@@ -33,6 +33,16 @@ final class ViewControllerObserverFactory<T: ViewControllerObserver, S: ScreenMe
     private let metricsReceiver: S
     private let observerMaker: (S.ScreenIdentifier) -> T
 
+    private func ensureDeallocationOnTheMainThread(viewController: UIViewController) {
+        DispatchQueue.main.async {
+            // Make sure viewController is deallocated on the main thread, because
+            // if the last access is on the background thread, it will be deallocated
+            // in background, and it can cause data races in UIKit.
+            // Calling `hash` to make sure this call is not removed by the compilation optimizer
+            _ = viewController.hash
+        }
+    }
+
     private func observer(for viewController: UIViewController) -> T? {
         if PerformanceMonitoring.experiments.observersOnBackgroundQueue {
             dispatchPrecondition(condition: .onQueue(PerformanceMonitoring.queue))
@@ -66,6 +76,7 @@ final class ViewControllerObserverFactory<T: ViewControllerObserver, S: ScreenMe
         if PerformanceMonitoring.experiments.observersOnBackgroundQueue {
             PerformanceMonitoring.queue.async {
                 self.observer(for: viewController)?.beforeInit(viewController: viewController)
+                self.ensureDeallocationOnTheMainThread(viewController: viewController)
             }
         } else {
             observer(for: viewController)?.beforeInit(viewController: viewController)
@@ -76,6 +87,7 @@ final class ViewControllerObserverFactory<T: ViewControllerObserver, S: ScreenMe
         if PerformanceMonitoring.experiments.observersOnBackgroundQueue {
             PerformanceMonitoring.queue.async {
                 self.observer(for: viewController)?.beforeViewDidLoad(viewController: viewController)
+                self.ensureDeallocationOnTheMainThread(viewController: viewController)
             }
         } else {
             observer(for: viewController)?.beforeViewDidLoad(viewController: viewController)
@@ -86,6 +98,7 @@ final class ViewControllerObserverFactory<T: ViewControllerObserver, S: ScreenMe
         if PerformanceMonitoring.experiments.observersOnBackgroundQueue {
             PerformanceMonitoring.queue.async {
                 self.observer(for: viewController)?.afterViewDidAppear(viewController: viewController)
+                self.ensureDeallocationOnTheMainThread(viewController: viewController)
             }
         } else {
             observer(for: viewController)?.afterViewDidAppear(viewController: viewController)
@@ -96,6 +109,7 @@ final class ViewControllerObserverFactory<T: ViewControllerObserver, S: ScreenMe
         if PerformanceMonitoring.experiments.observersOnBackgroundQueue {
             PerformanceMonitoring.queue.async {
                 self.observer(for: viewController)?.beforeViewWillDisappear(viewController: viewController)
+                self.ensureDeallocationOnTheMainThread(viewController: viewController)
             }
         } else {
             observer(for: viewController)?.beforeViewWillDisappear(viewController: viewController)
@@ -106,6 +120,7 @@ final class ViewControllerObserverFactory<T: ViewControllerObserver, S: ScreenMe
         if PerformanceMonitoring.experiments.observersOnBackgroundQueue {
             PerformanceMonitoring.queue.async {
                 self.observer(for: viewController)?.afterViewWillAppear(viewController: viewController)
+                self.ensureDeallocationOnTheMainThread(viewController: viewController)
             }
         } else {
             observer(for: viewController)?.afterViewWillAppear(viewController: viewController)
@@ -116,6 +131,7 @@ final class ViewControllerObserverFactory<T: ViewControllerObserver, S: ScreenMe
         if PerformanceMonitoring.experiments.observersOnBackgroundQueue {
             PerformanceMonitoring.queue.async {
                 self.observer(for: viewController)?.beforeViewDidDisappear(viewController: viewController)
+                self.ensureDeallocationOnTheMainThread(viewController: viewController)
             }
         } else {
             observer(for: viewController)?.beforeViewDidDisappear(viewController: viewController)

PerformanceSuite/Tests/TTIObserverExtensionTests.swift

@@ -19,6 +19,7 @@ class TTIObserverExtensionTests: XCTestCase {
 
     override func setUpWithError() throws {
         try super.setUpWithError()
+        PerformanceMonitoring.experiments = Experiments(observersOnBackgroundQueue: true)
         defaultTimeProvider = timeProvider
         metricsReceiver = TTIMetricsReceiverStub()
         try PerformanceMonitoring.enable(config: [.screenLevelTTI(metricsReceiver)], experiments: Experiments(observersOnBackgroundQueue: true))
@@ -244,7 +245,8 @@ class TTIObserverExtensionTests: XCTestCase {
         window.rootViewController = tabbar
         window.makeKeyAndVisible()
 
-        waitForExpectations(timeout: 3, handler: nil)
+        wait(for: [exp1], timeout: 3)
+        PerformanceMonitoring.queue.sync {}
 
         // for the first controller we should calculate TTI between `init` and `screenIsReady` -> 1 second
         vc1.screenIsReady()
@@ -334,7 +336,8 @@ class TTIObserverExtensionTests: XCTestCase {
         window.rootViewController = vc1
         window.makeKeyAndVisible()
 
-        waitForExpectations(timeout: 3, handler: nil)
+        wait(for: [exp1], timeout: 3)
+        PerformanceMonitoring.queue.sync {}
 
         // we call screenIsReady for the child, but it should work for the parent
         vc2.screenIsReady()
@@ -403,6 +406,10 @@ private class MyViewController: UIViewController {
         super.viewDidLayoutSubviews()
         output += "viewDidLayoutSubviews\n"
     }
+
+    deinit {
+        XCTAssertTrue(Thread.isMainThread)
+    }
 }
 
 private struct MyView: View {

PerformanceSuite/Tests/ViewControllerObserverTests.swift

@@ -12,6 +12,18 @@ import XCTest
 
 class ViewControllerObserverTests: XCTestCase {
 
+    override func setUp() {
+        super.setUp()
+        PerformanceMonitoring.experiments = Experiments(observersOnBackgroundQueue: true)
+    }
+
+    override func tearDown() {
+        super.tearDown()
+        PerformanceMonitoring.consumerQueue.sync { }
+        PerformanceMonitoring.queue.sync { }
+        PerformanceMonitoring.experiments = Experiments()
+    }
+
     func testObserversCollection() {
         let o1 = Observer()
         let o2 = Observer()
@@ -159,4 +171,8 @@ private class MetricsConsumerForSwiftUITest: ScreenMetricsReceiver {
     func appRenderingMetricsReceived(metrics: RenderingMetrics) {}
 }
 
-private class MyViewController: UIViewController { }
+private class MyViewController: UIViewController {
+    deinit {
+        XCTAssertTrue(Thread.isMainThread)
+    }
+}