Better concurrency support in iOS API Manager (#45)

* Add a unit test that fails during concurrent usage

* Ensure that the unsafe operations are executed concurrently, while also ensuring completions go back to the main thread

* Add tests to the test app to show the getFeatureFlags() working concurrently and reporting to the main thread

* Remove a probable memory leak in the API manager, which was keeping two strong references to each request / response

* Convert from NSMutableData to Data class internally

* A few small formatting and commenting fixes to finish the PR

* Test for API errors in our concurrent test

Example/FlagsmithClient/AppDelegate.swift

@@ -9,11 +9,15 @@
 import UIKit
 import FlagsmithClient
 
+func isSuccess<T,F>(_ result: Result<T,F>) -> Bool {
+    if case .success = result { return true } else { return false }
+}
+
 @UIApplicationMain
 class AppDelegate: UIResponder, UIApplicationDelegate {
 
   var window: UIWindow?
-
+  let concurrentQueue = DispatchQueue(label: "concurrentQueue", attributes: .concurrent)
 
   func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey: Any]?) -> Bool {
     // Override point for customization after application launch.
@@ -48,6 +52,16 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
     Flagsmith.shared.hasFeatureFlag(withID: "freeze_delinquent_accounts") { (result) in
       print(result)
     }
+
+    // Try getting the feature flags concurrently to ensure that this does not cause any issues
+    // This was originally highlighted in https://github.com/Flagsmith/flagsmith-ios-client/pull/40
+    for _ in 1...20 {
+      concurrentQueue.async {
+        Flagsmith.shared.getFeatureFlags() { (result) in
+        }
+      }
+    }
+
     //Flagsmith.shared.setTrait(Trait(key: "<my_key>", value: "<my_value>"), forIdentity: "<my_identity>") { (result) in print(result) }
     //Flagsmith.shared.getIdentity("<my_key>") { (result) in print(result) }
     return true

Example/Podfile.lock

@@ -1,5 +1,5 @@
 PODS:
-  - FlagsmithClient (1.1.2)
+  - FlagsmithClient (3.4.0)
 
 DEPENDENCIES:
   - FlagsmithClient (from `../`)
@@ -9,8 +9,8 @@ EXTERNAL SOURCES:
     :path: "../"
 
 SPEC CHECKSUMS:
-  FlagsmithClient: dd72c22b356fe008aaec4a7069e88579c3b7d979
+  FlagsmithClient: 0f8ed4a38dec385d73cc21a64b791b39bcc8c32b
 
 PODFILE CHECKSUM: 9fc876dee0cf031cae843156b0740a94b4994d8c
 
-COCOAPODS: 1.11.2
+COCOAPODS: 1.13.0

FlagsmithClient/Classes/Internal/APIManager.swift

@@ -21,8 +21,9 @@ class APIManager : NSObject, URLSessionDataDelegate {
   var apiKey: String?
 
   // store the completion handlers and accumulated data for each task
-  private var tasksToCompletionHandlers:[URLSessionDataTask:(Result<Data, Error>) -> Void] = [:]
-  private var tasksToData:[URLSessionDataTask:NSMutableData] = [:]
+  private var tasksToCompletionHandlers:[Int:(Result<Data, Error>) -> Void] = [:]
+  private var tasksToData:[Int:Data] = [:]
+  private let serialAccessQueue = DispatchQueue(label: "flagsmithSerialAccessQueue")
 
   override init() {
     super.init()
@@ -31,36 +32,42 @@ class APIManager : NSObject, URLSessionDataDelegate {
   }
 
   func urlSession(_ session: URLSession, task: URLSessionTask, didCompleteWithError error: Error?) {
-    if let dataTask = task as? URLSessionDataTask {
-      if let completion = tasksToCompletionHandlers[dataTask] {
-        if let error = error {
-          completion(.failure(FlagsmithError.unhandled(error)))
-        }
-        else {
-          let data = tasksToData[dataTask] ?? NSMutableData()
-          completion(.success(data as Data))
+    serialAccessQueue.sync {
+      if let dataTask = task as? URLSessionDataTask {
+        if let completion = tasksToCompletionHandlers[dataTask.taskIdentifier] {
+          if let error = error {
+            DispatchQueue.main.async { completion(.failure(FlagsmithError.unhandled(error))) }
+          }
+          else {
+            let data = tasksToData[dataTask.taskIdentifier] ?? Data()
+            DispatchQueue.main.async { completion(.success(data)) }
+          }
         }
+        tasksToCompletionHandlers[dataTask.taskIdentifier] = nil
+        tasksToData[dataTask.taskIdentifier] = nil
       }
-      tasksToCompletionHandlers[dataTask] = nil
-      tasksToData[dataTask] = nil
     }
   }
 
-  func urlSession(_ session: URLSession, dataTask: URLSessionDataTask, willCacheResponse proposedResponse: CachedURLResponse, completionHandler: @escaping (CachedURLResponse?) -> Void) {
-
-    // intercept and modify the cache settings for the response
-    if Flagsmith.shared.cacheConfig.useCache {
-      let newResponse = proposedResponse.response(withExpirationDuration: Int(Flagsmith.shared.cacheConfig.cacheTTL))
-      completionHandler(newResponse)
-    } else {
-      completionHandler(proposedResponse)
+  func urlSession(_ session: URLSession, dataTask: URLSessionDataTask, willCacheResponse proposedResponse: CachedURLResponse, 
+                  completionHandler: @escaping (CachedURLResponse?) -> Void) {
+    serialAccessQueue.sync {
+      // intercept and modify the cache settings for the response
+      if Flagsmith.shared.cacheConfig.useCache {
+        let newResponse = proposedResponse.response(withExpirationDuration: Int(Flagsmith.shared.cacheConfig.cacheTTL))
+        DispatchQueue.main.async { completionHandler(newResponse) }
+      } else {
+        DispatchQueue.main.async { completionHandler(proposedResponse) }
+      }
     }
   }
 
   func urlSession(_ session: URLSession, dataTask: URLSessionDataTask, didReceive data: Data) {
-    let existingData = tasksToData[dataTask] ?? NSMutableData()
-    existingData.append(data)
-    tasksToData[dataTask] = existingData
+    serialAccessQueue.sync {
+      var existingData = tasksToData[dataTask.taskIdentifier] ?? Data()
+      existingData.append(data)
+      tasksToData[dataTask.taskIdentifier] = existingData
+    }
   }
 
     func urlSession(_ session: URLSession, dataTask: URLSessionDataTask, didReceive response: URLResponse, completionHandler: @escaping (URLSession.ResponseDisposition) -> Void) {
@@ -98,9 +105,11 @@ class APIManager : NSObject, URLSessionDataDelegate {
     }
 
     // we must use the delegate form here, not the completion handler, to be able to modify the cache
-    let task = session.dataTask(with: request)
-    tasksToCompletionHandlers[task] = completion
-    task.resume()
+    serialAccessQueue.sync {
+      let task = session.dataTask(with: request)
+      tasksToCompletionHandlers[task.taskIdentifier] = completion
+      task.resume()
+    }
   }
 
   /// Requests a api route and only relays success or failure of the action.

FlagsmithClient/Classes/Internal/Router.swift

@@ -10,7 +10,7 @@ import Foundation
 import FoundationNetworking
 #endif
 
-enum Router {
+enum Router: Sendable {
   private enum HTTPMethod: String {
     case get = "GET"
     case post = "POST"

FlagsmithClient/Tests/APIManagerTests.swift

@@ -60,4 +60,32 @@ final class APIManagerTests: FlagsmithClientTestCase {
             return
         }
     }
+
+    func testConcurrentRequests() throws {
+        apiManager.apiKey = "8D5ABC87-6BBF-4AE7-BC05-4DC1AFE770DF"
+        let concurrentQueue = DispatchQueue(label: "concurrentQueue", attributes: .concurrent)
+
+        var expectations:[XCTestExpectation] = [];
+        let iterations = 1000
+        var error: FlagsmithError?
+
+        for concurrentIteration in 1...iterations {
+            let expectation = XCTestExpectation(description: "Multiple threads can access the APIManager \(concurrentIteration)")
+            expectations.append(expectation)
+            concurrentQueue.async {
+                self.apiManager.request(.getFlags) { (result: Result<Void, Error>) in
+                  if case let .failure(e) = result {
+                    error = e as? FlagsmithError
+                  }
+                    expectation.fulfill()
+                }
+            }
+        }
+
+        wait(for: expectations, timeout: 5)
+        // Ensure that we didn't have any errors during the process
+        XCTAssertTrue(error == nil)
+
+        print("Finished!")
+    }
 }