Overhaul Refresh Token Retry Mechanism #4
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,10 +33,10 @@ public enum AuthorizationHeaderScheme: String { | |
|
|
||
| public struct AuthenticatedWebServiceConfiguration { | ||
| let authorizationHeaderScheme: AuthorizationHeaderScheme | ||
| let refreshTriggerErrors: [NetworkError] | ||
|
|
||
| public init(authorizationHeaderScheme: AuthorizationHeaderScheme = .none, | ||
| 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 | ||
|
|
||
| func appendTokenAndExecute(accessToken: AccessToken) -> AnyPublisher<T, Error> { | ||
| urlRequest.setValue(self.configuration.authorizationHeaderScheme.rawValue + accessToken, forHTTPHeaderField: "Authorization") | ||
|
There was a problem hiding this comment. again, the consumer of this is deferred already. There is no direct access to this publisher without any defer mechanism. |
||
| return super.execute(urlRequest: urlRequest) | ||
| .subscribe(on: DispatchQueue.global()) | ||
|
There was a problem hiding this comment. same here, don't think it's responsibility of the framework itself There was a problem hiding this comment. It's a design decision that I took while creating the framework. For the consumer it doesn't change anything actually but I wanted fusion to provide a hassle-free development for the consumer. Therefore all the executions happen on the background thread and the deliveries happen on the main queue. If at any point a consumer wants to use a different thread, they are free to use |
||
| .eraseToAnyPublisher() | ||
| } | ||
|
|
||
| return Deferred { | ||
|
There was a problem hiding this comment. this deferred can be removed There was a problem hiding this comment. This, I am not sure since this was sort of the fixing point for the racing refrehTokens. I will let @emanuelschmoczer elaborate on this. |
||
| self.tokenProvider.accessToken | ||
| .compactMap { $0 } | ||
| .setFailureType(to: Error.self) | ||
| .flatMap { accessToken -> AnyPublisher<T, Error> in | ||
| return appendTokenAndExecute(accessToken: accessToken) | ||
| } | ||
| } | ||
| .timeout(10, scheduler: DispatchQueue.main, customError: { NetworkError.timeout }) | ||
|
There was a problem hiding this comment. Again, I don't think this timeout and queue logic is responsibility of this framework There was a problem hiding this comment. if there is no timeOut, the request will hang forever. so it's an extra thing to think about for the consumer. Token refreshing is a core feature of this framework, which comes with the retry mechanism and so on. If we keep all in but the timeout outside, then the consumer has to think about this extra responsibility. Does it make a difference if it's done inside or outside? |
||
| .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() | ||
| } | ||
| .receive(on: DispatchQueue.main) | ||
|
There was a problem hiding this comment. It's a design decision that I took while creating the framework. For the consumer it doesn't change anything actually but I wanted fusion to provide a hassle-free development for the consumer. Therefore all the executions happen on the background thread and the deliveries happen on the main queue. If at any point a consumer wants to use a different thread, they are free to use |
||
| .eraseToAnyPublisher() | ||
| } | ||
|
|
||
| override public func execute(urlRequest: URLRequest) -> AnyPublisher<Void, Error> { | ||
| var urlRequest = urlRequest | ||
|
|
||
| func appendTokenAndExecute(accessToken: AccessToken) -> AnyPublisher<Void, Error> { | ||
| urlRequest.setValue(self.configuration.authorizationHeaderScheme.rawValue + accessToken, forHTTPHeaderField: "Authorization") | ||
|
There was a problem hiding this comment. Consumer is deferred already. |
||
| return super.execute(urlRequest: urlRequest) | ||
| .subscribe(on: DispatchQueue.global()) | ||
| .eraseToAnyPublisher() | ||
| } | ||
|
|
||
| return Deferred { | ||
| self.tokenProvider.accessToken | ||
|
There was a problem hiding this comment. deferred can be removed |
||
| .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) | ||
|
Comment on lines
+111
to
+123
There was a problem hiding this comment. you have duplicated calls to There was a problem hiding this comment. yes, it looks weird indeed. This was a recursive function before, for the sake of readability and understandability we changed it to this. So the first appending happens when the request is executed for the first time, the second one is if it fails due to a 401. So it decorates the request and fires again. Should we go back to the recursion? |
||
| }.eraseToAnyPublisher() | ||
| } | ||
| return Fail<Void, Error>(error: error).eraseToAnyPublisher() | ||
| } | ||
| .receive(on: DispatchQueue.main) | ||
|
There was a problem hiding this comment. should be responsibility of the consumer There was a problem hiding this comment. It's a design decision that I took while creating the framework. For the consumer it doesn't change anything actually but I wanted fusion to provide a hassle-free development for the consumer. Therefore all the executions happen on the background thread and the deliveries happen on the main queue. If at any point a consumer wants to use a different thread, they are free to use |
||
| .eraseToAnyPublisher() | ||
| } | ||
|
|
||
| private func retrySynchronizedTokenRefresh() -> AnyPublisher<AccessToken, Error> { | ||
| tokenProvider.invalidateAccessToken() | ||
|
There was a problem hiding this comment. this should be deffered There was a problem hiding this comment. the consumer of this publisher is already deferred |
||
| return tokenProvider.reissueAccessToken() | ||
| .eraseToAnyPublisher() | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think networking framework should decide where to subscribe and publish it's changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a design decision that I took while creating the framework. For the consumer it doesn't change anything actually but I wanted fusion to provide a hassle-free development for the consumer. Therefore all the executions happen on the background thread and the deliveries happen on the main queue.
If at any point a consumer wants to use a different thread, they are free to use
subscribe or receive