From 8f253b62b4e7219d976d135412470f2e165fe3b5 Mon Sep 17 00:00:00 2001 From: Daniel Rodriguez Date: Mon, 3 May 2021 17:53:06 +0000 Subject: [PATCH 1/2] [core-rest-pipeline] Idea for improving the tokenCycler --- .../bearerTokenAuthenticationPolicy.ts | 2 +- ...earerTokenChallengeAuthenticationPolicy.ts | 4 +- .../src/util/tokenCycler.ts | 149 +++++++++--------- 3 files changed, 78 insertions(+), 77 deletions(-) diff --git a/sdk/core/core-rest-pipeline/src/policies/bearerTokenAuthenticationPolicy.ts b/sdk/core/core-rest-pipeline/src/policies/bearerTokenAuthenticationPolicy.ts index d19519bb0bb4..9b8b8d1ccfdc 100644 --- a/sdk/core/core-rest-pipeline/src/policies/bearerTokenAuthenticationPolicy.ts +++ b/sdk/core/core-rest-pipeline/src/policies/bearerTokenAuthenticationPolicy.ts @@ -43,7 +43,7 @@ export function bearerTokenAuthenticationPolicy( return { name: bearerTokenAuthenticationPolicyName, async sendRequest(request: PipelineRequest, next: SendRequest): Promise { - const { token } = await cycler.getToken(scopes, { + const { token } = await cycler(scopes, { abortSignal: request.abortSignal, tracingOptions: request.tracingOptions }); diff --git a/sdk/core/core-rest-pipeline/src/policies/bearerTokenChallengeAuthenticationPolicy.ts b/sdk/core/core-rest-pipeline/src/policies/bearerTokenChallengeAuthenticationPolicy.ts index 02f0b76320ab..5902cb4e01e6 100644 --- a/sdk/core/core-rest-pipeline/src/policies/bearerTokenChallengeAuthenticationPolicy.ts +++ b/sdk/core/core-rest-pipeline/src/policies/bearerTokenChallengeAuthenticationPolicy.ts @@ -157,7 +157,7 @@ export function bearerTokenChallengeAuthenticationPolicy( await callbacks.authorizeRequest({ scopes, request, - getAccessToken: cycler.getToken + getAccessToken: cycler }); let response: PipelineResponse; @@ -179,7 +179,7 @@ export function bearerTokenChallengeAuthenticationPolicy( scopes, request, response, - getAccessToken: cycler.getToken + getAccessToken: cycler }); if (shouldSendRequest) { diff --git a/sdk/core/core-rest-pipeline/src/util/tokenCycler.ts b/sdk/core/core-rest-pipeline/src/util/tokenCycler.ts index 4e096a667110..e6d80c1888db 100644 --- a/sdk/core/core-rest-pipeline/src/util/tokenCycler.ts +++ b/sdk/core/core-rest-pipeline/src/util/tokenCycler.ts @@ -15,14 +15,6 @@ export type AccessTokenGetter = ( options: GetTokenOptions ) => Promise; -/** - * The response of the - */ -export interface AccessTokenRefresher { - cachedToken: AccessToken | null; - getToken: AccessTokenGetter; -} - export interface TokenCyclerOptions { /** * The window of time before token expiration during which the token will be @@ -52,50 +44,61 @@ export const DEFAULT_CYCLER_OPTIONS: TokenCyclerOptions = { }; /** - * Converts an an unreliable access token getter (which may resolve with null) - * into an AccessTokenGetter by retrying the unreliable getter in a regular - * interval. + * Calls to asyncFn until the timeout is reached. + * Prevents errors from bubbling up until the refresh timeout is reached. + * Once the timeout is reached, errors will bubble up, and null return values will throw with the received "errorMessage". * - * @param getAccessToken - A function that produces a promise of an access token that may fail by returning null. - * @param retryIntervalInMs - The time (in milliseconds) to wait between retry attempts. - * @param refreshTimeout - The timestamp after which the refresh attempt will fail, throwing an exception. - * @returns - A promise that, if it resolves, will resolve with an access token. + * @param asyncFn - The asynchronous function to call. + * @param timeout - Unix time when to stop ignoring errors or null values. + * @param errorMessage - Message to be used if the final value after the timeout is reached is still "null". + * @returns - A promise that can be resolved with T or null until the time is reached, after which it will throw if it can't resolve with a value of the type T. */ -async function beginRefresh( - getAccessToken: () => Promise, - retryIntervalInMs: number, - refreshTimeout: number -): Promise { - // This wrapper handles exceptions gracefully as long as we haven't exceeded - // the timeout. - async function tryGetAccessToken(): Promise { - if (Date.now() < refreshTimeout) { - try { - return await getAccessToken(); - } catch { - return null; - } - } else { - const finalToken = await getAccessToken(); - - // Timeout is up, so throw if it's still null - if (finalToken === null) { - throw new Error("Failed to refresh access token."); - } - - return finalToken; +async function tryUntilTimeout( + asyncFn: () => Promise, + timeout: number, + errorMessage: string +): Promise { + if (Date.now() < timeout) { + try { + return asyncFn(); + } catch { + return null; + } + } else { + const result = await asyncFn(); + + // Timeout is up, so throw if it's still null + if (result === null) { + throw new Error(errorMessage); } + return result; } +} - let token: AccessToken | null = await tryGetAccessToken(); +/** + * Retries an asynchronous function as often as the provided interval milliseconds, + * until the provided Unix date timeout is reached. + * Once the timeout is reached, errors will bubble up, and null return values will throw with the received "errorMessage". + * + * @param asyncFn - The asynchronous function to call. + * @param retryIntervalInMs - The time (in milliseconds) to wait between retry attempts. + * @param refreshTimeout - The timestamp after which the refresh attempt will fail, throwing an exception. + * @returns - A promise that, if it resolves, will resolve with the T type. + */ +async function retryUntilTimeout( + asyncFn: () => Promise, + retryIntervalInMs: number, + retryTimeout: number, + errorMessage: string +): Promise { + let result = await tryUntilTimeout(asyncFn, retryTimeout, errorMessage); - while (token === null) { + while (result === null) { await delay(retryIntervalInMs); - - token = await tryGetAccessToken(); + result = await tryUntilTimeout(asyncFn, retryTimeout, errorMessage); } - return token; + return result; } /** @@ -115,7 +118,7 @@ async function beginRefresh( export function createTokenCycler( credential: TokenCredential, tokenCyclerOptions?: Partial -): AccessTokenRefresher { +): AccessTokenGetter { let refreshWorker: Promise | null = null; let token: AccessToken | null = null; @@ -124,11 +127,13 @@ export function createTokenCycler( ...tokenCyclerOptions }; + const errorMessage = "Failed to refresh access token."; + /** * This little holder defines several predicates that we use to construct * the rules of refreshing the token. */ - const cycler = { + const cyclerState = { /** * Produces true if a refresh job is currently in progress. */ @@ -141,7 +146,7 @@ export function createTokenCycler( */ get shouldRefresh(): boolean { return ( - !cycler.isRefreshing && + !cyclerState.isRefreshing && (token?.expiresOnTimestamp ?? 0) - options.refreshWindowInMs < Date.now() ); }, @@ -164,18 +169,19 @@ export function createTokenCycler( scopes: string | string[], getTokenOptions: GetTokenOptions ): Promise { - if (!cycler.isRefreshing) { + if (!cyclerState.isRefreshing) { // We bind `scopes` here to avoid passing it around a lot const tryGetAccessToken = (): Promise => credential.getToken(scopes, getTokenOptions); // Take advantage of promise chaining to insert an assignment to `token` // before the refresh can be considered done. - refreshWorker = beginRefresh( + refreshWorker = retryUntilTimeout( tryGetAccessToken, options.retryIntervalInMs, // If we don't have a token, then we should timeout immediately - token?.expiresOnTimestamp ?? Date.now() + token?.expiresOnTimestamp ?? Date.now(), + errorMessage ) .then((_token) => { refreshWorker = null; @@ -195,31 +201,26 @@ export function createTokenCycler( return refreshWorker as Promise; } - return { - get cachedToken(): AccessToken | null { - return token; - }, - getToken: async ( - scopes: string | string[], - tokenOptions: GetTokenOptions - ): Promise => { - // - // Simple rules: - // - If we MUST refresh, then return the refresh task, blocking - // the pipeline until a token is available. - // - If we SHOULD refresh, then run refresh but don't return it - // (we can still use the cached token). - // - Return the token, since it's fine if we didn't return in - // step 1. - // - - if (cycler.mustRefresh) return refresh(scopes, tokenOptions); - - if (cycler.shouldRefresh) { - refresh(scopes, tokenOptions); - } - - return token as AccessToken; + return async function( + scopes: string | string[], + tokenOptions: GetTokenOptions + ): Promise { + // + // Simple rules: + // - If we MUST refresh, then return the refresh task, blocking + // the pipeline until a token is available. + // - If we SHOULD refresh, then run refresh but don't return it + // (we can still use the cached token). + // - Return the token, since it's fine if we didn't return in + // step 1. + // + + if (cyclerState.mustRefresh) return refresh(scopes, tokenOptions); + + if (cyclerState.shouldRefresh) { + refresh(scopes, tokenOptions); } + + return token as AccessToken; }; } From 8217ca6e261fd269b511e5aed1b45598a4715bfc Mon Sep 17 00:00:00 2001 From: Daniel Rodriguez Date: Wed, 19 May 2021 22:16:37 +0000 Subject: [PATCH 2/2] recent feedback by Will --- .../bearerTokenAuthenticationPolicy.ts | 4 +- .../src/util/tokenCycler.ts | 45 +++++++++---------- .../bearerTokenAuthenticationPolicy.spec.ts | 4 +- 3 files changed, 24 insertions(+), 29 deletions(-) diff --git a/sdk/core/core-rest-pipeline/src/policies/bearerTokenAuthenticationPolicy.ts b/sdk/core/core-rest-pipeline/src/policies/bearerTokenAuthenticationPolicy.ts index 9b8b8d1ccfdc..b9318514e8dd 100644 --- a/sdk/core/core-rest-pipeline/src/policies/bearerTokenAuthenticationPolicy.ts +++ b/sdk/core/core-rest-pipeline/src/policies/bearerTokenAuthenticationPolicy.ts @@ -38,12 +38,12 @@ export function bearerTokenAuthenticationPolicy( // The options are left out of the public API until there's demand to configure this. // Remember to extend `BearerTokenAuthenticationPolicyOptions` with `TokenCyclerOptions` // in order to pass through the `options` object. - const cycler = createTokenCycler(credential /* , options */); + const getToken = createTokenCycler(credential /* , options */); return { name: bearerTokenAuthenticationPolicyName, async sendRequest(request: PipelineRequest, next: SendRequest): Promise { - const { token } = await cycler(scopes, { + const { token } = await getToken(scopes, { abortSignal: request.abortSignal, tracingOptions: request.tracingOptions }); diff --git a/sdk/core/core-rest-pipeline/src/util/tokenCycler.ts b/sdk/core/core-rest-pipeline/src/util/tokenCycler.ts index e6d80c1888db..161d9d849ab9 100644 --- a/sdk/core/core-rest-pipeline/src/util/tokenCycler.ts +++ b/sdk/core/core-rest-pipeline/src/util/tokenCycler.ts @@ -44,32 +44,31 @@ export const DEFAULT_CYCLER_OPTIONS: TokenCyclerOptions = { }; /** - * Calls to asyncFn until the timeout is reached. + * Calls to producer until the timeout is reached. * Prevents errors from bubbling up until the refresh timeout is reached. * Once the timeout is reached, errors will bubble up, and null return values will throw with the received "errorMessage". * - * @param asyncFn - The asynchronous function to call. + * @param producer - The asynchronous function to call. * @param timeout - Unix time when to stop ignoring errors or null values. * @param errorMessage - Message to be used if the final value after the timeout is reached is still "null". * @returns - A promise that can be resolved with T or null until the time is reached, after which it will throw if it can't resolve with a value of the type T. */ async function tryUntilTimeout( - asyncFn: () => Promise, - timeout: number, - errorMessage: string + producer: () => Promise, + timeout: number ): Promise { if (Date.now() < timeout) { try { - return asyncFn(); + return await producer(); } catch { return null; } } else { - const result = await asyncFn(); + const result = await producer(); // Timeout is up, so throw if it's still null if (result === null) { - throw new Error(errorMessage); + throw new Error("operation timed out"); } return result; } @@ -80,22 +79,21 @@ async function tryUntilTimeout( * until the provided Unix date timeout is reached. * Once the timeout is reached, errors will bubble up, and null return values will throw with the received "errorMessage". * - * @param asyncFn - The asynchronous function to call. + * @param producer - The asynchronous function to call. * @param retryIntervalInMs - The time (in milliseconds) to wait between retry attempts. * @param refreshTimeout - The timestamp after which the refresh attempt will fail, throwing an exception. * @returns - A promise that, if it resolves, will resolve with the T type. */ async function retryUntilTimeout( - asyncFn: () => Promise, + producer: () => Promise, retryIntervalInMs: number, - retryTimeout: number, - errorMessage: string + retryTimeout: number ): Promise { - let result = await tryUntilTimeout(asyncFn, retryTimeout, errorMessage); + let result = await tryUntilTimeout(producer, retryTimeout); while (result === null) { await delay(retryIntervalInMs); - result = await tryUntilTimeout(asyncFn, retryTimeout, errorMessage); + result = await tryUntilTimeout(producer, retryTimeout); } return result; @@ -127,13 +125,11 @@ export function createTokenCycler( ...tokenCyclerOptions }; - const errorMessage = "Failed to refresh access token."; - /** * This little holder defines several predicates that we use to construct * the rules of refreshing the token. */ - const cyclerState = { + const self = { /** * Produces true if a refresh job is currently in progress. */ @@ -146,7 +142,7 @@ export function createTokenCycler( */ get shouldRefresh(): boolean { return ( - !cyclerState.isRefreshing && + !self.isRefreshing && (token?.expiresOnTimestamp ?? 0) - options.refreshWindowInMs < Date.now() ); }, @@ -169,7 +165,7 @@ export function createTokenCycler( scopes: string | string[], getTokenOptions: GetTokenOptions ): Promise { - if (!cyclerState.isRefreshing) { + if (!self.isRefreshing) { // We bind `scopes` here to avoid passing it around a lot const tryGetAccessToken = (): Promise => credential.getToken(scopes, getTokenOptions); @@ -180,21 +176,20 @@ export function createTokenCycler( tryGetAccessToken, options.retryIntervalInMs, // If we don't have a token, then we should timeout immediately - token?.expiresOnTimestamp ?? Date.now(), - errorMessage + token?.expiresOnTimestamp ?? Date.now() ) .then((_token) => { refreshWorker = null; token = _token; return token; }) - .catch((reason) => { + .catch((error) => { // We also should reset the refresher if we enter a failed state. All // existing awaiters will throw, but subsequent requests will start a // new retry chain. refreshWorker = null; token = null; - throw reason; + throw new Error(`Failed to refresh access token: ${error.message}`); }); } @@ -215,9 +210,9 @@ export function createTokenCycler( // step 1. // - if (cyclerState.mustRefresh) return refresh(scopes, tokenOptions); + if (self.mustRefresh) return refresh(scopes, tokenOptions); - if (cyclerState.shouldRefresh) { + if (self.shouldRefresh) { refresh(scopes, tokenOptions); } diff --git a/sdk/core/core-rest-pipeline/test/bearerTokenAuthenticationPolicy.spec.ts b/sdk/core/core-rest-pipeline/test/bearerTokenAuthenticationPolicy.spec.ts index cdc12a215d95..52ccca318d28 100644 --- a/sdk/core/core-rest-pipeline/test/bearerTokenAuthenticationPolicy.spec.ts +++ b/sdk/core/core-rest-pipeline/test/bearerTokenAuthenticationPolicy.spec.ts @@ -172,7 +172,7 @@ describe("BearerTokenAuthenticationPolicy", function() { } catch (e) { error = e; } - assert.equal(error?.message, "Failed to retrieve the token"); + assert.equal(error?.message, "Failed to refresh access token: Test Error"); assert.strictEqual( credential.authCount, @@ -248,7 +248,7 @@ class MockRefreshAzureCredential implements TokenCredential { this.authCount++; if (this.shouldThrow) { - throw new Error("Failed to retrieve the token"); + throw new Error("Test Error"); } // Allowing getToken to take a while