-
Notifications
You must be signed in to change notification settings - Fork 495
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
fix(auth): token refresh rapidly called unexpectedly #6479
base: master
Are you sure you want to change the base?
Conversation
Problem: getChatAuthState() is called in many places by the Q features simultaneously, this eventually triggers multiple calls to getToken() and if needed refreshToken(). The resulted in refreshToken being spammed and the Identity team seeing spikes in token refreshes from clients. Solution: Throttle getChatAuthState(). Throttling w/ leading: true, allows us to instantly return a fresh result OR a cached result in the case we are throttled. Debounce on the other hand would cause callers to hang since they have to wait for debounce to timeout. Also, we put a debounce on getToken() before but this did not work since a new SsoAccessToken instance is created each time the offending code flow triggered (we could look to cache the instance instead which would enable the getToken() debounce to be useful. Signed-off-by: nkomonen-amazon <[email protected]>
Since this is not required to fix the original issue we will revert this previous change to reduce potential confusion. I will create a SIM ticket to describe a better overall fix though Signed-off-by: nkomonen-amazon <[email protected]>
|
@@ -44,6 +44,7 @@ import { telemetry } from '../../shared/telemetry/telemetry' | |||
import { asStringifiedStack } from '../../shared/telemetry/spans' | |||
import { withTelemetryContext } from '../../shared/telemetry/util' | |||
import { focusAmazonQPanel } from '../../codewhispererChat/commands/registerCommands' | |||
import { throttle } from 'lodash' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we have a backlog item to remove dependency on lodash
for performance reasons. The work was started here: #5157
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some discussions w/ our move to the LSP work we will probably be able to drop this code, getting rid of the dependency. Also there is currently no existing throttle implementation. We will either need to build our own or add a new dependency. Ticket for removing lodash is here: https://taskei.amazon.dev/tasks/IDE-10293
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW it looks like throttle just uses debounce under the hood: https://github.com/lodash/lodash/blob/npm/throttle.js
Problem:
getChatAuthState() is called in many places by the Q features simultaneously,
this eventually triggers multiple calls to getToken() and if needed refreshToken().
This resulted in refreshToken being spammed and the Identity team seeing spikes in token refreshes
from clients.
Solution:
Throttle getChatAuthState().
Throttling w/ leading: true, allows us to instantly return
a fresh result OR a cached result in the case we are throttled. Debounce on the
other hand would cause callers to hang since they have to wait for debounce to timeout.
Also, we put a debounce on getToken() before in #6282 but this did not work since a new
SsoAccessToken instance is created each time the offending code flow triggered (we could
look to cache the instance instead which would enable the getToken() debounce to be useful.
Testing
To test the difference after adding the throttle:
getToken()
What would happen is that without throttle it would trigger getChatAuthState() many times, likely due to the connection
becoming invalid and sending an event to all Q features, causing each of them to call getChatAuthState() at the same time.
But when the throttle was added, the amount of these calls dropped to at most 2.
Signed-off-by: nkomonen-amazon [email protected]
feature/x
branches will not be squash-merged at release time.