azure: Add in-memory cache for Azure location/provider API calls#2239
azure: Add in-memory cache for Azure location/provider API calls#2239alexweininger merged 7 commits intomainfrom
Conversation
LocationCache deduplicates concurrent requests and caches results across wizard instances within a single extension activation. Wired into getAllLocations and getProviderLocations in LocationListStep.
There was a problem hiding this comment.
Pull request overview
Adds an in-memory cache to deduplicate and reuse Azure location/provider API calls across wizard instances within a single extension activation.
Changes:
- Introduces
LocationCachewith in-flight request deduplication and optional TTL. - Wires caching into
getAllLocationsandgetProviderLocations. - Adds unit tests covering cache hits, concurrency dedupe, TTL expiry, and error behavior.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| azure/src/wizard/LocationCache.ts | Implements the shared in-memory cache + singleton caches. |
| azure/src/wizard/LocationListStep.ts | Routes subscription/provider location lookups through the cache. |
| azure/test/LocationCache.test.ts | Adds unit tests for caching, concurrency, TTL, and error handling. |
| azure/src/index.ts | Exports LocationCache and the singleton caches from the package entrypoint. |
| azure/index.d.ts | Publishes type declarations for the new cache API/exports. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const promise = loader().then(data => { | ||
| this.cache.set(key, { data, storedAt: Date.now() }); | ||
| this.inflight.delete(key); | ||
| return data; | ||
| }).catch(err => { | ||
| this.inflight.delete(key); | ||
| throw err; | ||
| }); |
There was a problem hiding this comment.
Calling clear() can be undone by currently in-flight requests: once they resolve, they will repopulate this.cache via this.cache.set(...), potentially reintroducing stale entries right after a sign-out/subscription change. Consider clearing inflight as well, or introduce a cache 'generation' token checked before writing results (ignore writes from older generations).
| /** Remove all entries (e.g. on sign-out or subscription change) */ | ||
| clear(): void { | ||
| this.cache.clear(); | ||
| // Don't clear inflight — let pending requests finish naturally | ||
| } |
There was a problem hiding this comment.
Calling clear() can be undone by currently in-flight requests: once they resolve, they will repopulate this.cache via this.cache.set(...), potentially reintroducing stale entries right after a sign-out/subscription change. Consider clearing inflight as well, or introduce a cache 'generation' token checked before writing results (ignore writes from older generations).
| return existing; | ||
| } | ||
|
|
||
| const promise = loader().then(data => { |
There was a problem hiding this comment.
loader() can throw synchronously before returning a Promise. In that case, inflight is never set, and callers won't get consistent dedup/error behavior. Wrap the call so sync throws become a rejected Promise (e.g., start from Promise.resolve().then(loader)), and ensure inflight bookkeeping is consistent.
| const promise = loader().then(data => { | |
| const promise = Promise.resolve().then(loader).then(data => { |
| // Wait for expiry | ||
| await new Promise(r => setTimeout(r, 10)); |
There was a problem hiding this comment.
This time-based test can be flaky under slow/loaded CI agents. Prefer fake timers (if the repo test stack supports them) or otherwise make the timing deterministic (e.g., inject a clock into LocationCache for tests) to avoid intermittent failures.
| export * from './utils/setupAzureLogger'; | ||
| export * from './utils/uiUtils'; | ||
| export * from './wizard/LocationListStep'; | ||
| export { LocationCache, subscriptionLocationsCache, providerLocationsCache } from './wizard/LocationCache'; |
There was a problem hiding this comment.
Exporting the singleton caches (subscriptionLocationsCache, providerLocationsCache) makes them part of the package’s public API (also reflected in index.d.ts). If these are intended to be internal implementation details, consider not exporting them (or marking as internal/unstable) to avoid external consumers depending on shared global state that may need to change later.
| export { LocationCache, subscriptionLocationsCache, providerLocationsCache } from './wizard/LocationCache'; | |
| export { LocationCache } from './wizard/LocationCache'; |
Promise.resolve().then(loader) deferred loader execution to a microtask, which meant concurrent getOrLoad calls couldn't detect the in-flight request because the loader hadn't run yet. Call loader() directly with a try-catch for sync throws instead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Log cache misses and successful fetches in getAllLocations and getProviderLocations using ext.outputChannel.appendLog. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
LocationCache deduplicates concurrent requests and caches results across wizard instances within a single extension activation. Wired into getAllLocations and getProviderLocations in LocationListStep.