Repository: auth0/auth0-spa-js (ts)

Issue:
getTokenSilently is slow, even with the cache
### Describe the problem

Calls to `getTokenSilently()` are slow, on the order of 50ms. In our case this breaks a subsequent use of [`apollo-link-batch-http`](https://www.apollographql.com/docs/link/links/batch-http/), as the delay pushes every query outside of the batching window.

The extra time comes from acquiring a lock to avoid multiple requests. The implementation was changed to fix this in https://github.com/auth0/auth0-spa-js/pull/339, but reverted in https://github.com/auth0/auth0-spa-js/pull/525 since it seems to have caused other unwanted behaviour.

### What was the expected behavior?

Calls should only take a few milliseconds, as they do with `getIdTokenClaims`.

### Reproduction

1.  In the SDK Playground's `index.html`, replace the implementation of `showAuth0Info` with:
```js
        showAuth0Info: function () {
          var _self = this;
          _self.access_tokens = [];

          _self.auth0.getTokenSilently().then(function (token) {
            _self.audienceScopes[0].access_tokens.push(token);

            _self.auth0.getUser().then(function (user) {
              _self.profile = user;
            });

            _self.auth0.getIdTokenClaims().then(function (claims) {
              _self.id_token = claims.__raw;
            });
          }).then(() => {
            const start = performance.now();

            _self.auth0.getIdTokenClaims().then(() => {
              console.log(`getIdTokenClaims took ${performance.now() - start}ms`);
            });

            _self.auth0.getTokenSilently().then(() => {
              console.log(`getTokenSilently took ${performance.now() - start}ms`);
            });
          });
        },
```

2. Login via "Login popup" a few times.

3. Note the difference in processing times for the two methods. In our app we've seen a range of roughly 15ms - 90ms depending on whatever else the app is doing at the time.

```
getIdTokenClaims took 10ms
getTokenSilently took 81ms

getIdTokenClaims took 7ms
getTokenSilently took 79ms

getIdTokenClaims took 7ms
getTokenSilently took 67ms

getIdTokenClaims took 10ms
getTokenSilently took 80ms

getIdTokenClaims took 10ms
getTokenSilently took 77ms
```

(Currently on package version 1.11.0)

Cheers.

Pull request:
[SDK-1912] Unnecessary latency in `getTokenSilently` with primed cache
### Description

Check the cache before acquiring lock to avoid latency when the cache is populated (see #552)
Check the cache after acquiring lock to prevent unnecessary requests, because a previous call might have populated the cache while the current call was waiting to acquire the lock (see #480)

### References

fixes #552

### Testing

To check #552:

Call `getTokenSilently` with a primed cache, latency should be <20ms

Check regression of #480:

Hammer the `getTokenSilently` button in the SPA js playground with "Use token cache when fetching new tokens" enabled and check that only one token is requested 

### Checklist

- [ ] I have added documentation for new/changed functionality in this PR or in auth0.com/docs
- [ ] All active GitHub checks for tests, formatting, and security are passing
- [ ] The correct base branch is being used, if not `master`


Patch:
diff --git a/__mocks__/browser-tabs-lock/index.ts b/__mocks__/browser-tabs-lock/index.ts
index ea23563..be5dfc5 100644
--- a/__mocks__/browser-tabs-lock/index.ts
+++ b/__mocks__/browser-tabs-lock/index.ts
@@ -1,7 +1,15 @@
-export const acquireLockMock = jest.fn();
-export const releaseLockMock = jest.fn();
+const Lock = jest.requireActual('browser-tabs-lock').default;
 
-export default class FakeLock {
-  acquireLock = acquireLockMock;
-  releaseLock = releaseLockMock;
+export const acquireLockSpy = jest.fn();
+export const releaseLockSpy = jest.fn();
+
+export default class extends Lock {
+  acquireLock(...args) {
+    acquireLockSpy(...args);
+    return super.acquireLock(...args);
+  }
+  releaseLock(...args) {
+    releaseLockSpy(...args);
+    return super.releaseLock(...args);
+  }
 }
diff --git a/src/Auth0Client.ts b/src/Auth0Client.ts
index fa965af..e1fb764 100644
--- a/src/Auth0Client.ts
+++ b/src/Auth0Client.ts
@@ -582,22 +582,36 @@ export default class Auth0Client {
       scope: getUniqueScopes(this.defaultScope, this.scope, options.scope)
     };
 
+    const getAccessTokenFromCache = () => {
+      const cache = this.cache.get(
+        {
+          scope: getTokenOptions.scope,
+          audience: getTokenOptions.audience || 'default',
+          client_id: this.options.client_id
+        },
+        60 // get a new token if within 60 seconds of expiring
+      );
+
+      return cache && cache.access_token;
+    };
+
+    // Check the cache before acquiring the lock to avoid the latency of
+    // `lock.acquireLock` when the cache is populated.
+    if (!ignoreCache) {
+      let accessToken = getAccessTokenFromCache();
+      if (accessToken) {
+        return accessToken;
+      }
+    }
+
     try {
       await lock.acquireLock(GET_TOKEN_SILENTLY_LOCK_KEY, 5000);
-
+      // Check the cache a second time, because it may have been populated
+      // by a previous call while this call was waiting to acquire the lock.
       if (!ignoreCache) {
-        const cache = this.cache.get(
-          {
-            scope: getTokenOptions.scope,
-            audience: getTokenOptions.audience || 'default',
-            client_id: this.options.client_id
-          },
-          60 // get a new token if within 60 seconds of expiring
-        );
-
-        if (cache && cache.access_token) {
-          await lock.releaseLock(GET_TOKEN_SILENTLY_LOCK_KEY);
-          return cache.access_token;
+        let accessToken = getAccessTokenFromCache();
+        if (accessToken) {
+          return accessToken;
         }
       }
 


Test patch:
diff --git a/__tests__/Auth0Client.test.ts b/__tests__/Auth0Client.test.ts
index c5bd7cd..4bc0830 100644
--- a/__tests__/Auth0Client.test.ts
+++ b/__tests__/Auth0Client.test.ts
@@ -8,14 +8,14 @@ import * as utils from '../src/utils';
 import { Auth0ClientOptions, IdToken } from '../src';
 import * as scope from '../src/scope';
 import { expectToHaveBeenCalledWithAuth0ClientParam } from './helpers';
+// @ts-ignore
+import { acquireLockSpy } from 'browser-tabs-lock';
 
 jest.mock('unfetch');
 jest.mock('es-cookie');
 jest.mock('../src/jwt');
 jest.mock('../src/token.worker');
 
-jest.unmock('browser-tabs-lock');
-
 const mockWindow = <any>global;
 const mockFetch = (mockWindow.fetch = <jest.Mock>unfetch);
 const mockVerify = <jest.Mock>verify;
@@ -684,6 +684,31 @@ describe('Auth0Client', () => {
     expect(utils.runIframe).not.toHaveBeenCalled();
   });
 
+  it('should not acquire a browser lock when cache is populated', async () => {
+    const auth0 = setup();
+    await login(auth0);
+    jest.spyOn(<any>utils, 'runIframe').mockResolvedValue({
+      access_token: 'my_access_token',
+      state: 'MTIz'
+    });
+    mockFetch.mockResolvedValue(
+      fetchResponse(true, {
+        id_token: 'my_id_token',
+        refresh_token: 'my_refresh_token',
+        access_token: 'my_access_token',
+        expires_in: 86400
+      })
+    );
+    let access_token = await auth0.getTokenSilently({ audience: 'foo' });
+    expect(access_token).toEqual('my_access_token');
+    expect(acquireLockSpy).toHaveBeenCalled();
+    acquireLockSpy.mockClear();
+    // This request will hit the cache, so should not acquire the lock
+    access_token = await auth0.getTokenSilently({ audience: 'foo' });
+    expect(access_token).toEqual('my_access_token');
+    expect(acquireLockSpy).not.toHaveBeenCalled();
+  });
+
   it('sends custom options through to the token endpoint when using an iframe', async () => {
     const auth0 = setup({
       custom_param: 'foo',
diff --git a/__tests__/index.test.ts b/__tests__/index.test.ts
index 9124709..dcf8db8 100644
--- a/__tests__/index.test.ts
+++ b/__tests__/index.test.ts
@@ -1,13 +1,14 @@
-jest.mock('browser-tabs-lock');
+import { expectToHaveBeenCalledWithAuth0ClientParam } from './helpers';
+import { CacheLocation, Auth0ClientOptions } from '../src/global';
+import * as scope from '../src/scope';
+// @ts-ignore
+import { acquireLockSpy, releaseLockSpy } from 'browser-tabs-lock';
+
 jest.mock('../src/jwt');
 jest.mock('../src/storage');
 jest.mock('../src/transaction-manager');
 jest.mock('../src/utils');
 
-import { expectToHaveBeenCalledWithAuth0ClientParam } from './helpers';
-import { CacheLocation, Auth0ClientOptions } from '../src/global';
-import * as scope from '../src/scope';
-
 import createAuth0Client, {
   Auth0Client,
   GetTokenSilentlyOptions
@@ -64,12 +65,6 @@ const webWorkerMatcher = expect.objectContaining({
 });
 
 const setup = async (clientOptions: Partial<Auth0ClientOptions> = {}) => {
-  const auth0 = await createAuth0Client({
-    domain: TEST_DOMAIN,
-    client_id: TEST_CLIENT_ID,
-    ...clientOptions
-  });
-
   const getDefaultInstance = m => require(m).default.mock.instances[0];
 
   const storage = {
@@ -78,11 +73,9 @@ const setup = async (clientOptions: Partial<Auth0ClientOptions> = {}) => {
     remove: require('../src/storage').remove
   };
 
-  const lock = require('browser-tabs-lock');
   const cache = mockEnclosedCache;
 
   const tokenVerifier = require('../src/jwt').verify;
-  const transactionManager = getDefaultInstance('../src/transaction-manager');
   const utils = require('../src/utils');
 
   utils.createQueryParams.mockReturnValue(TEST_QUERY_PARAMS);
@@ -126,6 +119,13 @@ const setup = async (clientOptions: Partial<Auth0ClientOptions> = {}) => {
     close: jest.fn()
   };
 
+  const auth0 = await createAuth0Client({
+    domain: TEST_DOMAIN,
+    client_id: TEST_CLIENT_ID,
+    ...clientOptions
+  });
+  const transactionManager = getDefaultInstance('../src/transaction-manager');
+
   return {
     auth0,
     storage,
@@ -133,7 +133,6 @@ const setup = async (clientOptions: Partial<Auth0ClientOptions> = {}) => {
     tokenVerifier,
     transactionManager,
     utils,
-    lock,
     popup
   };
 };
@@ -155,7 +154,7 @@ describe('Auth0', () => {
   });
 
   afterEach(() => {
-    jest.resetAllMocks();
+    jest.clearAllMocks();
     getUniqueScopesSpy.mockRestore();
   });
 
@@ -216,6 +215,7 @@ describe('Auth0', () => {
         'access_denied'
       ];
       for (let error of recoverableErrors) {
+        utils.runIframe.mockClear();
         utils.runIframe.mockRejectedValue({ error });
         const auth0 = await createAuth0Client({
           domain: TEST_DOMAIN,
@@ -223,7 +223,6 @@ describe('Auth0', () => {
         });
         expect(auth0).toBeInstanceOf(Auth0Client);
         expect(utils.runIframe).toHaveBeenCalledTimes(1);
-        utils.runIframe.mockClear();
       }
     });
 
@@ -1527,7 +1526,8 @@ describe('Auth0', () => {
         });
 
         it('continues method execution when there is no cache available', async () => {
-          const { auth0, utils } = await setup();
+          const { auth0, utils, cache } = await setup();
+          cache.get.mockReturnValue(undefined);
 
           await auth0.getTokenSilently();
 
@@ -1718,18 +1718,20 @@ describe('Auth0', () => {
       };
 
       it('releases the lock when there is an error', async () => {
-        const { auth0, lock, utils } = await setup();
+        const { auth0, utils } = await setup();
 
-        utils.runIframe.mockReturnValue(Promise.reject(new Error('Failed')));
+        utils.runIframe.mockImplementation(() =>
+          Promise.reject(new Error('Failed'))
+        );
 
         await expect(auth0.getTokenSilently()).rejects.toThrowError('Failed');
 
-        expect(lock.acquireLockMock).toHaveBeenCalledWith(
+        expect(acquireLockSpy).toHaveBeenCalledWith(
           GET_TOKEN_SILENTLY_LOCK_KEY,
           5000
         );
 
-        expect(lock.releaseLockMock).toHaveBeenCalledWith(
+        expect(releaseLockSpy).toHaveBeenCalledWith(
           GET_TOKEN_SILENTLY_LOCK_KEY
         );
       });
@@ -1892,7 +1894,7 @@ describe('Auth0', () => {
       });
 
       it('throws error if state from popup response is different from the provided state', async () => {
-        const { auth0, utils, lock } = await setup();
+        const { auth0, utils } = await setup();
 
         utils.runIframe.mockReturnValue(
           Promise.resolve({
@@ -1904,7 +1906,7 @@ describe('Auth0', () => {
           auth0.getTokenSilently(defaultOptionsIgnoreCacheTrue)
         ).rejects.toThrowError('Invalid state');
 
-        expect(lock.releaseLockMock).toHaveBeenCalledWith(
+        expect(releaseLockSpy).toHaveBeenCalledWith(
           GET_TOKEN_SILENTLY_LOCK_KEY
         );
       });
@@ -1969,15 +1971,15 @@ describe('Auth0', () => {
         );
       });
       it('acquires and releases lock', async () => {
-        const { auth0, lock } = await setup();
+        const { auth0 } = await setup();
 
         await auth0.getTokenSilently(defaultOptionsIgnoreCacheTrue);
 
-        expect(lock.acquireLockMock).toHaveBeenCalledWith(
+        expect(acquireLockSpy).toHaveBeenCalledWith(
           GET_TOKEN_SILENTLY_LOCK_KEY,
           5000
         );
-        expect(lock.releaseLockMock).toHaveBeenCalledWith(
+        expect(releaseLockSpy).toHaveBeenCalledWith(
           GET_TOKEN_SILENTLY_LOCK_KEY
         );
       });
@@ -2179,17 +2181,18 @@ describe('Auth0', () => {
 });
 
 describe('default creation function', () => {
-  it('does nothing if there is nothing storage', async () => {
-    Auth0Client.prototype.getTokenSilently = jest.fn();
+  it('does nothing if there is nothing in storage', async () => {
+    jest.spyOn(Auth0Client.prototype, 'getTokenSilently');
+    const getSpy = jest
+      .spyOn(require('../src/storage'), 'get')
+      .mockReturnValueOnce(false);
 
     const auth0 = await createAuth0Client({
       domain: TEST_DOMAIN,
       client_id: TEST_CLIENT_ID
     });
 
-    expect(require('../src/storage').get).toHaveBeenCalledWith(
-      'auth0.is.authenticated'
-    );
+    expect(getSpy).toHaveBeenCalledWith('auth0.is.authenticated');
 
     expect(auth0.getTokenSilently).not.toHaveBeenCalled();
   });