-
Notifications
You must be signed in to change notification settings - Fork 76
azure: Add in-memory cache for Azure location/provider API calls #2239
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 1 commit
daadb51
7078b12
0f31e88
2d8f734
5f9ba49
7aaf770
a445188
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,82 @@ | ||||||
| /*--------------------------------------------------------------------------------------------- | ||||||
| * Copyright (c) Microsoft Corporation. All rights reserved. | ||||||
| * Licensed under the MIT License. See License.txt in the project root for license information. | ||||||
| *--------------------------------------------------------------------------------------------*/ | ||||||
|
|
||||||
| /** | ||||||
| * A simple cache for location data that deduplicates in-flight requests. | ||||||
| * | ||||||
| * Currently backed by an in-memory Map. Designed so the backing store can be | ||||||
| * swapped to persistent storage (e.g. `vscode.Memento` / globalState) in the | ||||||
| * future to survive across VS Code restarts with a longer TTL (e.g. 7 days). | ||||||
| */ | ||||||
|
|
||||||
| interface CacheEntry<T> { | ||||||
| data: T; | ||||||
| /** Unix timestamp (ms) when this entry was stored */ | ||||||
| storedAt: number; | ||||||
| } | ||||||
|
|
||||||
| export class LocationCache { | ||||||
| private readonly cache = new Map<string, CacheEntry<unknown>>(); | ||||||
|
alexweininger marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| /** | ||||||
| * In-flight promises keyed the same way as the cache. | ||||||
| * Ensures concurrent callers share the same request instead of firing duplicates. | ||||||
| */ | ||||||
| private readonly inflight = new Map<string, Promise<unknown>>(); | ||||||
|
|
||||||
| /** | ||||||
| * @param ttlMs Optional time-to-live in milliseconds. When omitted, entries | ||||||
| * never expire (suitable for in-memory caches that reset on extension | ||||||
| * deactivation). Set this when switching to persistent storage. | ||||||
| */ | ||||||
| constructor(private readonly ttlMs?: number) { } | ||||||
|
|
||||||
| /** | ||||||
| * Get a value from the cache, or fetch it if missing/expired. | ||||||
| * Concurrent calls with the same key share a single in-flight request. | ||||||
| */ | ||||||
| async getOrLoad<T>(key: string, loader: () => Promise<T>): Promise<T> { | ||||||
| const cached = this.cache.get(key) as CacheEntry<T> | undefined; | ||||||
| if (cached && !this.isExpired(cached)) { | ||||||
| return cached.data; | ||||||
| } | ||||||
|
|
||||||
| // Check for an in-flight request we can piggy-back on | ||||||
| const existing = this.inflight.get(key) as Promise<T> | undefined; | ||||||
| if (existing) { | ||||||
| return existing; | ||||||
| } | ||||||
|
|
||||||
| const promise = loader().then(data => { | ||||||
|
||||||
| const promise = loader().then(data => { | |
| const promise = Promise.resolve().then(loader).then(data => { |
Copilot
AI
Mar 25, 2026
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.
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).
Copilot
AI
Mar 25, 2026
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.
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).
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,154 @@ | ||
| /*--------------------------------------------------------------------------------------------- | ||
| * Copyright (c) Microsoft Corporation. All rights reserved. | ||
| * Licensed under the MIT License. See License.txt in the project root for license information. | ||
| *--------------------------------------------------------------------------------------------*/ | ||
|
|
||
| import * as assert from 'assert'; | ||
| import { LocationCache } from '../src/wizard/LocationCache'; | ||
|
|
||
| suite('LocationCache', () => { | ||
| let cache: LocationCache; | ||
|
|
||
| setup(() => { | ||
| cache = new LocationCache(); | ||
| }); | ||
|
|
||
| test('returns data from loader on cache miss', async () => { | ||
| const result = await cache.getOrLoad('key1', async () => ['eastus', 'westus']); | ||
| assert.deepStrictEqual(result, ['eastus', 'westus']); | ||
| }); | ||
|
|
||
| test('returns cached data on subsequent calls without calling loader again', async () => { | ||
| let callCount = 0; | ||
| const loader = async () => { | ||
| callCount++; | ||
| return ['eastus']; | ||
| }; | ||
|
|
||
| const result1 = await cache.getOrLoad('key1', loader); | ||
| const result2 = await cache.getOrLoad('key1', loader); | ||
|
|
||
| assert.deepStrictEqual(result1, ['eastus']); | ||
| assert.deepStrictEqual(result2, ['eastus']); | ||
| assert.strictEqual(callCount, 1, 'loader should only be called once'); | ||
| }); | ||
|
|
||
| test('uses separate entries for different keys', async () => { | ||
| await cache.getOrLoad('sub1|false', async () => ['eastus']); | ||
| await cache.getOrLoad('sub2|false', async () => ['westus']); | ||
|
|
||
| const result1 = await cache.getOrLoad('sub1|false', async () => { throw new Error('should not call'); }); | ||
| const result2 = await cache.getOrLoad('sub2|false', async () => { throw new Error('should not call'); }); | ||
|
|
||
| assert.deepStrictEqual(result1, ['eastus']); | ||
| assert.deepStrictEqual(result2, ['westus']); | ||
| }); | ||
|
|
||
| test('deduplicates concurrent in-flight requests for the same key', async () => { | ||
| let callCount = 0; | ||
| let resolve: (value: string[]) => void; | ||
| const loader = () => { | ||
| callCount++; | ||
| return new Promise<string[]>(r => { resolve = r; }); | ||
| }; | ||
|
|
||
| const p1 = cache.getOrLoad('key1', loader); | ||
| const p2 = cache.getOrLoad('key1', loader); | ||
|
|
||
| // Both should be waiting on the same promise | ||
| assert.strictEqual(callCount, 1, 'loader should only be called once for concurrent requests'); | ||
|
|
||
| resolve!(['eastus']); | ||
| const [result1, result2] = await Promise.all([p1, p2]); | ||
|
|
||
| assert.deepStrictEqual(result1, ['eastus']); | ||
| assert.deepStrictEqual(result2, ['eastus']); | ||
| }); | ||
|
|
||
| test('clear removes all cached entries', async () => { | ||
| let callCount = 0; | ||
| const loader = async () => { | ||
| callCount++; | ||
| return ['eastus']; | ||
| }; | ||
|
|
||
| await cache.getOrLoad('key1', loader); | ||
| assert.strictEqual(callCount, 1); | ||
|
|
||
| cache.clear(); | ||
|
|
||
| await cache.getOrLoad('key1', loader); | ||
| assert.strictEqual(callCount, 2, 'loader should be called again after clear'); | ||
| }); | ||
|
|
||
| test('expired entries are refreshed', async () => { | ||
| // Use a very short TTL | ||
| const shortCache = new LocationCache(1); | ||
| let callCount = 0; | ||
| const loader = async () => { | ||
| callCount++; | ||
| return [`result-${callCount}`]; | ||
| }; | ||
|
|
||
| const result1 = await shortCache.getOrLoad('key1', loader); | ||
| assert.deepStrictEqual(result1, ['result-1']); | ||
|
|
||
| // Wait for expiry | ||
| await new Promise(r => setTimeout(r, 10)); | ||
|
||
|
|
||
| const result2 = await shortCache.getOrLoad('key1', loader); | ||
| assert.deepStrictEqual(result2, ['result-2']); | ||
| assert.strictEqual(callCount, 2, 'loader should be called again after expiry'); | ||
| }); | ||
|
|
||
| test('entries without TTL never expire', async () => { | ||
| // Default constructor has no TTL | ||
| let callCount = 0; | ||
|
|
||
| await cache.getOrLoad('key1', async () => { callCount++; return ['eastus']; }); | ||
| await cache.getOrLoad('key1', async () => { callCount++; return ['westus']; }); | ||
|
|
||
| assert.strictEqual(callCount, 1); | ||
| }); | ||
|
|
||
| test('loader error does not poison the cache', async () => { | ||
| let shouldFail = true; | ||
| const loader = async () => { | ||
| if (shouldFail) { | ||
| throw new Error('network error'); | ||
| } | ||
| return ['eastus']; | ||
| }; | ||
|
|
||
| await assert.rejects(() => cache.getOrLoad('key1', loader), /network error/); | ||
|
|
||
| shouldFail = false; | ||
| const result = await cache.getOrLoad('key1', loader); | ||
| assert.deepStrictEqual(result, ['eastus']); | ||
| }); | ||
|
|
||
| test('loader error is propagated to all concurrent waiters', async () => { | ||
| let reject: (err: Error) => void; | ||
| const loader = () => new Promise<string[]>((_, r) => { reject = r; }); | ||
|
|
||
| const p1 = cache.getOrLoad('key1', loader); | ||
| const p2 = cache.getOrLoad('key1', loader); | ||
|
|
||
| reject!(new Error('boom')); | ||
|
|
||
| await assert.rejects(() => p1, /boom/); | ||
| await assert.rejects(() => p2, /boom/); | ||
| }); | ||
|
|
||
| test('after error, a new loader call succeeds', async () => { | ||
| let callCount = 0; | ||
| const failLoader = async () => { callCount++; throw new Error('fail'); }; | ||
| const okLoader = async () => { callCount++; return ['eastus']; }; | ||
|
|
||
| await assert.rejects(() => cache.getOrLoad('key1', failLoader), /fail/); | ||
| const result = await cache.getOrLoad('key1', okLoader); | ||
|
|
||
| assert.deepStrictEqual(result, ['eastus']); | ||
| assert.strictEqual(callCount, 2); | ||
| }); | ||
| }); | ||
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.
Exporting the singleton caches (
subscriptionLocationsCache,providerLocationsCache) makes them part of the package’s public API (also reflected inindex.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.