fix(@angular/build): prevent concurrent stylesheet bundling esbuild context leaks#33318
fix(@angular/build): prevent concurrent stylesheet bundling esbuild context leaks#33318clydin wants to merge 1 commit into
Conversation
…ontext 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 angular#33317
There was a problem hiding this comment.
Code Review
This pull request introduces request deduplication and write tracking to the Cache class to handle concurrent getOrCreate calls safely, alongside improvements to BundlerContext to manage active bundle promises and disposal. The review feedback highlights a potential race condition in Cache.getOrCreate where an active request might be incorrectly deleted after an await gap, a failing assertion in the new unit tests regarding promise resolution after a cache override, and a potential memory leak due to #writeCounts entries never being cleaned up.
| if (this.#requests.get(namespacedKey) === activeRequest) { | ||
| this.#incrementWrite(namespacedKey); | ||
| await this.store.set(namespacedKey, newValue); | ||
| this.#requests.delete(namespacedKey); | ||
| } |
There was a problem hiding this comment.
There is a potential race condition here. After await this.store.set(namespacedKey, newValue) resolves, the active request in this.#requests might have changed (for example, if put() was called and then a new getOrCreate() was initiated during the await gap). Calling this.#requests.delete(namespacedKey) unconditionally inside the if block will delete the new active request instead of the current one.
To prevent this, we should re-verify that this.#requests.get(namespacedKey) === activeRequest is still true after the await before deleting it.
| if (this.#requests.get(namespacedKey) === activeRequest) { | |
| this.#incrementWrite(namespacedKey); | |
| await this.store.set(namespacedKey, newValue); | |
| this.#requests.delete(namespacedKey); | |
| } | |
| if (this.#requests.get(namespacedKey) === activeRequest) { | |
| this.#incrementWrite(namespacedKey); | |
| await this.store.set(namespacedKey, newValue); | |
| if (this.#requests.get(namespacedKey) === activeRequest) { | |
| this.#requests.delete(namespacedKey); | |
| } | |
| } |
| resolveCreator('original-value'); | ||
|
|
||
| const val1 = await p1; | ||
| expect(val1).toBe('override-value'); |
There was a problem hiding this comment.
In this test, p1 is the promise returned by getOrCreate('key', creator). Since creator returns promise (which is resolved with 'original-value'), the promise p1 will eventually resolve to 'original-value'. Calling cache.put('key', 'override-value') will update the cache store and delete the active request from the internal map, but it cannot retroactively change the resolution value of the already-created promise p1. Therefore, await p1 will resolve to 'original-value', and expect(val1).toBe('override-value') will fail.
The assertion should be updated to expect 'original-value' for val1, while val2 (the subsequent getOrCreate call) is the one that correctly returns 'override-value'.
| expect(val1).toBe('override-value'); | |
| expect(val1).toBe('original-value'); |
| */ | ||
| export class Cache<V, S extends CacheStore<V> = CacheStore<V>> { | ||
| readonly #requests = new Map<string, Promise<V>>(); | ||
| readonly #writeCounts = new Map<string, number>(); |
There was a problem hiding this comment.
The #writeCounts map is used to detect mid-flight writes during the store.get await gap. However, entries are added to #writeCounts but never deleted (except when the entire cache is cleared). In a long-running process like ng serve with many unique keys over time, this will cause a memory leak as #writeCounts grows indefinitely.
Since write counts are only needed to detect writes that occur during an active store.get call, we can track the number of pending get requests for each key and safely delete the write count entry when there are no more pending gets.
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