Skip to content

Comments

Token Refresh Offset#11704

Closed
jianghaolu wants to merge 57 commits intoAzure:masterfrom
jianghaolu:tokenrefreshoffset
Closed

Token Refresh Offset#11704
jianghaolu wants to merge 57 commits intoAzure:masterfrom
jianghaolu:tokenrefreshoffset

Conversation

@jianghaolu
Copy link
Contributor

@jianghaolu jianghaolu commented Jun 3, 2020

Summary

We are moving the token refresh offset from AccessToken to TokenCredential, to keep the originality and correctness of the expiresAt() getter on AccessToken.
On top of refreshing the token at an offset before expiry, we are also allowing the current non-expired token to be used if a proactive refresh fails. The proactive refresh will be attempted again next time some thread needs to get a token. To prevent too many refresh attempts, there will also be a timeout in place.

Proactive token refresh design

Initial token acquisition

The initial token acquisition will be done by the first thread entering the getToken() and all other threads will subscribe to a publisher that will emit the token once acquired.

For the first thread, once the token is acquired, it's cached and sent to the publisher. The entire operation on this thread is atomic.

When a valid token is in cache

When multiple threads are reading from the cache, at any given time one thread will check the token validity and if any proactive token refresh should happen. If not, it simply falls back to the cached token with minimal performance hit.

Proactive token refresh

When a thread is checking the validity of a token, shouldRefresh predicate may return true before the token expires. The token is considered "about to expire" for our customer and this thread is responsible for doing a proactive token refresh.

If the refresh succeeds, the cached token will be updated. If the refresh fails, no error will be raised (will be logged) and the currently cached token will continue to be used. The next refresh attempt will not be allowed until 30 seconds later - the next thread coming in within 30 seconds will not attempt a proactive token refresh.

Likewise, these are all happening on one thread at any given time. All the others are simply getting the cached token emitted by the publisher.

After token expires

If proactive token refreshes keep failing, or a token hasn't been checked until the currently cached token has expired, the next thread calling getToken() will be responsible for acquiring a new token. If there is a 30 seconds refresh timeout currently in place, since there are no more available tokens to fallback to, the thread will wait for the timeout to be over to attempt another token refresh.

If the refresh fails at this point, we have run out of available tokens, thus raising an error to the downstream.

@jianghaolu jianghaolu requested a review from g2vinay June 4, 2020 00:20
@jianghaolu jianghaolu changed the title Tokenrefreshoffset Token Refresh Offset Jun 4, 2020
public AccessToken(String token, OffsetDateTime expiresAt) {
this.token = token;
this.expiresAt = expiresAt.minusMinutes(2); // 2 minutes before token expires
this.expiresAt = expiresAt;
Copy link
Member

Choose a reason for hiding this comment

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

This will be a breaking change for developers not using the Identity library and using this class directly from the core for their custom credentials.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I synced with @srnagar and he confirmed this is allowed in a minor version bump. It's not an API change but a behavioral change/fix. Users using this in conjunction with BearerTokenAuthenticationPolicy will experience the same behavior as the token cache now handles the 2 minutes default.

@JimSuplizio
Copy link
Contributor

@jianghaolu I think you may need to rebase with master. The changes in version_client.txt that are in this PR are already in master but the versions in the pom files in your fork aren't going to match which is going to cause the analyze step to fail.

@jianghaolu
Copy link
Contributor Author

jianghaolu commented Jun 4, 2020

Thanks @JimSuplizio! Pulling in master.

@jianghaolu jianghaolu marked this pull request as ready for review June 16, 2020 23:53
@jianghaolu jianghaolu requested a review from danieljurek as a code owner June 16, 2020 23:53
@jianghaolu
Copy link
Contributor Author

Moved to #13029

@jianghaolu jianghaolu closed this Jul 10, 2020
@jianghaolu jianghaolu deleted the tokenrefreshoffset branch October 5, 2020 18:24
openapi-sdkautomation bot referenced this pull request in AzureSDKAutomation/azure-sdk-for-java Nov 20, 2020
Microsoft.Compute: Include CloudServices Resource Swagger under API Version 2020-10-01-preview (#11704)

Co-authored-by: Sanchit Kumar <Sanchit.Kumar@microsoft.com>
openapi-sdkautomation bot referenced this pull request in AzureSDKAutomation/azure-sdk-for-java Nov 20, 2020
Microsoft.Compute: Include CloudServices Resource Swagger under API Version 2020-10-01-preview (#11704)

Co-authored-by: Sanchit Kumar <Sanchit.Kumar@microsoft.com>
openapi-sdkautomation bot referenced this pull request in AzureSDKAutomation/azure-sdk-for-java Nov 20, 2020
Microsoft.Compute: Include CloudServices Resource Swagger under API Version 2020-10-01-preview (#11704)

Co-authored-by: Sanchit Kumar <Sanchit.Kumar@microsoft.com>
openapi-sdkautomation bot referenced this pull request in AzureSDKAutomation/azure-sdk-for-java Nov 20, 2020
Microsoft.Compute: Include CloudServices Resource Swagger under API Version 2020-10-01-preview (#11704)

Co-authored-by: Sanchit Kumar <Sanchit.Kumar@microsoft.com>
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.

5 participants