fix(#31973): Added JavaScriptTransformer close to dispose method #32082
Conversation
There was a problem hiding this comment.
Please update the commit msg to the below (as per guidelines) and squash the commits.
fix(@angular/build): added JavaScriptTransformer close to dispose method
This change allows for awaiting the disposed plugin and added the
missing close method of the JavaScript transformer to the onDispose.
When the plugin is run multiple times on different projects/bundles
without proper closing it will result in a node heap corruption. This
allows us to cleanup the build before we start a new one.
Closes #31973
packages/angular/build/src/tools/esbuild/angular/compiler-plugin.ts
Outdated
Show resolved
Hide resolved
packages/angular/build/src/tools/esbuild/angular/compiler-plugin.ts
Outdated
Show resolved
Hide resolved
alan-agius4
added
action: cleanup
Aukevanoost
force-pushed
the
main
branch
3 times, most recently
from
99183e1 to
43a2631
Compare
|
It should be good now. Let me know if I'm missing something. |
|
I think I found a robust fix that doesn't break the ESlint rules, but only exposes an Promise. Let me know what you think! |
This change allows for awaiting the disposed plugin and added the missing close method of the JavaScript transformer to the onDispose. When the plugin is run multiple times on different projects/bundles without proper closing it will result in a node heap corruption. This allows us to cleanup the build before we start a new one. Closes angular#31973
| void compilation.close?.(); | ||
| void cacheStore?.close(); | ||
| }); | ||
| build.onDispose( |
There was a problem hiding this comment.
Looking at this more, isn't the below enough to your issue? Is the getSharedCompilationStateCleanup truly needed?
build.onDispose(() => {
sharedTSCompilationState?.dispose();
void compilation.close?.();
void cacheStore?.close();
void javascriptTransformer.close();
});
The compilation, cacheStore and javascriptTransformer are local variables and scoped to a single plugin. The only shared instance is sharedTSCompilationState which it's clean up is sync. I am still failing to understand how the sharedTSCompilationState is causing the heap error, as this is used primarily for NoopCompilations which in your case you are not using.
My point here is that dispose can be fire and forget and it should not be blocking to start a new compilation, as long as sharedTSCompilationState.dispose is sync.
There was a problem hiding this comment.
Hi great question.
While the properties are local, their workers and state aren't. The sharedTSCompilationState is not the issue but the solution. The compilation and javascriptTransformer contain state and workers that crash the node:process on sequential ESbuilds. This is caused by the fact that the new build/compilation run is using workers and state that hasn't been cleaned up yet in the previous build, causing the heap corruption. The only way to fix this is to wait for all the workers and processes to be cleaned up before starting the next ESbuild run. The exposed getSharedCompilationStateCleanup function allows me to do this.
There was a problem hiding this comment.
The
sharedTSCompilationStateis not the issue but the solution.
But in your plugin you are not even using sharedTSCompilationState since none of the compilations mode is noop.
While the properties are local, their workers and state aren't .... The compilation and javascriptTransformer contain state and workers that crash the node:process on sequential ESbuilds. This is caused by the fact that the new build/compilation run is using workers and state that hasn't been cleaned up yet in the previous build
I'm confused by this. What non local state are you your referring too? Both the compilation and the transformer operate with local state; they do not rely on a share state. Therefore, creating multiple instances of createCompilerPlugin() will not result in a single, shared pool of workers or compilation environment. Each invocation remains completely isolated and initializes its own separate resources. We do this in the Angular CLI where each ng build creates multiple instances of createCompilerPlugin.
There was a problem hiding this comment.
While you are correct about the sharedTSCompilationState only used in noop mode, it is still being initialized regardless of the compilation type: https://github.com/angular/angular-cli/blob/c4af1932c3e8e94022237ea8e56adb19bf338978/packages/angular/build/src/tools/esbuild/angular/compiler-plugin.ts#L146C1-L146C64 therefore it is (only) being used for the clean disposal signal.
About the scoped workers and state, while I still can't proof exactly where it's happening, awaiting this disposal does prevent the heap error. I'll have to dig deeper to find out exactly which part is causing this.
There was a problem hiding this comment.
While you are correct about the sharedTSCompilationState only used in noop mode, it is still being initialized regardless of the compilation type: https://github.com/angular/angular-cli/blob/c4af1932c3e8e94022237ea8e56adb19bf338978/packages/angular/build/src/tools/esbuild/angular/compiler-plugin.ts#L146C1-L146C64 therefore it is (only) being used for the clean disposal signal
That seems like an oversight.
| build.onDispose( | ||
| () => | ||
| void Promise.all( | ||
| [compilation?.close?.(), cacheStore?.close(), javascriptTransformer.close()].filter( |
There was a problem hiding this comment.
If you do not mind, I might create (or you can if you wish too), to actually call include just void javascriptTransformer.close(); so that it's included in today's release.
There was a problem hiding this comment.
I can create a separate PR for this if you don't mind!
alan-agius4
removed
the
action: cleanup
2 participants
This change allows for awaiting the disposed plugin and added the missing close method of the JavaScript transformer to the onDispose. When the plugin is run multiple times on different projects/bundles without proper closing it will result in a node heap corruption. This allows us to cleanup the build before we start a new one.
PR Checklist
Please check to confirm your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
The current behavior doesn't cleanup the JavascriptTransformer and also doesn't await the promises. I know that ESbuild is not yet processing promises in the onDispose callback but I am working on that! evanw/esbuild#4356
In the meantime ESbuild will just treat the callback as a void so no functional changes.
Closes #31973.
What is the new behavior?
For Angular no functional changes other than the clean disposal of the JavascriptTransformer to avoid Node heap corruptions.
Does this PR introduce a breaking change?
Other information
For us this means that we can create a wrapper around the plugin. This wrapper creates a promise from the callback and awaits that promise after an ESbuild dispose. This allows us to fully cleanup the build after it's finished and thus avoids unwanted memory leaks.