fix(rolldown_binding): use Weak<OutputChunk> to prevent memory leak
#6346
Conversation
The js side should not own the Arc.
How to use the Graphite Merge QueueAdd the label graphite: merge to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
✅ Deploy Preview for rolldown-rs canceled.
|
| } | ||
|
|
||
| fn inner(&self) -> Arc<OutputAsset> { | ||
| self.inner.upgrade().unwrap() |
There was a problem hiding this comment.
Instead of crashing, the js side can set the value to undefined, or we throw a panic message "Rolldown has been shutdown, access to this data is no longer possible".
| pub(crate) cache: ScanStageCache, | ||
| pub(crate) session: rolldown_debug::Session, | ||
| pub(crate) build_count: u32, | ||
| pub(crate) assets: Vec<Output>, |
There was a problem hiding this comment.
Bundler needs to own the Arcs from Output.
There was a problem hiding this comment.
Not oppose this direction, but would it casue error if the bundler/assets get dropped and user try to access the assets?
This brings up a convention that if users wants to access the asset, they shouldn't make the the js object Bundler get GCed, which is hard thing to tell in the js world due to js doesn't have ownership concept.
There was a problem hiding this comment.
Yes, for script usages, use after free will not be allowed. i.e. it will crash when accessing asset data after rolldown has been closed.
|
|
||
| self.assets.clear(); | ||
| self.cache = ScanStageCache::default(); | ||
| self.file_emitter.clear(); |
There was a problem hiding this comment.
We need to clear everything stored in Bundler instead of relying on Drop.
There was a problem hiding this comment.
I think this improvement could be appied first. No negative point found.
| } | ||
|
|
||
| let mut output = bundle_output?; | ||
| self.assets.extend(output.assets.iter().map(Clone::clone)); |
There was a problem hiding this comment.
This line might get triggered many times due to incremental build.
There was a problem hiding this comment.
Something else should own the assets, I stored them in bundler to prove my hypothesis.
…emory immediately (#6721) This PR consolidates #6707 and #6696. Part of #6346. ```typescript import { freeExternalMemory } from 'rolldown/experimental'; const bundle = await build.generate(); freeExternalMemory(bundle) bundle.output // error ``` ```typescript import { freeExternalMemory } from 'rolldown/experimental'; const bundle = await build.generate(); freeExternalMemory(bundle, /*keepDataAlive:*/ true) bundle.output // no error ```
may fix #6121
The js side should not own the Arc.