Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CIS-1167] Implement token refreshing for HTTP requests #1446

Merged
merged 9 commits into from Sep 18, 2021

Conversation

dmigach
Copy link
Contributor

@dmigach dmigach commented Sep 13, 2021

Currently we only refresh expired token when we get an error over WebSocket connection which is suboptimal because backend doesn't send an event for token expiration. So we should react to this error when we get it as a response for an HTTP request

There are three main things to consider, all covered with a corresponding test:

When request fail, we add them to a queue to retry them later with a new token.
Then we try refreshing the token, if it's successful - retry all the failed requests
If refreshing failed or hanging - timeout all the requests with failures

Two main cases to test:

  1. Token expires, you send a couple of requests (like creating new messages). They hang for a bit and then retried once the token is refreshed
  2. Token expires, token refreshing fails, requests should fail after the timeout

@Stream-iOS-Bot
Copy link
Collaborator

Stream-iOS-Bot commented Sep 13, 2021

4 Errors
🚫 Please start subject with capital letter.
e7dec70
🚫 Please use more than one word.
e7dec70
🚫 Please start subject with capital letter.
bbfe858
🚫 Please start subject with capital letter.
b236382

Generated by 🚫 Danger

Sources/StreamChat/APIClient/APIClient.swift Outdated Show resolved Hide resolved
Sources/StreamChat/APIClient/APIClient.swift Outdated Show resolved Hide resolved
Sources/StreamChat/APIClient/APIClient.swift Outdated Show resolved Hide resolved
Sources/StreamChat/APIClient/APIClient.swift Outdated Show resolved Hide resolved
Sources/StreamChat/APIClient/APIClient.swift Outdated Show resolved Hide resolved
Sources/StreamChat/APIClient/APIClient.swift Show resolved Hide resolved
Sources/StreamChat/APIClient/APIClient.swift Show resolved Hide resolved
nuno-vieira
nuno-vieira previously approved these changes Sep 17, 2021
Copy link
Member

@nuno-vieira nuno-vieira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Atomic private var tokenRefreshConsecutiveFailures: Int = 0

/// How many times can the token refresh fail before giving up with an error
let maxTokenRefreshAttempts = 10
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this too much? 🤔 AFAIK on iOS the default for this kind of thing is 5.

@tbarbugli tbarbugli merged commit cd2c469 into main Sep 18, 2021
@tbarbugli tbarbugli deleted the CIS-1167-token-exiration branch September 18, 2021 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants