Release build resources early in non-watch mode and prevent process hang on unhandled error - #33715
Conversation
angular-robot
Bot
added
area: performance
alan-agius4
added
action: review
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
aecad28 to
3709966
Compare
Disposes the Angular compilation and bundler contexts within the build execution for non-watch builds once bundling has completed. This terminates the TypeScript compilation worker and JavaScript transformation workers before the post-bundle steps (chunk optimization, i18n inlining, index generation) execute instead of after all results have been emitted. Repeat compilation disposal calls now return the in progress disposal to ensure all callers can await completion of the underlying compiler shutdown. The result disposal also releases the incremental build caches which can retain the emitted contents of every TypeScript file within the program. This reduces the peak memory usage of the build for both the non-watch early disposal and watch mode teardown. Closes angular#33368
3709966 to
ef75f6a
Compare
When an unhandled exception is thrown during a build (such as IndexHtmlGenerator.readIndex throwing when an index HTML file is missing or unreadable), executeBuild() previously re-threw without calling .dispose() on executionResult or angularCompilationContext. Consequently, open ESBuild child processes, TypeScript compiler worker threads, and SQLite database connections prevented the Node.js process from exiting. This commit addresses the issue by: 1. Wrapping the entire body of executeBuild() in a try...catch block and instantiating executionResult immediately after bundlerContexts is created, so any thrown error reliably disposes all worker pools and AST caches before re-throwing. 2. Closing the SQLite cache store (angular-i18n.db) in I18nInliner.close() alongside worker pool destruction. 3. Ensuring Sass workers are shut down in build-action even if an initial --watch build throws an exception. Closes angular#33716
7ffbbac to
c0cf4a5
Compare
alan-agius4
changed the title
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves resource management and cleanup during the build process. Key changes include disposing of bundler rebuild contexts, Angular compilation contexts, and caches early in non-watch mode or when a build fails. Additionally, disposal methods across various classes (such as AngularCompilationContext and ExecutionResult) have been updated to cache and reuse their disposal promises to prevent redundant teardown operations. A test was also updated to ensure the build terminates cleanly when a non-existent index HTML file is provided. There are no review comments, so I have no feedback to provide.
alan-agius4
changed the title
| // This is fire-and-forget so worker teardown runs concurrently in the background | ||
| // without blocking the critical path of post-bundle optimization steps. | ||
| // Any in-flight disposal is awaited in the builder action's finally block. | ||
| void Promise.allSettled([angularCompilationContext?.dispose(), executionResult.dispose()]); |
There was a problem hiding this comment.
Consider: Do we need Promise.allSettled if we're not await-ing it? Can we just do:
void angularCompilationContext?.dispose();
void executeResult.dispose();There was a problem hiding this comment.
Good point! Since we aren't awaiting it here (and any in-flight disposal promises are awaited in the builder action's finally block), Promise.allSettled is unnecessary. Updated to call .dispose() on each directly.
| dispose(): void { | ||
| this.clear(); | ||
| this.modifiedFiles.clear(); | ||
| this.typeScriptFileCache.clear(); | ||
| this.loadResultCache.clear(); | ||
| this.referencedFiles = undefined; | ||
| } |
There was a problem hiding this comment.
Consider: This looks like it's just clearing maps, not really releasing any resources. Do we need this dispose, or could we just have the consumer of this class drop its reference to this SourceFileCache object? Wouldn't that have the same GC effect?
OTOH, "clearing the cache" while still keep a reference to that cache does seem like a reasonable feature, so maybe I'm just getting hung up on the name dispose? The class is still valid after being disposed right? Would clear be a better name to communicate that?
There was a problem hiding this comment.
Great observation. In non-watch mode, ExecutionResult is returned by executeBuild() and passed to post-bundling steps (such as index HTML generation, service worker generation, and disk writing) while holding a reference to codeBundleCache. Clearing the cache here immediately releases cached TypeScript ASTs and emitted file contents before those post-bundling steps execute without requiring callers to drop their reference to ExecutionResult.
Renamed this method to override clear() (and added a super.clear() call) to better communicate that it clears all cached entries across the internal maps while leaving the cache instance valid for potential reuse.
- Replace unnecessary Promise.allSettled with direct dispose calls in execute-build.ts - Rename SourceFileCache dispose() to override clear() and call super.clear() to clear base Map alongside internal cache structures
e3ad0d6 to
b3fe12b
Compare
alan-agius4
added
action: merge
b3fe12b to
e934d32
Compare
…#33715) Disposes the Angular compilation and bundler contexts within the build execution for non-watch builds once bundling has completed. This terminates the TypeScript compilation worker and JavaScript transformation workers before the post-bundle steps (chunk optimization, i18n inlining, index generation) execute instead of after all results have been emitted. Repeat compilation disposal calls now return the in progress disposal to ensure all callers can await completion of the underlying compiler shutdown. The result disposal also releases the incremental build caches which can retain the emitted contents of every TypeScript file within the program. This reduces the peak memory usage of the build for both the non-watch early disposal and watch mode teardown. Closes #33368 PR Close #33715
…33715) When an unhandled exception is thrown during a build (such as IndexHtmlGenerator.readIndex throwing when an index HTML file is missing or unreadable), executeBuild() previously re-threw without calling .dispose() on executionResult or angularCompilationContext. Consequently, open ESBuild child processes, TypeScript compiler worker threads, and SQLite database connections prevented the Node.js process from exiting. This commit addresses the issue by: 1. Wrapping the entire body of executeBuild() in a try...catch block and instantiating executionResult immediately after bundlerContexts is created, so any thrown error reliably disposes all worker pools and AST caches before re-throwing. 2. Closing the SQLite cache store (angular-i18n.db) in I18nInliner.close() alongside worker pool destruction. 3. Ensuring Sass workers are shut down in build-action even if an initial --watch build throws an exception. Closes #33716 PR Close #33715
…33715) When an unhandled exception is thrown during a build (such as IndexHtmlGenerator.readIndex throwing when an index HTML file is missing or unreadable), executeBuild() previously re-threw without calling .dispose() on executionResult or angularCompilationContext. Consequently, open ESBuild child processes, TypeScript compiler worker threads, and SQLite database connections prevented the Node.js process from exiting. This commit addresses the issue by: 1. Wrapping the entire body of executeBuild() in a try...catch block and instantiating executionResult immediately after bundlerContexts is created, so any thrown error reliably disposes all worker pools and AST caches before re-throwing. 2. Closing the SQLite cache store (angular-i18n.db) in I18nInliner.close() alongside worker pool destruction. 3. Ensuring Sass workers are shut down in build-action even if an initial --watch build throws an exception. Closes #33716 PR Close #33715
2 participants
This PR combines two related improvements to resource lifecycle management and error handling in
@angular/build:application:1. Early Resource Release in Non-Watch Mode
angularCompilationContext,typescriptContexts,otherContexts,componentStyleBundler, andcodeBundleCacheearly in non-watch mode once bundling completes.ParallelCompilation), AST caches (SourceFileCache,MemoryLoadResultCache), and ESBuild child processes before post-bundle steps and disk writing.AngularCompilationContext.dispose()andExecutionResult.dispose()memoized and idempotent so subsequent disposal calls safely share#disposalpromises.2. Process Hang Prevention on Unhandled Build Errors (#33716)
executeBuild()in a comprehensivetry...catchblock and instantiatesexecutionResultimmediately afterbundlerContextsis created, guaranteeing that all ESBuild child processes, TypeScript compiler worker threads, and stylesheet bundlers are disposed if an error is thrown at any stage of the build.I18nInliner.close()by closingthis.#cache(angular-i18n.db) alongside worker pool destruction.build-actioneven if an initial--watchbuild throws an unhandled exception.Note
Reviewer Note: Since wrapping
executeBuild()in atry...catchblock indents the function body, it is recommended to review this PR with "Hide whitespace" enabled in GitHub diff settings for easier review.