Overhaul Refresh Token Retry Mechanism (#4)

erenkabakci · Apr 28, 2020 · 8512cd2 · 8512cd2
1 parent 4fead08
commit 8512cd2
Show file tree

Hide file tree

Showing 11 changed files with 326 additions and 331 deletions.
diff --git a/.github/workflows/swift.yml b/.github/workflows/swift.yml
@@ -15,4 +15,4 @@ jobs:
     - name: Build
       run: swift build -v
     - name: Run tests
-      run: swift test -v
+      run: xcodebuild test -project fusion.xcodeproj -scheme fusion -destination 'platform=iOS Simulator,name=iPhone 11'
diff --git a/Sources/NetworkError.swift b/Sources/NetworkError.swift
@@ -30,6 +30,7 @@ public enum NetworkError: Error, Equatable {
   case urlError(URLError?)
   case parsingFailure
   case corruptUrl
+  case timeout
   case unauthorized
   case forbidden
   case generic(HttpStatusCode)

diff --git a/Sources/SessionPublisherProtocol.swift b/Sources/SessionPublisherProtocol.swift
@@ -32,7 +32,7 @@ public protocol SessionPublisherProtocol: AnyObject {
 extension URLSession: SessionPublisherProtocol {
   public func dataTaskPublisher(for request: URLRequest) -> AnyPublisher<(data: Data, response: URLResponse), Error> {
     self.dataTaskPublisher(for: request)
-    .receive(on: DispatchQueue.main)
+    .subscribe(on: DispatchQueue.global())
     .mapError { NetworkError.urlError($0) }
     .eraseToAnyPublisher()
   }

diff --git a/Sources/WebService/AuthenticatedWebService/AuthenticatedWebService.swift b/Sources/WebService/AuthenticatedWebService/AuthenticatedWebService.swift
@@ -33,10 +33,10 @@ public enum AuthorizationHeaderScheme: String {
 
 public struct AuthenticatedWebServiceConfiguration {
   let authorizationHeaderScheme: AuthorizationHeaderScheme
-  let refreshTriggerErrors: [Error]
+  let refreshTriggerErrors: [NetworkError]
 
   public init(authorizationHeaderScheme: AuthorizationHeaderScheme = .none,
-              refreshTriggerErrors: [Error] = [NetworkError.unauthorized]) {
+              refreshTriggerErrors: [NetworkError] = [NetworkError.unauthorized]) {
     self.authorizationHeaderScheme = authorizationHeaderScheme
     self.refreshTriggerErrors = refreshTriggerErrors
   }
@@ -56,84 +56,83 @@ open class AuthenticatedWebService: WebService {
     self.configuration = configuration
     super.init(urlSession: urlSession)
   }
-  
+
   override public func execute<T>(urlRequest: URLRequest) -> AnyPublisher<T, Error> where T : Decodable {
     var urlRequest = urlRequest
-    var currentAccessToken: String?
 
-    authenticationQueue.sync {
-      currentAccessToken = self.tokenProvider.accessToken.value
+    func appendTokenAndExecute(accessToken: AccessToken) -> AnyPublisher<T, Error> {
+      urlRequest.setValue(self.configuration.authorizationHeaderScheme.rawValue + accessToken, forHTTPHeaderField: "Authorization")
+      return super.execute(urlRequest: urlRequest)
+        .subscribe(on: DispatchQueue.global())
+        .eraseToAnyPublisher()
+    }
+
+    return Deferred {
+      self.tokenProvider.accessToken
+        .compactMap { $0 }
+        .setFailureType(to: Error.self)
+        .flatMap { accessToken -> AnyPublisher<T, Error> in
+          return appendTokenAndExecute(accessToken: accessToken)
+      }
     }
-
-    guard let accessToken = currentAccessToken else {
-      return Fail<T, Error>(error: NetworkError.unauthorized).eraseToAnyPublisher()
+    .timeout(10, scheduler: DispatchQueue.main, customError: { NetworkError.timeout })
+    .catch { [weak self] error -> AnyPublisher<T, Error> in
+      guard let self = self else {
+        return Fail<T, Error>(error: NetworkError.unknown).eraseToAnyPublisher()
+      }
+
+      if self.configuration.refreshTriggerErrors.contains(where: { return $0.reflectedString == error.reflectedString }){
+        return self.retrySynchronizedTokenRefresh()
+          .flatMap {
+            appendTokenAndExecute(accessToken: $0)
+        }.eraseToAnyPublisher()
+      }
+      return Fail<T, Error>(error: error).eraseToAnyPublisher()
     }
-
-    urlRequest.setValue(self.configuration.authorizationHeaderScheme.rawValue + accessToken, forHTTPHeaderField: "Authorization")
-
-    return super.execute(urlRequest: urlRequest)
-      .catch { [weak self] error -> AnyPublisher<T, Error> in
-        guard let self = self else {
-          return Fail<T, Error>(error: NetworkError.unknown).eraseToAnyPublisher()
-        }
-
-        if self.configuration.refreshTriggerErrors.contains(where: { return $0.reflectedString == error.reflectedString }){
-          self.retrySynchronizedTokenRefresh()
-
-          return self.execute(urlRequest: urlRequest)
-            .delay(for: 0.2, scheduler: self.authenticationQueue)
-            .eraseToAnyPublisher()
-        }
-        return Fail<T, Error>(error: error).eraseToAnyPublisher()
-      }.eraseToAnyPublisher()
+    .receive(on: DispatchQueue.main)
+    .eraseToAnyPublisher()
   }
-
-
+
   override public func execute(urlRequest: URLRequest) -> AnyPublisher<Void, Error> {
     var urlRequest = urlRequest
-    var currentAccessToken: String?
 
-    authenticationQueue.sync {
-      currentAccessToken = self.tokenProvider.accessToken.value
+    func appendTokenAndExecute(accessToken: AccessToken) -> AnyPublisher<Void, Error> {
+      urlRequest.setValue(self.configuration.authorizationHeaderScheme.rawValue + accessToken, forHTTPHeaderField: "Authorization")
+      return super.execute(urlRequest: urlRequest)
+        .subscribe(on: DispatchQueue.global())
+        .eraseToAnyPublisher()
     }
 
-    guard let accessToken = currentAccessToken else {
-      return Fail<Void, Error>(error: NetworkError.unauthorized).eraseToAnyPublisher()
+    return Deferred {
+      self.tokenProvider.accessToken
+        .compactMap { $0 }
+        .setFailureType(to: Error.self)
+        .flatMap { accessToken -> AnyPublisher<Void, Error> in
+          return appendTokenAndExecute(accessToken: accessToken)
+        }.eraseToAnyPublisher()
+    }
+    .timeout(10, scheduler: DispatchQueue.main, customError: { NetworkError.timeout })
+    .catch { [weak self] error -> AnyPublisher<Void, Error> in
+      guard let self = self else {
+        return Fail<Void, Error>(error: NetworkError.unknown).eraseToAnyPublisher()
+      }
+
+      if self.configuration.refreshTriggerErrors.contains(where: { return $0.reflectedString == error.reflectedString }){
+        return self.retrySynchronizedTokenRefresh()
+          .flatMap {
+            appendTokenAndExecute(accessToken: $0)
+        }.eraseToAnyPublisher()
+      }
+      return Fail<Void, Error>(error: error).eraseToAnyPublisher()
     }
-
-    urlRequest.setValue(self.configuration.authorizationHeaderScheme.rawValue + accessToken, forHTTPHeaderField: "Authorization")
-
-    return super.execute(urlRequest: urlRequest)
-      .catch { [weak self] error -> AnyPublisher<Void, Error> in
-        guard let self = self else {
-          return Fail<Void, Error>(error: NetworkError.unknown).eraseToAnyPublisher()
-        }
-
-        if self.configuration.refreshTriggerErrors.contains(where: { return $0.reflectedString == error.reflectedString }){
-          self.retrySynchronizedTokenRefresh()
-
-          return self.execute(urlRequest: urlRequest)
-            .delay(for: 0.2, scheduler: self.authenticationQueue)
-            .eraseToAnyPublisher()
-        }
-        return Fail<Void, Error>(error: error).eraseToAnyPublisher()
-    }.eraseToAnyPublisher()
+    .receive(on: DispatchQueue.main)
+    .eraseToAnyPublisher()
   }
 
-  private func retrySynchronizedTokenRefresh() {
-    let dispatchGroup = DispatchGroup()
-    dispatchGroup.enter()
-
-    authenticationQueue.sync(flags: .barrier) {
-      self.tokenProvider.invalidateAccessToken()
-      self.tokenProvider.reissueAccessToken()
-        .sink(receiveCompletion: { _ in
-          dispatchGroup.leave()
-        },
-              receiveValue: { _ in })
-        .store(in: &self.subscriptions)
-      dispatchGroup.wait()
-    }
+  private func retrySynchronizedTokenRefresh() -> AnyPublisher<AccessToken, Error> {
+    tokenProvider.invalidateAccessToken()
+    return tokenProvider.reissueAccessToken()
+      .eraseToAnyPublisher()
   }
 }
 

diff --git a/Sources/WebService/AuthenticatedWebService/AuthenticationTokenProvidable.swift b/Sources/WebService/AuthenticatedWebService/AuthenticationTokenProvidable.swift
@@ -25,10 +25,13 @@
 import Combine
 import Foundation
 
+public typealias AccessToken = String
+public typealias RefreshToken = String
+
 public protocol AuthenticationTokenProvidable: AnyObject {
-  var accessToken: CurrentValueSubject<String?, Never> { get }
-  var refreshToken: CurrentValueSubject<String?, Never> { get }
-  func reissueAccessToken() -> AnyPublisher<Never, Error>
+  var accessToken: CurrentValueSubject<AccessToken?, Never> { get }
+  var refreshToken: CurrentValueSubject<RefreshToken?, Never> { get }
+  func reissueAccessToken() -> AnyPublisher<AccessToken, Error>
   func invalidateAccessToken()
   func invalidateRefreshToken()
 }
diff --git a/Sources/WebService/WebService.swift b/Sources/WebService/WebService.swift
@@ -13,7 +13,7 @@
 //
 //  The above copyright notice and this permission notice shall be included in all
 //  copies or substantial portions of the Software.
-//  
+//
 //  THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
 //  IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
 //  FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
@@ -35,7 +35,7 @@ CustomDecodable{
   public let jsonDecoder: JSONDecoder = JSONDecoder()
   private let session: SessionPublisherProtocol
   @ThreadSafe open var subscriptions = Set<AnyCancellable>()
-  
+
   public init(urlSession: SessionPublisherProtocol = URLSession(configuration: URLSessionConfiguration.ephemeral,
                                                                 delegate: nil,
                                                                 delegateQueue: nil)) {
@@ -49,9 +49,11 @@ CustomDecodable{
           return Fail(error: NetworkError.unknown).eraseToAnyPublisher()
         }
         return CurrentValueSubject((output.data, httpResponse)).eraseToAnyPublisher()
-    }.eraseToAnyPublisher()
+    }
+    .receive(on: DispatchQueue.main)
+    .eraseToAnyPublisher()
   }
-  
+
   public func execute<T>(urlRequest: URLRequest) -> AnyPublisher<T, Error> where T : Decodable {
     Deferred {
       Future { [weak self] promise in
@@ -62,8 +64,9 @@ CustomDecodable{
 
         var urlRequest = urlRequest
         urlRequest.appendAdditionalHeaders(headers: self.defaultHttpHeaders)
-        
+
         self.rawResponse(urlRequest: urlRequest)
+        .subscribe(on: DispatchQueue.global())
           .tryMap {
             try self.mapHttpResponseCodes(output: $0)
 
@@ -80,21 +83,24 @@ CustomDecodable{
               receiveValue: { promise(.success($0)) })
           .store(in: &self.subscriptions)
       }
-    }.eraseToAnyPublisher()
+    }
+    .receive(on: DispatchQueue.main)
+    .eraseToAnyPublisher()
   }
-  
+
   public func execute(urlRequest: URLRequest) -> AnyPublisher<Void, Error> {
     Deferred {
       Future { [weak self] promise in
         guard let self = self else {
           promise(.failure(NetworkError.unknown))
           return
         }
-        
+
         var urlRequest = urlRequest
         urlRequest.appendAdditionalHeaders(headers: self.defaultHttpHeaders)
-        
+
         self.rawResponse(urlRequest: urlRequest)
+            .subscribe(on: DispatchQueue.global())
           .tryMap {
             try self.mapHttpResponseCodes(output: $0)
             return
@@ -107,7 +113,9 @@ CustomDecodable{
               receiveValue: { promise(.success($0)) })
           .store(in: &self.subscriptions)
       }
-    }.eraseToAnyPublisher()
+    }
+    .receive(on: DispatchQueue.main)
+    .eraseToAnyPublisher()
   }
 
   open func mapHttpResponseCodes(output: (data:Data, response: HTTPURLResponse)) throws {

diff --git a/Tests/MockSession.swift b/Tests/MockSession.swift
@@ -35,16 +35,18 @@ open class MockSession: SessionPublisherProtocol {
   public func dataTaskPublisher(for urlRequest: URLRequest) -> AnyPublisher<(data: Data, response: URLResponse), Error> {
     methodCallStack.append(#function)
     finalUrlRequest = urlRequest
-    return Future<(data: Data, response: URLResponse), Error> { promise in
-      usleep(20)
-      if let successResponse = self.result?.0 {
-        promise(.success((successResponse.0,
-                          HTTPURLResponse(url: URL(string: "foo.com")!,
-                                          statusCode: successResponse.1,
-                                          httpVersion: nil,
-                                          headerFields: nil)!)))
-      } else if let errorResponse = self.result?.1 {
-        promise(.failure(NetworkError.urlError(errorResponse)))
+    return Deferred {
+      Future<(data: Data, response: URLResponse), Error> { promise in
+        usleep(20)
+        if let successResponse = self.result?.0 {
+          promise(.success((successResponse.0,
+                            HTTPURLResponse(url: URL(string: "foo.com")!,
+                                            statusCode: successResponse.1,
+                                            httpVersion: nil,
+                                            headerFields: nil)!)))
+        } else if let errorResponse = self.result?.1 {
+          promise(.failure(NetworkError.urlError(errorResponse)))
+        }
       }
     }.eraseToAnyPublisher()
   }