Merge pull request #1227 from apollographql/fix/multifetch

Fix for concurrent fetching bugs

Author: designatednerd

Apollo.xcodeproj/project.pbxproj

@@ -26,6 +26,7 @@
 		9B518C87235F819E004C426D /* CLIDownloader.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9B518C85235F8125004C426D /* CLIDownloader.swift */; };
 		9B518C8C235F8B5F004C426D /* ApolloFilePathHelper.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9B518C8A235F8B05004C426D /* ApolloFilePathHelper.swift */; };
 		9B518C8D235F8B9E004C426D /* CLIDownloaderTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9B518C88235F8AD4004C426D /* CLIDownloaderTests.swift */; };
+		9B554CC4247DC29A002F452A /* TaskData.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9B554CC3247DC29A002F452A /* TaskData.swift */; };
 		9B5A1EFC243528AA00F066BB /* InputObjectGenerationTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9B5A1EFB243528AA00F066BB /* InputObjectGenerationTests.swift */; };
 		9B5A1EFD24352AC100F066BB /* ExpectedReviewInput.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9B68F06C241C646700E97318 /* ExpectedReviewInput.swift */; };
 		9B5A1EFE24352AED00F066BB /* ExpectedColorInput.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9B68F06A241C643000E97318 /* ExpectedColorInput.swift */; };
@@ -399,6 +400,7 @@
 		9B518C85235F8125004C426D /* CLIDownloader.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CLIDownloader.swift; sourceTree = "<group>"; };
 		9B518C88235F8AD4004C426D /* CLIDownloaderTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CLIDownloaderTests.swift; sourceTree = "<group>"; };
 		9B518C8A235F8B05004C426D /* ApolloFilePathHelper.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ApolloFilePathHelper.swift; sourceTree = "<group>"; };
+		9B554CC3247DC29A002F452A /* TaskData.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TaskData.swift; sourceTree = "<group>"; };
 		9B5A1EE3243284F300F066BB /* Package.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Package.swift; sourceTree = "<group>"; };
 		9B5A1EFB243528AA00F066BB /* InputObjectGenerationTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = InputObjectGenerationTests.swift; sourceTree = "<group>"; };
 		9B5A1EFF2435356400F066BB /* ExpectedColorInputNoModifier.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ExpectedColorInputNoModifier.swift; sourceTree = "<group>"; };
@@ -1287,6 +1289,7 @@
 				9F69FFA81D42855900E000B1 /* NetworkTransport.swift */,
 				9BEDC79D22E5D2CF00549BF6 /* RequestCreator.swift */,
 				9B4F453E244A27B900C2CF7D /* URLSessionClient.swift */,
+				9B554CC3247DC29A002F452A /* TaskData.swift */,
 			);
 			name = Network;
 			sourceTree = "<group>";
@@ -2112,6 +2115,7 @@
 				54DDB0921EA045870009DD99 /* InMemoryNormalizedCache.swift in Sources */,
 				9FC9A9C51E2D6CE70023C4D5 /* GraphQLSelectionSet.swift in Sources */,
 				9BDE43DD22C6705300FD7C7F /* GraphQLHTTPResponseError.swift in Sources */,
+				9B554CC4247DC29A002F452A /* TaskData.swift in Sources */,
 				9FCDFD231E33A0D8007519DC /* AsynchronousOperation.swift in Sources */,
 				9BA1244A22D8A8EA00BF1D24 /* JSONSerialization+Sorting.swift in Sources */,
 				9B708AAD2305884500604A11 /* ApolloClientProtocol.swift in Sources */,

Sources/Apollo/Atomic.swift

@@ -27,6 +27,12 @@ public class Atomic<T> {
       _value = newValue
     }
   }
+  
+  public func mutate(block: (inout T) -> Void) {
+    lock.lock()
+    block(&_value)
+    lock.unlock()
+  }
 }
 
 public extension Atomic where T == Int {

Sources/Apollo/TaskData.swift

@@ -0,0 +1,25 @@
+import Foundation
+
+public class TaskData {
+  
+  public let rawCompletion: URLSessionClient.RawCompletion?
+  public let completionBlock: URLSessionClient.Completion
+  private(set) var data: Data = Data()
+  private(set) var response: HTTPURLResponse? = nil
+  
+  init(rawCompletion: URLSessionClient.RawCompletion?,
+       completionBlock: @escaping URLSessionClient.Completion) {
+    self.rawCompletion = rawCompletion
+    self.completionBlock = completionBlock
+  }
+  
+  func append(additionalData: Data) {
+    self.data.append(additionalData)
+  }
+  
+  func responseReceived(response: URLResponse) {
+    if let httpResponse = response as? HTTPURLResponse {
+      self.response = httpResponse
+    }
+  }
+}

Sources/Apollo/URLSessionClient.swift

@@ -23,10 +23,7 @@ open class URLSessionClient: NSObject, URLSessionDelegate, URLSessionTaskDelegat
   /// A completion block returning a result. On `.success` it will contain a tuple with non-nil `Data` and its corresponding `HTTPURLResponse`. On `.failure` it will contain an error.
   public typealias Completion = (Result<(Data, HTTPURLResponse), Error>) -> Void
 
-  private var completionBlocks = Atomic<[Int: Completion]>([:])
-  private var rawCompletions = Atomic<[Int: RawCompletion]>([:])
-  private var datas = Atomic<[Int: Data]>([:])
-  private var responses = Atomic<[Int: HTTPURLResponse]>([:])
+  private var tasks = Atomic<[Int: TaskData]>([:])
 
   /// The raw URLSession being used for this client
   open private(set) var session: URLSession!
@@ -44,24 +41,22 @@ open class URLSessionClient: NSObject, URLSessionDelegate, URLSessionTaskDelegat
                               delegateQueue: callbackQueue)
   }
 
+  deinit {
+    self.clearAllTasks()
+  }
+  
   /// Clears underlying dictionaries of any data related to a particular task identifier.
   ///
   /// - Parameter identifier: The identifier of the task to clear.
-  open func clearTask(with identifier: Int) {
-    self.rawCompletions.value.removeValue(forKey: identifier)
-    self.completionBlocks.value.removeValue(forKey: identifier)
-    self.datas.value.removeValue(forKey: identifier)
-    self.responses.value.removeValue(forKey: identifier)
+  open func clear(task identifier: Int) {
+    self.tasks.mutate { $0.removeValue(forKey: identifier) }
   }
 
   /// Clears underlying dictionaries of any data related to all tasks.
   ///
   /// Mostly useful for cleanup and/or after invalidation of the `URLSession`.
   open func clearAllTasks() {
-    self.rawCompletions.value.removeAll()
-    self.completionBlocks.value.removeAll()
-    self.datas.value.removeAll()
-    self.responses.value.removeAll()
+    self.tasks.mutate { $0.removeAll() }
   }
 
   /// The main method to perform a request.
@@ -76,16 +71,15 @@ open class URLSessionClient: NSObject, URLSessionDelegate, URLSessionTaskDelegat
   open func sendRequest(_ request: URLRequest,
                         rawTaskCompletionHandler: RawCompletion? = nil,
                         completion: @escaping Completion) -> URLSessionTask {
-    let dataTask = self.session.dataTask(with: request)
-    if let rawCompletion = rawTaskCompletionHandler {
-      self.rawCompletions.value[dataTask.taskIdentifier] = rawCompletion
-    }
+    let task = self.session.dataTask(with: request)
+    let taskData = TaskData(rawCompletion: rawTaskCompletionHandler,
+                            completionBlock: completion)
 
-    self.completionBlocks.value[dataTask.taskIdentifier] = completion
-    self.datas.value[dataTask.taskIdentifier] = Data()
-    dataTask.resume()
+    self.tasks.mutate { $0[task.taskIdentifier] = taskData }
 
-    return dataTask
+    task.resume()
+    
+    return task
   }
 
   /// Cancels a given task and clears out its underlying data.
@@ -94,16 +88,16 @@ open class URLSessionClient: NSObject, URLSessionDelegate, URLSessionTaskDelegat
   ///
   /// - Parameter task: The task you wish to cancel.
   open func cancel(task: URLSessionTask) {
-    self.clearTask(with: task.taskIdentifier)
+    self.clear(task: task.taskIdentifier)
     task.cancel()
   }
 
   // MARK: - URLSessionDelegate
 
   open func urlSession(_ session: URLSession, didBecomeInvalidWithError error: Error?) {
     let finalError = error ?? URLSessionClientError.sessionBecameInvalidWithoutUnderlyingError
-    for block in self.completionBlocks.value.values {
-      block(.failure(finalError))
+    for task in self.tasks.value.values {
+      task.completionBlock(.failure(finalError))
     }
 
     self.clearAllTasks()
@@ -145,38 +139,33 @@ open class URLSessionClient: NSObject, URLSessionDelegate, URLSessionTaskDelegat
   open func urlSession(_ session: URLSession,
                        task: URLSessionTask,
                        didCompleteWithError error: Error?) {
-    let taskIdentifier = task.taskIdentifier
     defer {
-      self.clearTask(with: taskIdentifier)
+      self.clear(task: task.taskIdentifier)
     }
 
-    guard let completion = self.completionBlocks.value.removeValue(forKey: taskIdentifier) else {
+    guard let taskData = self.tasks.value[task.taskIdentifier] else {
       // No completion blocks, the task has likely been cancelled. Bail out.
       return
     }
 
-    let data = self.datas.value[taskIdentifier]
-    let response = self.responses.value[taskIdentifier]
+    let data = taskData.data
+    let response = taskData.response
 
-    if let rawCompletion = self.rawCompletions.value.removeValue(forKey: taskIdentifier) {
+    if let rawCompletion = taskData.rawCompletion {
       rawCompletion(data, response, error)
     }
 
-    guard let finalData = data else {
-      // Data is immediately created for a task on creation, so if it's not there, something's gone wrong.
-      completion(.failure(URLSessionClientError.dataForRequestNotFound(request: task.originalRequest)))
-      return
-    }
+    let completion = taskData.completionBlock
 
     if let finalError = error {
-      completion(.failure(URLSessionClientError.networkError(data: finalData, response: response, underlying: finalError)))
+      completion(.failure(URLSessionClientError.networkError(data: data, response: response, underlying: finalError)))
     } else {
       guard let finalResponse = response else {
         completion(.failure(URLSessionClientError.noHTTPResponse(request: task.originalRequest)))
         return
       }
 
-      completion(.success((finalData, finalResponse)))
+      completion(.success((data, finalResponse)))
     }
   }
 
@@ -215,7 +204,14 @@ open class URLSessionClient: NSObject, URLSessionDelegate, URLSessionTaskDelegat
   open func urlSession(_ session: URLSession,
                        dataTask: URLSessionDataTask,
                        didReceive data: Data) {
-    self.datas.value[dataTask.taskIdentifier]?.append(data)
+    self.tasks.mutate {
+      guard let taskData = $0[dataTask.taskIdentifier] else {
+        assertionFailure("No data found for task \(dataTask.taskIdentifier), cannot append received data")
+        return
+      }
+      
+      taskData.append(additionalData: data)
+    }
   }
 
   @available(iOS 9.0, OSXApplicationExtension 10.11, OSX 10.11, *)
@@ -246,8 +242,12 @@ open class URLSessionClient: NSObject, URLSessionDelegate, URLSessionTaskDelegat
       completionHandler(.allow)
     }
 
-    if let httpResponse = response as? HTTPURLResponse {
-      self.responses.value[dataTask.taskIdentifier] = httpResponse
+    self.tasks.mutate {
+      guard let taskData = $0[dataTask.taskIdentifier] else {
+        return
+      }
+      
+      taskData.responseReceived(response: response)
     }
   }
 }

Tests/ApolloCacheDependentTests/StarWarsServerTests.swift

@@ -9,15 +9,17 @@ protocol TestConfig {
 }
 
 class DefaultConfig: TestConfig {
+  let transport =  HTTPNetworkTransport(url: URL(string: "http://localhost:8080/graphql")!)
   func network() -> HTTPNetworkTransport {
-    return HTTPNetworkTransport(url: URL(string: "http://localhost:8080/graphql")!)
+    return transport
   }
 }
 
 class APQsConfig: TestConfig {
+  let transport = HTTPNetworkTransport(url: URL(string: "http://localhost:8080/graphql")!,
+                                       enableAutoPersistedQueries: true)
   func network() -> HTTPNetworkTransport {
-    return HTTPNetworkTransport(url: URL(string: "http://localhost:8080/graphql")!,
-                                enableAutoPersistedQueries: true)
+    return transport
   }
 }
 

Tests/ApolloTests/HTTPBinAPI.swift

@@ -1,43 +1,59 @@
 import Foundation
 
 enum HTTPBinAPI {
-    static let baseURL = URL(string: "https://httpbin.org/")!
-    enum Endpoint {
-        case bytes(count: Int)
-        case get
-        case headers
-        case image
-        case post
-        
-        var toString: String {
-            
-            switch self {
-            case .bytes(let count):
-                return "bytes/\(count)"
-            case .get:
-                return "get"
-            case .headers:
-                return "headers"
-            case .image:
-                return "image/jpeg"
-            case .post:
-                return "post"
-            }
-        }
-        
-        var toURL: URL {
-            HTTPBinAPI.baseURL.appendingPathComponent(self.toString)
-        }
+  static let baseURL = URL(string: "https://httpbin.org")!
+  enum Endpoint {
+    case bytes(count: Int)
+    case get
+    case getWithIndex(index: Int)
+    case headers
+    case image
+    case post
+    
+    var toString: String {
+      
+      switch self {
+      case .bytes(let count):
+        return "bytes/\(count)"
+      case .get,
+           .getWithIndex:
+        return "get"
+      case .headers:
+        return "headers"
+      case .image:
+        return "image/jpeg"
+      case .post:
+        return "post"
+      }
     }
-}
-
-struct HTTPBinResponse: Codable {
 
-    let headers: [String: String]
-    let url: String
-    let json: [String: String]?
+    var queryParams: [URLQueryItem]? {
+      switch self {
+      case .getWithIndex(let index):
+        return [URLQueryItem(name: "index", value: "\(index)")]
+      default:
+        return nil
+      }
+    }
 
-    init(data: Data) throws {
-        self = try JSONDecoder().decode(Self.self, from: data)
+    var toURL: URL {
+      var components = URLComponents(url: HTTPBinAPI.baseURL, resolvingAgainstBaseURL: false)!
+      components.path = "/\(self.toString)"
+      components.queryItems = self.queryParams
+      
+      return components.url!
     }
+  }
+}
+
+struct HTTPBinResponse: Codable {
+  
+  let headers: [String: String]
+  let url: String
+  let json: [String: String]?
+  let args: [String: String]?
+  
+  init(data: Data) throws {
+    self = try JSONDecoder().decode(Self.self, from: data)
+  }
 }