Skip to content

Commit 983db14

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 983db14

3 files changed

Lines changed: 216 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: 63 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -44,11 +44,18 @@ 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+
4750
constructor(
4851
protected readonly store: S,
4952
readonly namespace?: string,
5053
) {}
5154

55+
#incrementWrite(key: string) {
56+
this.#writeCounts.set(key, (this.#writeCounts.get(key) || 0) + 1);
57+
}
58+
5259
/**
5360
* Prefixes a key with the cache namespace if present.
5461
* @param key A key string to prefix.
@@ -72,14 +79,52 @@ export class Cache<V, S extends CacheStore<V> = CacheStore<V>> {
7279
*/
7380
async getOrCreate(key: string, creator: () => V | Promise<V>): Promise<V> {
7481
const namespacedKey = this.withNamespace(key);
75-
let value = await this.store.get(namespacedKey);
7682

77-
if (value === undefined) {
78-
value = await creator();
79-
await this.store.set(namespacedKey, value);
83+
let activeRequest = this.#requests.get(namespacedKey);
84+
if (activeRequest !== undefined) {
85+
return activeRequest;
8086
}
8187

82-
return value;
88+
const startWriteCount = this.#writeCounts.get(namespacedKey) || 0;
89+
90+
const value = await this.store.get(namespacedKey);
91+
92+
// If a write occurred during the await gap, we must re-evaluate
93+
if ((this.#writeCounts.get(namespacedKey) || 0) !== startWriteCount) {
94+
return this.getOrCreate(key, creator);
95+
}
96+
97+
if (value !== undefined) {
98+
return value;
99+
}
100+
101+
// Recheck active request after the await gap in case another one was initiated
102+
activeRequest = this.#requests.get(namespacedKey);
103+
if (activeRequest !== undefined) {
104+
return activeRequest;
105+
}
106+
107+
activeRequest = Promise.resolve(creator()).then(
108+
async (newValue) => {
109+
if (this.#requests.get(namespacedKey) === activeRequest) {
110+
this.#incrementWrite(namespacedKey);
111+
await this.store.set(namespacedKey, newValue);
112+
this.#requests.delete(namespacedKey);
113+
}
114+
115+
return newValue;
116+
},
117+
(error) => {
118+
if (this.#requests.get(namespacedKey) === activeRequest) {
119+
this.#requests.delete(namespacedKey);
120+
}
121+
throw error;
122+
},
123+
);
124+
125+
this.#requests.set(namespacedKey, activeRequest);
126+
127+
return activeRequest;
83128
}
84129

85130
/**
@@ -100,7 +145,18 @@ export class Cache<V, S extends CacheStore<V> = CacheStore<V>> {
100145
* @param value A value to put in the cache.
101146
*/
102147
async put(key: string, value: V): Promise<void> {
103-
await this.store.set(this.withNamespace(key), value);
148+
const namespacedKey = this.withNamespace(key);
149+
this.#requests.delete(namespacedKey);
150+
this.#incrementWrite(namespacedKey);
151+
await this.store.set(namespacedKey, value);
152+
}
153+
154+
/**
155+
* Clears the base class internal state (requests and write counts).
156+
*/
157+
protected clearInternal(): void {
158+
this.#requests.clear();
159+
this.#writeCounts.clear();
104160
}
105161
}
106162

@@ -116,6 +172,7 @@ export class MemoryCache<V> extends Cache<V, Map<string, V>> {
116172
* Removes all entries from the cache instance.
117173
*/
118174
clear() {
175+
this.clearInternal();
119176
this.store.clear();
120177
}
121178

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
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+
const creator = () => promise;
116+
117+
const p1 = cache.getOrCreate('key', creator);
118+
119+
// Call put before the creator promise resolves
120+
await cache.put('key', 'override-value');
121+
122+
resolveCreator('original-value');
123+
124+
const val1 = await p1;
125+
expect(val1).toBe('override-value');
126+
127+
// Subsequent getOrCreate should return the put/overridden value, not the resolved original-value
128+
const val2 = await cache.getOrCreate('key', () => 'should-not-run');
129+
expect(val2).toBe('override-value');
130+
});
131+
});

0 commit comments

Comments
 (0)