-
Notifications
You must be signed in to change notification settings - Fork 748
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
Merged
+232
−3
Merged
Changes from 6 commits
Commits
Show all changes
29 commits
Select commit
Hold shift + click to select a range
21af372
wip cache resource implementation
Will-ShaoHua 1daab83
nit
Will-ShaoHua 7380422
refacotr resourceCache.ts not verified yet
Will-ShaoHua 84609ec
verified
Will-ShaoHua ed782e2
patch not verified
Will-ShaoHua 3e50145
merge 2 global states variable but might need to revert because it wi…
Will-ShaoHua 0392707
Revert "merge 2 global states variable but might need to revert becau…
Will-ShaoHua 95d5c9e
cl
Will-ShaoHua b7144f0
private
Will-ShaoHua 3fec764
compile error
Will-ShaoHua 57e81a1
some docstr
Will-ShaoHua d388b8f
try catch
Will-ShaoHua e50bfab
pass waitUntil option via ctor
Will-ShaoHua 69a474e
docstr
Will-ShaoHua 33dd9bf
docstr
Will-ShaoHua d52d659
docstr
Will-ShaoHua 4c8a6b5
docstr
Will-ShaoHua a3514ff
rename
Will-ShaoHua 9eb0954
log
Will-ShaoHua 42c3451
docstr
Will-ShaoHua b06932b
rename updateResourceCache
Will-ShaoHua 9b57d7b
docstr
Will-ShaoHua 25b04d8
refactor and overload releaseLock instead of using updateResourceCache
Will-ShaoHua f6f1772
update log level / topic
Will-ShaoHua cdb9bc8
* update log string
Will-ShaoHua 97a4482
add clearCache
Will-ShaoHua 8466c95
docstr
Will-ShaoHua b7d5b73
newline
Will-ShaoHua 6a0c54f
update log level
Will-ShaoHua File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,127 @@ | ||
| /*! | ||
| * 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' | ||
|
|
||
| interface Resource<V> { | ||
| result: V | undefined | ||
| locked: boolean | ||
| timestamp: number | ||
| } | ||
|
|
||
| // GlobalStates schema, which is used for vscode global states deserialization | ||
| // [globals.globalState.tryGet<T>] | ||
| 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() | ||
| } | ||
|
|
||
| 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> | ||
| ) {} | ||
|
|
||
| abstract resourceProvider(): Promise<V> | ||
|
|
||
| async getResource(): Promise<V> { | ||
| const cachedValue = await this.readResourceAndLock() | ||
| const resource = cachedValue?.resource | ||
|
|
||
| // if cache is still fresh, return | ||
| if (cachedValue && resource && resource.result) { | ||
| if (now() - resource.timestamp < this.expirationInMilli) { | ||
| logger.info(`cache hit`) | ||
| // release the lock | ||
| await this.updateCache(cachedValue, { | ||
| ...resource, | ||
| locked: false, | ||
| }) | ||
| return resource.result | ||
| } else { | ||
| logger.info(`cache hit but cached value is stale, invoking service API to pull the latest response`) | ||
| } | ||
| } | ||
Will-ShaoHua marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| // catch and error case? | ||
Will-ShaoHua marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| logger.info(`cache miss, invoking service API to pull the latest response`) | ||
| const latest = await this.resourceProvider() | ||
|
|
||
| // update resource cache and release the lock | ||
| const r: Resource<V> = { | ||
| locked: false, | ||
| timestamp: now(), | ||
| result: latest, | ||
| } | ||
| await this.updateCache(cachedValue, r) | ||
| return latest | ||
| } | ||
|
|
||
| async readResourceAndLock(): 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.updateCache(cachedValue, { | ||
| ...cachedValue.resource, | ||
| locked: true, | ||
| }) | ||
| return cachedValue | ||
| } | ||
|
|
||
| return undefined | ||
| } | ||
|
|
||
| const lock = await waitUntil( | ||
| async () => { | ||
| const lock = await _acquireLock() | ||
| logger.info(`try obtain resource cache read lock %s`, lock) | ||
| if (lock) { | ||
| return lock | ||
| } | ||
| }, | ||
| { timeout: 15000, interval: 1500, truthy: true } // TODO: pass via ctor | ||
Will-ShaoHua marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| ) | ||
|
|
||
| return lock | ||
| } | ||
|
|
||
| async releaseLock() { | ||
| await globals.globalState.update(this.key, { | ||
| ...this.readCacheOrDefault(), | ||
| locked: false, | ||
| }) | ||
| } | ||
|
|
||
| private async updateCache(cache: GlobalStateSchema<any> | undefined, resource: Resource<any>) { | ||
| await globals.globalState.update(this.key, { | ||
| ...(cache ? cache : this.readCacheOrDefault()), | ||
| resource: resource, | ||
| }) | ||
| } | ||
|
|
||
| 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 | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.