-
Notifications
You must be signed in to change notification settings - Fork 747
fix(amazonq): fix enterprise users not able to sign in correctly if they have 2+ vscode instances open #7151
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
Changes from 23 commits
21af372
1daab83
7380422
84609ec
ed782e2
3e50145
0392707
95d5c9e
b7144f0
3fec764
57e81a1
d388b8f
e50bfab
69a474e
33dd9bf
d52d659
4c8a6b5
a3514ff
9eb0954
42c3451
b06932b
9b57d7b
25b04d8
f6f1772
cdb9bc8
97a4482
8466c95
b7d5b73
6a0c54f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| { | ||
| "type": "Bug Fix", | ||
| "description": "Fix users can not log in successfully with 2+ IDE instnaces open due to throttle error throw by the service" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,177 @@ | ||
| /*! | ||
| * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import globals from '../extensionGlobals' | ||
| import { globalKey } from '../globalState' | ||
| import { getLogger } from '../logger/logger' | ||
| import { waitUntil } from '../utilities/timeoutUtils' | ||
|
|
||
| /** | ||
| * args: | ||
| * @member result: the actual resource type callers want to use | ||
| * @member locked: readWriteLock, while the lock is acquired by one process, the other can't access to it until it's released by the previous | ||
| * @member timestamp: used for determining the resource is stale or not | ||
| */ | ||
| interface Resource<V> { | ||
| result: V | undefined | ||
| locked: boolean | ||
| timestamp: number | ||
| } | ||
|
|
||
| /** | ||
| * GlobalStates schema, which is used for vscode global states deserialization, [globals.globalState#tryGet<T>] etc. | ||
| * The purpose of it is to allow devs to overload the resource into existing global key and no need to create a specific key for only this purpose. | ||
| */ | ||
| export interface GlobalStateSchema<V> { | ||
| resource: Resource<V> | ||
justinmk3 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| const logger = getLogger() | ||
Will-ShaoHua marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| function now() { | ||
| return globals.clock.Date.now() | ||
| } | ||
|
|
||
| /** | ||
| * constructor: | ||
| * @param key: global state key, which is used for globals.globalState#update, #tryGet etc. | ||
| * @param expirationInMilli: cache expiration time in milli seconds | ||
| * @param defaultValue: default value for the cache if the cache doesn't pre-exist in users' FS | ||
| * @param waitUntilOption: waitUntil option for acquire lock | ||
| * | ||
| * methods: | ||
| * @method resourceProvider: implementation needs to implement this method to obtain the latest resource either via network calls or FS read | ||
| * @method getResource: obtain the resource from cache or pull the latest from the service if the cache either expires or doesn't exist | ||
| */ | ||
| export abstract class CachedResource<V> { | ||
Will-ShaoHua marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| constructor( | ||
| private readonly key: globalKey, | ||
| private readonly expirationInMilli: number, | ||
| private readonly defaultValue: GlobalStateSchema<V>, | ||
| private readonly waitUntilOption: { timeout: number; interval: number; truthy: boolean } | ||
| ) {} | ||
|
|
||
| abstract resourceProvider(): Promise<V> | ||
|
|
||
| async getResource(): Promise<V> { | ||
| const cachedValue = await this.tryLoadResourceAndLock() | ||
| const resource = cachedValue?.resource | ||
|
|
||
| // If cache is still fresh, return cached result, otherwise pull latest from the service | ||
| if (cachedValue && resource && resource.result) { | ||
| const duration = now() - resource.timestamp | ||
| if (duration < this.expirationInMilli) { | ||
| logger.info(`cache hit, duration(%sms) is less than expiration(%sms)`, duration, this.expirationInMilli) | ||
| // release the lock | ||
| await this.releaseLock(resource, cachedValue) | ||
| return resource.result | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, I think I get it: multiple vscode instances will all acquire the lock, but the N+1 instances will each I guess that's fine. But it means they all must acquire a lock just to do the read. Maybe that's necessary, to avoid reading during an update.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yea exactly, i should have mentioned it more explicitly in the doc string, it's like a read write lock? if i understand it correctly. But not multiple vscode instances will all acquire the lock, only 1 can hold the lock and the rest of them need to wait until the first one finish pulling the real response and release the lock. Then the rest of them can start acquiring the lock 1 by 1 and return early here using the cached value.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there're definitely room to improve i think in the future. Not have enough time to consider all the potential use cases and requirements so currently it only meets the needs for listProfile / listCustomization. |
||
| } else { | ||
| logger.info( | ||
| `cached value is stale, duration(%sms) is older than expiration(%sms), invoking service API to pull the latest response`, | ||
| duration, | ||
| this.expirationInMilli | ||
| ) | ||
| } | ||
| } | ||
Will-ShaoHua marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| /** | ||
| * Possible paths here | ||
| * 1. cache doesn't exist. | ||
| * 2. cache exists but expired. | ||
| * 3. lock is held by other process and the waiting time is greater than the specified waiting time | ||
| */ | ||
| logger.info(`cache miss, invoking service API to pull the latest response`) | ||
| try { | ||
| // Make the real network call / FS read to pull the resource | ||
| const latest = await this.resourceProvider() | ||
|
|
||
| // Update resource cache and release the lock | ||
| const r: Resource<V> = { | ||
| locked: false, | ||
| timestamp: now(), | ||
| result: latest, | ||
| } | ||
| logger.info(`doen loading the latest of resource(%s), updating resource cache`, this.key) | ||
Will-ShaoHua marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| await this.releaseLock(r) | ||
| return latest | ||
| } catch (e) { | ||
| logger.info( | ||
| `encountered unexpected error while loading the latest of resource(%s), releasing resource lock`, | ||
| this.key | ||
| ) | ||
| await this.releaseLock() | ||
| throw e | ||
| } | ||
| } | ||
|
|
||
| // This method will lock the resource so other callers have to wait until the lock is released, otherwise will return undefined if it times out | ||
| private async tryLoadResourceAndLock(): Promise<GlobalStateSchema<V> | undefined> { | ||
| const _acquireLock = async () => { | ||
| const cachedValue = this.readCacheOrDefault() | ||
|
|
||
| if (!cachedValue.resource.locked) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't this (also) check |
||
| await this.lockResource(cachedValue) | ||
| return cachedValue | ||
| } | ||
|
|
||
| return undefined | ||
| } | ||
|
|
||
| const lock = await waitUntil(async () => { | ||
| const lock = await _acquireLock() | ||
| logger.info(`try obtain resource cache read lock %s`, lock) | ||
Will-ShaoHua marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| if (lock) { | ||
| return lock | ||
| } | ||
| }, this.waitUntilOption) | ||
|
|
||
| return lock | ||
| } | ||
|
|
||
| async lockResource(baseCache: GlobalStateSchema<V>): Promise<void> { | ||
| await this.updateResourceCache({ locked: true }, baseCache) | ||
| } | ||
|
|
||
| async releaseLock(): Promise<void> | ||
| async releaseLock(resource: Partial<Resource<V>>): Promise<void> | ||
| async releaseLock(resource: Partial<Resource<V>>, baseCache: GlobalStateSchema<V>): Promise<void> | ||
| async releaseLock(resource?: Partial<Resource<V>>, baseCache?: GlobalStateSchema<V>): Promise<void> { | ||
| if (!resource) { | ||
| await this.updateResourceCache({ locked: false }, undefined) | ||
| } else if (baseCache) { | ||
| await this.updateResourceCache(resource, baseCache) | ||
| } else { | ||
| await this.updateResourceCache(resource, undefined) | ||
| } | ||
| } | ||
|
|
||
| private async updateResourceCache(resource: Partial<Resource<any>>, cache: GlobalStateSchema<any> | undefined) { | ||
| const baseCache = cache ?? this.readCacheOrDefault() | ||
|
|
||
| const toUpdate: GlobalStateSchema<V> = { | ||
| ...baseCache, | ||
| resource: { | ||
| ...baseCache.resource, | ||
| ...resource, | ||
| }, | ||
| } | ||
|
|
||
| await globals.globalState.update(this.key, toUpdate) | ||
| } | ||
|
|
||
| private readCacheOrDefault(): GlobalStateSchema<V> { | ||
| const cachedValue = globals.globalState.tryGet<GlobalStateSchema<V>>(this.key, Object, { | ||
| ...this.defaultValue, | ||
| resource: { | ||
| ...this.defaultValue.resource, | ||
| locked: false, | ||
| result: undefined, | ||
| timestamp: 0, | ||
| }, | ||
| }) | ||
|
|
||
| return cachedValue | ||
| } | ||
| } | ||
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.
should
invalidateProfiledo this internally?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.
this is kinda redundant i think, and also its name is confusing for sure (should've named it
releaseLock), but i will remove it.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.
invalidateProfiledoesn't need this i think as the source of truth of profileinvalidateProfileis also from this resourceCache. Originally i added this line is kinda for debugging purpose and to avoid a dead lock scenario when the code was problematic. However i think releaseLock will be calledgetResourcegoes to pull real response and executes successfullygetResourcegoes to pull real response and executes exceptionallygetResourcegoes to return cached valueso I think it's redundant
Uh oh!
There was an error while loading. Please reload this page.
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.
just reminds me that i need to add one
clearCacheonConnectionChanged. Cache expiration is only 60s atm tho, worth adding one in case ppl change connection and use the wrong resultset.