fix: panic in add_entry_batch when loader calls importModule#13410
fix: panic in add_entry_batch when loader calls importModule#13410henryqdineen wants to merge 6 commits intoweb-infra-dev:mainfrom
Conversation
github-actions
Bot
added
the
release: bug fix
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61dea169c5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| let make_artifact = self.build_module_graph_artifact.steal(); | ||
| let exports_info_artifact = self.exports_info_artifact.steal(); | ||
| let (make_artifact, exports_info_artifact) = update_module_graph( |
There was a problem hiding this comment.
If update_module_graph(...).await? fails after before_build_module_graph has started the executor, this returns before the teardown at lines 686-690 runs. The new entries were already registered at lines 651-652, so a JS plugin can catch the addEntry callback error and still let compilation continue; do_build_module_graph then starts the executor again unconditionally in crates/rspack_core/src/compilation/build_module_graph/mod.rs:33-38, replacing the old channel and orphaning the first background task. This leaves the executor in exactly the stale state the patch is trying to avoid, so the temporary lifecycle needs cleanup on both success and error paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Should be resolved if you want to re-review.
henryqdineen
force-pushed
the
hqd-fix-add-entry-import-module
branch
from
a493040 to
93a321d
Compare
Merging this PR will not alter performance
Comparing 
Footnotes
|
When `compilation.addEntry()` is called from the JS `make` hook, `add_entry_batch` eagerly builds modules via `update_module_graph` to match webpack's callback semantics (the callback receives the built module). However, the `ModuleExecutor` event channel was never initialized for this code path, so any loader that calls `this.importModule()` during the build would panic: panicked at crates/rspack_core/src/compilation/build_module_graph/module_executor/mod.rs should have event sender The normal build path (`do_build_module_graph`) calls `before_build_module_graph` which sets up the channel before `update_module_graph` runs. `add_entry_batch` skipped this step. This fix ensures the module executor is started before the eager build in `add_entry_batch` and torn down afterward so that `build_module_graph_pass` can set it up fresh. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Defer the `?` on `update_module_graph` result until after the executor is torn down, so the background task isn't orphaned if the build fails. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
after_build_module_graph reads compilation.build_module_graph_artifact, so the artifacts must be restored before teardown. On error, reset to defaults to avoid reading stolen state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use StealCell::new instead of Default, move artifacts instead of cloning on the success path, and simplify the match. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
henryqdineen
force-pushed
the
hqd-fix-add-entry-import-module
branch
from
e292a07 to
1ecfff2
Compare
1 participant
Summary
When
compilation.addEntry()is called from a JS plugin'smakehook and the added entry has a loader that callsthis.importModule(), rspack panics:I originally contributed
addEntrysupport in #10268, modeled afteraddInclude. The eager build inadd_entry_batchwas part of that implementation to match webpack's callback semantics. This PR fixes a gap where theModuleExecutorwasn't initialized for that eager build path.Root cause
add_entry_batcheagerly builds modules viaupdate_module_graphto match webpack'saddEntrycallback contract (the callback receives the built module). However, theModuleExecutorevent channel is never initialized for this code path.The normal build path (
do_build_module_graph) callsbefore_build_module_graphto set up the channel before building.add_entry_batchskips this, soModuleExecutor::import_modulepanics on the missingevent_sender.Fix
Before the eager
update_module_graphcall inadd_entry_batch, start the module executor if it isn't already running. Tear it down afterward sobuild_module_graph_passcan set it up fresh.Tradeoffs considered
Why not remove the eager build?
add_entry_batchbuilds eagerly to match webpack's API where theaddEntrycallback receives the built module. Removing it would be a behavior change — callers that read the module in the callback would get an error instead.Why not leave the executor running?
do_build_module_graphunconditionally callsbefore_build_module_graph, which would overwrite the channel and orphan the background task. Tearing down first is the safe path.Double executor lifecycle: This adds a second
before/after_build_module_graphcycle where there was previously one. It only fires whenaddEntryis called from JS duringmakewith a loader that usesimportModule. Long-term, the// TODO move to MakeArtifactonmodule_executorwould eliminate this by tying the executor lifecycle to the artifact.Test
Added
configCases/compilation/add-entry-import-module— a plugin adds an entry viacompilation.addEntry()during themakehook, and that entry's loader callsthis.importModule(). Without the fix, this panics withshould have event sender.