Skip to content

Commit 01404ec

Browse files
committed
fix(@angular/build): prevent concurrent stylesheet bundling esbuild context leaks
When multiple components in the same file define identical inline styles, concurrent calls to the stylesheet bundler invoke Cache.getOrCreate concurrently with the same key. Because getOrCreate is not concurrency-safe, multiple BundlerContext instances are created. Additionally, if multiple bundle() requests occur concurrently on a BundlerContext when the esbuild context is not yet initialized, they both invoke context(), causing one to overwrite and leak the other. Leaked esbuild contexts keep the Node event loop active forever, hanging the test runner. Resolve the Cache race by using an internal Map of in-flight promises to share the same active request, along with a version counter to detect mid-flight writes and safely retry. Resolve the BundlerContext race by memoizing and sharing the active performBundle promise. Fixes #33317
1 parent 11a4438 commit 01404ec

3 files changed

Lines changed: 246 additions & 8 deletions

File tree

packages/angular/build/src/tools/esbuild/bundler-context.ts

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,8 @@ export class BundlerContext {
5959
#esbuildContext?: BuildContext<{ metafile: true; write: false }>;
6060
#esbuildOptions?: BuildOptions & { metafile: true; write: false };
6161
#esbuildResult?: BundleContextResult;
62+
#activeBundlePromise?: Promise<BundleContextResult>;
63+
#disposed = false;
6264
#optionsFactory: BundlerOptionsFactory<BuildOptions & { metafile: true; write: false }>;
6365
#shouldCacheResult: boolean;
6466
#loadCache?: MemoryLoadResultCache;
@@ -177,7 +179,18 @@ export class BundlerContext {
177179
return this.#esbuildResult;
178180
}
179181

180-
const result = await this.#performBundle();
182+
if (!force && this.#activeBundlePromise) {
183+
return this.#activeBundlePromise;
184+
}
185+
186+
const bundlePromise = this.#performBundle().finally(() => {
187+
if (this.#activeBundlePromise === bundlePromise) {
188+
this.#activeBundlePromise = undefined;
189+
}
190+
});
191+
this.#activeBundlePromise = bundlePromise;
192+
193+
const result = await bundlePromise;
181194
if (this.#shouldCacheResult) {
182195
this.#esbuildResult = result;
183196
}
@@ -207,7 +220,12 @@ export class BundlerContext {
207220
} else {
208221
// Create a build context and perform the build.
209222
// Context creation does not perform a build.
210-
this.#esbuildContext = await context(this.#esbuildOptions);
223+
const esbuildContext = await context(this.#esbuildOptions);
224+
if (this.#disposed) {
225+
await esbuildContext.dispose();
226+
throw new Error('BundlerContext was disposed during build.');
227+
}
228+
this.#esbuildContext = esbuildContext;
211229
result = await this.#esbuildContext.rebuild();
212230
}
213231
} catch (failure) {
@@ -446,9 +464,11 @@ export class BundlerContext {
446464
* @returns A promise that resolves when disposal is complete.
447465
*/
448466
async dispose(): Promise<void> {
467+
this.#disposed = true;
449468
try {
450469
this.#esbuildOptions = undefined;
451470
this.#esbuildResult = undefined;
471+
this.#activeBundlePromise = undefined;
452472
this.#loadCache = undefined;
453473
await this.#esbuildContext?.dispose();
454474
} finally {

packages/angular/build/src/tools/esbuild/cache.ts

Lines changed: 80 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -44,11 +44,21 @@ export interface CacheStore<V> {
4444
* to use a cache.
4545
*/
4646
export class Cache<V, S extends CacheStore<V> = CacheStore<V>> {
47+
readonly #requests = new Map<string, Promise<V>>();
48+
readonly #writeCounts = new Map<string, number>();
49+
readonly #pendingGets = new Map<string, number>();
50+
4751
constructor(
4852
protected readonly store: S,
4953
readonly namespace?: string,
5054
) {}
5155

56+
#incrementWrite(key: string) {
57+
if (this.#pendingGets.has(key)) {
58+
this.#writeCounts.set(key, (this.#writeCounts.get(key) || 0) + 1);
59+
}
60+
}
61+
5262
/**
5363
* Prefixes a key with the cache namespace if present.
5464
* @param key A key string to prefix.
@@ -72,14 +82,65 @@ export class Cache<V, S extends CacheStore<V> = CacheStore<V>> {
7282
*/
7383
async getOrCreate(key: string, creator: () => V | Promise<V>): Promise<V> {
7484
const namespacedKey = this.withNamespace(key);
75-
let value = await this.store.get(namespacedKey);
7685

77-
if (value === undefined) {
78-
value = await creator();
79-
await this.store.set(namespacedKey, value);
86+
let activeRequest = this.#requests.get(namespacedKey);
87+
if (activeRequest !== undefined) {
88+
return activeRequest;
8089
}
8190

82-
return value;
91+
const currentPending = this.#pendingGets.get(namespacedKey) || 0;
92+
this.#pendingGets.set(namespacedKey, currentPending + 1);
93+
94+
try {
95+
const startWriteCount = this.#writeCounts.get(namespacedKey) || 0;
96+
97+
const value = await this.store.get(namespacedKey);
98+
99+
// If a write occurred during the await gap, we must re-evaluate
100+
if ((this.#writeCounts.get(namespacedKey) || 0) !== startWriteCount) {
101+
return this.getOrCreate(key, creator);
102+
}
103+
104+
if (value !== undefined) {
105+
return value;
106+
}
107+
108+
// Recheck active request after the await gap in case another one was initiated
109+
activeRequest = this.#requests.get(namespacedKey);
110+
if (activeRequest !== undefined) {
111+
return activeRequest;
112+
}
113+
114+
activeRequest = Promise.resolve(creator()).then(
115+
async (newValue) => {
116+
if (this.#requests.get(namespacedKey) === activeRequest) {
117+
this.#incrementWrite(namespacedKey);
118+
await this.store.set(namespacedKey, newValue);
119+
this.#requests.delete(namespacedKey);
120+
}
121+
122+
return newValue;
123+
},
124+
(error) => {
125+
if (this.#requests.get(namespacedKey) === activeRequest) {
126+
this.#requests.delete(namespacedKey);
127+
}
128+
throw error;
129+
},
130+
);
131+
132+
this.#requests.set(namespacedKey, activeRequest);
133+
134+
return activeRequest;
135+
} finally {
136+
const current = this.#pendingGets.get(namespacedKey) || 0;
137+
if (current <= 1) {
138+
this.#pendingGets.delete(namespacedKey);
139+
this.#writeCounts.delete(namespacedKey);
140+
} else {
141+
this.#pendingGets.set(namespacedKey, current - 1);
142+
}
143+
}
83144
}
84145

85146
/**
@@ -100,7 +161,19 @@ export class Cache<V, S extends CacheStore<V> = CacheStore<V>> {
100161
* @param value A value to put in the cache.
101162
*/
102163
async put(key: string, value: V): Promise<void> {
103-
await this.store.set(this.withNamespace(key), value);
164+
const namespacedKey = this.withNamespace(key);
165+
this.#requests.delete(namespacedKey);
166+
this.#incrementWrite(namespacedKey);
167+
await this.store.set(namespacedKey, value);
168+
}
169+
170+
/**
171+
* Clears the base class internal state (requests, write counts, and pending gets).
172+
*/
173+
protected clearInternal(): void {
174+
this.#requests.clear();
175+
this.#writeCounts.clear();
176+
this.#pendingGets.clear();
104177
}
105178
}
106179

@@ -116,6 +189,7 @@ export class MemoryCache<V> extends Cache<V, Map<string, V>> {
116189
* Removes all entries from the cache instance.
117190
*/
118191
clear() {
192+
this.clearInternal();
119193
this.store.clear();
120194
}
121195

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.dev/license
7+
*/
8+
9+
import { MemoryCache } from './cache';
10+
11+
describe('MemoryCache', () => {
12+
let cache: MemoryCache<string>;
13+
14+
beforeEach(() => {
15+
cache = new MemoryCache<string>();
16+
});
17+
18+
it('should return cached value on subsequent getOrCreate calls', async () => {
19+
let callCount = 0;
20+
const creator = () => {
21+
callCount++;
22+
23+
return 'value';
24+
};
25+
26+
const val1 = await cache.getOrCreate('key', creator);
27+
const val2 = await cache.getOrCreate('key', creator);
28+
29+
expect(val1).toBe('value');
30+
expect(val2).toBe('value');
31+
expect(callCount).toBe(1);
32+
});
33+
34+
it('should call creator only once for concurrent getOrCreate calls with the same key', async () => {
35+
let callCount = 0;
36+
let resolveCreator!: (value: string) => void;
37+
const promise = new Promise<string>((resolve) => {
38+
resolveCreator = resolve;
39+
});
40+
41+
const creator = () => {
42+
callCount++;
43+
44+
return promise;
45+
};
46+
47+
const p1 = cache.getOrCreate('key', creator);
48+
const p2 = cache.getOrCreate('key', creator);
49+
50+
resolveCreator('concurrent-value');
51+
52+
const [val1, val2] = await Promise.all([p1, p2]);
53+
54+
expect(val1).toBe('concurrent-value');
55+
expect(val2).toBe('concurrent-value');
56+
expect(callCount).toBe(1);
57+
});
58+
59+
it('should call creator multiple times for concurrent getOrCreate calls with different keys', async () => {
60+
let callCount = 0;
61+
const creator = (val: string) => {
62+
callCount++;
63+
64+
return Promise.resolve(val);
65+
};
66+
67+
const p1 = cache.getOrCreate('key1', () => creator('value1'));
68+
const p2 = cache.getOrCreate('key2', () => creator('value2'));
69+
70+
const [val1, val2] = await Promise.all([p1, p2]);
71+
72+
expect(val1).toBe('value1');
73+
expect(val2).toBe('value2');
74+
expect(callCount).toBe(2);
75+
});
76+
77+
it('should clean up active request if creator throws/rejects', async () => {
78+
let callCount = 0;
79+
let rejectCreator!: (err: Error) => void;
80+
const promise = new Promise<string>((_, reject) => {
81+
rejectCreator = reject;
82+
});
83+
84+
const creator = () => {
85+
callCount++;
86+
87+
return promise;
88+
};
89+
90+
const p1 = cache.getOrCreate('key', creator);
91+
const p2 = cache.getOrCreate('key', creator);
92+
93+
rejectCreator(new Error('creator error'));
94+
95+
await expectAsync(p1).toBeRejectedWithError('creator error');
96+
await expectAsync(p2).toBeRejectedWithError('creator error');
97+
98+
// Subsequent call should trigger the creator again
99+
const p3 = cache.getOrCreate('key', () => {
100+
callCount++;
101+
102+
return Promise.resolve('new-value');
103+
});
104+
const val3 = await p3;
105+
expect(val3).toBe('new-value');
106+
expect(callCount).toBe(2);
107+
});
108+
109+
it('should override/clear active requests when put is called', async () => {
110+
let resolveCreator!: (value: string) => void;
111+
const promise = new Promise<string>((resolve) => {
112+
resolveCreator = resolve;
113+
});
114+
115+
let creatorStarted!: (value: void) => void;
116+
const creatorStartedPromise = new Promise<void>((resolve) => {
117+
creatorStarted = resolve;
118+
});
119+
120+
const creator = () => {
121+
creatorStarted();
122+
123+
return promise;
124+
};
125+
126+
const p1 = cache.getOrCreate('key', creator);
127+
128+
// Wait for the creator to be called so that the active request is created
129+
await creatorStartedPromise;
130+
131+
// Call put before the creator promise resolves
132+
await cache.put('key', 'override-value');
133+
134+
resolveCreator('original-value');
135+
136+
const val1 = await p1;
137+
// p1 was already returned, so it resolves to original-value
138+
expect(val1).toBe('original-value');
139+
140+
// Subsequent getOrCreate should return the put/overridden value, not the resolved original-value
141+
const val2 = await cache.getOrCreate('key', () => 'should-not-run');
142+
expect(val2).toBe('override-value');
143+
});
144+
});

0 commit comments

Comments
 (0)