Overhaul Refresh Token Retry Mechanism #4

erenkabakci · 2020-04-28T10:34:46Z

Changing the retry token mechanism and dropping the usage of DispatchGroup. Currently refresh token requests can run on parallel and will refresh tokens for themselves respectively but only the last valid one will be used in the next call. So we don't prevent firing them anymore but defer the usage of the accessToken

This is actually a setback in terms of atomic refresh token request execution, but the consumer app showed that the dispatchGroup usage was causing a deadlock.
refreshTriggeringErrors is now a [fusion.NetworkError] rather than [Error]. This was a poor designing since fusion can never evaluate based on consumer app's custom error. The consumer should provide an appropriate fusion error to trigger the refreshing mechanism.
Explicit thread management by adding receive(on: DispatchQueue.global()) and receive(on: DispatchQueue.main) to every execution of the API call to ensure that returning threads are not UI blocking and delivery still happens on the main thread.
Changes in the AuthenticationTokenProvidable API contract reissueAccessToken() is not returning a publisher.

Known issue #5 : One test is disabled due to an unknown (possibly threading) issue causing multiple other tests fail. This should ideally be fixed and still using EntwineTest for better observation of the stream.

GlebRadchenko · 2020-04-28T11:08:07Z

Sources/WebService/AuthenticatedWebService/AuthenticatedWebService.swift

-      dispatchGroup.wait()
-    }
+  private func retrySynchronizedTokenRefresh() -> AnyPublisher<AccessToken, Error> {
+    tokenProvider.invalidateAccessToken()


this should be deffered

the consumer of this publisher is already deferred

GlebRadchenko · 2020-04-28T11:08:58Z

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())


I don't think networking framework should decide where to subscribe and publish it's changes

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

GlebRadchenko · 2020-04-28T11:09:21Z

Sources/WebService/AuthenticatedWebService/AuthenticatedWebService.swift

-    authenticationQueue.sync {
-      currentAccessToken = self.tokenProvider.accessToken.value
+    func appendTokenAndExecute(accessToken: AccessToken) -> AnyPublisher<T, Error> {
+      urlRequest.setValue(self.configuration.authorizationHeaderScheme.rawValue + accessToken, forHTTPHeaderField: "Authorization")


should be deffered

again, the consumer of this is deferred already. There is no direct access to this publisher without any defer mechanism.

GlebRadchenko · 2020-04-28T11:09:47Z

Sources/WebService/AuthenticatedWebService/AuthenticatedWebService.swift

+    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())


same here, don't think it's responsibility of the framework itself

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

GlebRadchenko · 2020-04-28T11:10:17Z

Sources/WebService/AuthenticatedWebService/AuthenticatedWebService.swift

+        .eraseToAnyPublisher()
+    }
+
+    return Deferred {


this deferred can be removed

This, I am not sure since this was sort of the fixing point for the racing refrehTokens. I will let @emanuelschmoczer elaborate on this.

GlebRadchenko · 2020-04-28T11:12:06Z

Sources/WebService/AuthenticatedWebService/AuthenticatedWebService.swift

-    authenticationQueue.sync {
-      currentAccessToken = self.tokenProvider.accessToken.value
+    func appendTokenAndExecute(accessToken: AccessToken) -> AnyPublisher<Void, Error> {
+      urlRequest.setValue(self.configuration.authorizationHeaderScheme.rawValue + accessToken, forHTTPHeaderField: "Authorization")


should be deferred

Consumer is deferred already.

GlebRadchenko · 2020-04-28T11:12:26Z

Sources/WebService/AuthenticatedWebService/AuthenticatedWebService.swift

-    guard let accessToken = currentAccessToken else {
-      return Fail<Void, Error>(error: NetworkError.unauthorized).eraseToAnyPublisher()
+    return Deferred {
+      self.tokenProvider.accessToken


deferred can be removed

@emanuelschmoczer

GlebRadchenko · 2020-04-28T11:13:11Z

Sources/WebService/AuthenticatedWebService/AuthenticatedWebService.swift

+          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)


you have duplicated calls to appendTokenAndExecute in one function, which I think design issue

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?

GlebRadchenko · 2020-04-28T11:13:31Z

Sources/WebService/AuthenticatedWebService/AuthenticatedWebService.swift

-        }
-        return Fail<Void, Error>(error: error).eraseToAnyPublisher()
-    }.eraseToAnyPublisher()
+    .receive(on: DispatchQueue.main)


should be responsibility of the consumer

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

GlebRadchenko · 2020-04-28T11:14:29Z

Tests/MockSession.swift

-        promise(.failure(NetworkError.urlError(errorResponse)))
+    return Deferred {
+      Future<(data: Data, response: URLResponse), Error> { promise in
+        usleep(20)


why do we need this timeout? it doesn't make any sense to me and looks like a system design issue

I put it just to simulate a slower execution in tests. It has nothing to do with the production code. I wanted to execute them slightly after each other. I guess it can be removed.

erenkabakci added 3 commits April 27, 2020 20:56

Overhaul token refresh mechanism and discard dispatchGroup

31e9537

Change testing approach drastically

c380259

Update swift.yml

8ac9e97

erenkabakci added bug Something isn't working enhancement New feature or request labels Apr 28, 2020

erenkabakci requested review from emanuelschmoczer and ediazest April 28, 2020 10:34

erenkabakci self-assigned this Apr 28, 2020

erenkabakci mentioned this pull request Apr 28, 2020

Two parallel Entwine tests are causing flakiness and random failures of others #5

Closed

emanuelschmoczer approved these changes Apr 28, 2020

View reviewed changes

emanuelschmoczer merged commit 8512cd2 into master Apr 28, 2020

GlebRadchenko reviewed Apr 28, 2020

View reviewed changes

ediazest mentioned this pull request Apr 29, 2020

Reactive refresh token #6

Closed

erenkabakci added a commit that referenced this pull request Sep 24, 2020

Overhaul Refresh Token Retry Mechanism (#4)

d2fd473

erenkabakci deleted the feature/reactiveRefreshToken branch November 30, 2020 12:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overhaul Refresh Token Retry Mechanism #4

Overhaul Refresh Token Retry Mechanism #4

erenkabakci commented Apr 28, 2020 •

edited

GlebRadchenko Apr 28, 2020

erenkabakci Apr 28, 2020

GlebRadchenko Apr 28, 2020

erenkabakci Apr 28, 2020

GlebRadchenko Apr 28, 2020

erenkabakci Apr 28, 2020

GlebRadchenko Apr 28, 2020

erenkabakci Apr 28, 2020

GlebRadchenko Apr 28, 2020

erenkabakci Apr 28, 2020

GlebRadchenko Apr 28, 2020

erenkabakci Apr 28, 2020

GlebRadchenko Apr 28, 2020

erenkabakci Apr 28, 2020

GlebRadchenko Apr 28, 2020

erenkabakci Apr 28, 2020

GlebRadchenko Apr 28, 2020

erenkabakci Apr 28, 2020

GlebRadchenko Apr 28, 2020 •

edited

erenkabakci Apr 28, 2020

Overhaul Refresh Token Retry Mechanism #4

Overhaul Refresh Token Retry Mechanism #4

Conversation

erenkabakci commented Apr 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GlebRadchenko Apr 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erenkabakci commented Apr 28, 2020 •

edited

GlebRadchenko Apr 28, 2020 •

edited