Avoid the possibility to do simultaneous calls to the token endpoint #664
Conversation
| const canProceed = await acquireLockSpy(...args); | ||
| if (canProceed) { | ||
| return super.acquireLock(...args); | ||
| } |
There was a problem hiding this comment.
Changing this enables us to control the locking success/failure from inside the tests.
| @@ -663,6 +664,68 @@ describe('Auth0Client', () => { | |||
| ); | |||
| }); | |||
|
|
|||
| describe('concurrency', () => { | |||
| it('should call _getTokenSilently multiple times when no call in flight concurrently', async () => { | |||
There was a problem hiding this comment.
In order to test the promise concurrency fix, we can not rely on the actual HTTP call as this will be avoided by the locking library. So instead of testing that, we are verifying that _getTokenSilently is or isn't called as expected.
src/Auth0Client.ts
Outdated
| ); | ||
| } | ||
|
|
||
| private async _getTokenSilently({ ignoreCache, getTokenOptions }) { |
There was a problem hiding this comment.
Moved the original code to get a token to its own private method in order to be able to easily wrap it.
| getTokenOptions | ||
| }), | ||
| `${this.options.client_id}::${getTokenOptions.audience}::${getTokenOptions.scope}` | ||
| ); |
There was a problem hiding this comment.
singlePromise is a function that is ensuring we only have a single concurrent promise based on the provided key.
| await retryPromise( | ||
| () => lock.acquireLock(GET_TOKEN_SILENTLY_LOCK_KEY, 5000), | ||
| 10 | ||
| ) |
There was a problem hiding this comment.
Instead of using a while loop, retryPromise will control the retrying for us.
frederikprijck
force-pushed
the
frederik/ferrt
branch
from
02592d6 to
4055ee0
Compare
| expect(client1['_getTokenSilently']).toHaveBeenCalledTimes(1); | ||
| expect(client2['_getTokenSilently']).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
I think this is true, but just to clarify - is client1 guaranteed to claim the lock first or could this potentially be a flakey test?
There was a problem hiding this comment.
Promise.all is strictly ordered, so I would say this is fine
There was a problem hiding this comment.
That is actually a good question. This should be flakey as the lock is acquired async https://github.com/supertokens/browser-tabs-lock/blob/master/index.ts#L93 🤔
There was a problem hiding this comment.
@adamjmcgrath Also when considering the delay linked above?
There was a problem hiding this comment.
The delay wont make any difference https://repl.it/@AdamMcGrath1/jest-playground#concurrency.test.js
This PR ensures the SPA is less likely to do concurrent API calls to the token endpoint.
clientId,audienceandscope.This solves #553 and #657