From 75f95737db0fecbca6c739991a04add6cdfc4456 Mon Sep 17 00:00:00 2001 From: Cong-Cong Pan Date: Fri, 8 Nov 2024 16:40:32 +0800 Subject: [PATCH] fix: tapable run in wrong closure (#8376) --- Cargo.lock | 2 - crates/node_binding/Cargo.toml | 2 - .../node_binding/src/plugins/interceptor.rs | 80 ++++++++--- crates/node_binding/src/plugins/mod.rs | 84 +++++++++++ crates/rspack_core/src/compiler/hmr.rs | 3 +- crates/rspack_core/src/compiler/mod.rs | 2 +- crates/rspack_core/src/plugin/mod.rs | 2 + .../rspack_core/src/plugin/plugin_driver.rs | 7 + .../tests/compilerCases/hooks-closure.js | 134 ++++++++++++++++++ 9 files changed, 288 insertions(+), 28 deletions(-) create mode 100644 packages/rspack-test-tools/tests/compilerCases/hooks-closure.js diff --git a/Cargo.lock b/Cargo.lock index 25a04d7562a..20f99363565 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3911,7 +3911,6 @@ dependencies = [ "napi", "napi-build", "napi-derive", - "once_cell", "ropey", "rspack_allocator", "rspack_binding_options", @@ -3929,7 +3928,6 @@ dependencies = [ "rspack_plugin_javascript", "rspack_tracing", "rspack_util", - "tokio", "tracing", ] diff --git a/crates/node_binding/Cargo.toml b/crates/node_binding/Cargo.toml index 49bc698b86d..0304f1e58f7 100644 --- a/crates/node_binding/Cargo.toml +++ b/crates/node_binding/Cargo.toml @@ -32,11 +32,9 @@ rspack_plugin_javascript = { version = "0.1.0", path = "../rspack_plugin_javascr rspack_util = { version = "0.1.0", path = "../rspack_util" } rspack_tracing = { version = "0.1.0", path = "../rspack_tracing" } -tokio = { workspace = true, features = ["rt", "rt-multi-thread", "tracing"] } async-trait = { workspace = true } cow-utils = { workspace = true } -once_cell = { workspace = true } tracing = { workspace = true } napi = { workspace = true } diff --git a/crates/node_binding/src/plugins/interceptor.rs b/crates/node_binding/src/plugins/interceptor.rs index 0556c951d5d..aaa5b4c7968 100644 --- a/crates/node_binding/src/plugins/interceptor.rs +++ b/crates/node_binding/src/plugins/interceptor.rs @@ -1,5 +1,4 @@ use std::{ - borrow::Cow, hash::Hash, sync::{Arc, RwLock}, }; @@ -135,8 +134,7 @@ impl Clone for RegisterJsTapsInner { enum RegisterJsTapsCache { NoCache, - Cache(Arc>>), - SyncCache(Arc>>), + Cache(Arc>>>), } impl Clone for RegisterJsTapsCache { @@ -144,19 +142,14 @@ impl Clone for RegisterJsTapsCache { match self { Self::NoCache => Self::NoCache, Self::Cache(c) => Self::Cache(c.clone()), - Self::SyncCache(c) => Self::SyncCache(c.clone()), } } } impl RegisterJsTapsCache { - pub fn new(cache: bool, sync: bool) -> Self { + pub fn new(cache: bool, _sync: bool) -> Self { if cache { - if sync { - Self::SyncCache(Default::default()) - } else { - Self::Cache(Default::default()) - } + Self::Cache(Default::default()) } else { Self::NoCache } @@ -180,15 +173,27 @@ impl RegisterJsTapsInner rspack_error::Result>> { - if let RegisterJsTapsCache::Cache(cache) = &self.cache { - let js_taps = cache - .get_or_try_init(|| self.call_register_impl(hook)) - .await?; - Ok(Cow::Borrowed(js_taps)) + ) -> rspack_error::Result> { + if let RegisterJsTapsCache::Cache(rw) = &self.cache { + let cache = { + #[allow(clippy::unwrap_used)] + rw.read().unwrap().clone() + }; + Ok(match cache { + Some(js_taps) => js_taps, + None => { + let js_taps = self.call_register_impl(hook).await?; + { + #[allow(clippy::unwrap_used)] + let mut cache = rw.write().unwrap(); + *cache = Some(js_taps.clone()); + } + js_taps + } + }) } else { let js_taps = self.call_register_impl(hook).await?; - Ok(Cow::Owned(js_taps)) + Ok(js_taps) } } @@ -204,13 +209,27 @@ impl RegisterJsTapsInner rspack_error::Result>> { - if let RegisterJsTapsCache::SyncCache(cache) = &self.cache { - let js_taps = cache.get_or_try_init(|| self.call_register_blocking_impl(hook))?; - Ok(Cow::Borrowed(js_taps)) + ) -> rspack_error::Result> { + if let RegisterJsTapsCache::Cache(rw) = &self.cache { + let cache = { + #[allow(clippy::unwrap_used)] + rw.read().unwrap().clone() + }; + Ok(match cache { + Some(js_taps) => js_taps, + None => { + let js_taps = self.call_register_blocking_impl(hook)?; + { + #[allow(clippy::unwrap_used)] + let mut cache = rw.write().unwrap(); + *cache = Some(js_taps.clone()); + } + js_taps + } + }) } else { let js_taps = self.call_register_blocking_impl(hook)?; - Ok(Cow::Owned(js_taps)) + Ok(js_taps) } } @@ -222,6 +241,17 @@ impl RegisterJsTapsInner {} + RegisterJsTapsCache::Cache(cache) => { + #[allow(clippy::unwrap_used)] + let mut cache = cache.write().unwrap(); + *cache = None; + } + } + } } /// define js taps register @@ -243,6 +273,12 @@ macro_rules! define_register { inner: RegisterJsTapsInner<$arg, $ret>, } + impl $name { + pub fn clear_cache(&self) { + self.inner.clear_cache(); + } + } + #[derive(Clone)] struct $tap_name { function: ThreadsafeFunction<$arg, $ret>, diff --git a/crates/node_binding/src/plugins/mod.rs b/crates/node_binding/src/plugins/mod.rs index 64916079ec2..3d5a5b9a18f 100644 --- a/crates/node_binding/src/plugins/mod.rs +++ b/crates/node_binding/src/plugins/mod.rs @@ -312,6 +312,90 @@ impl rspack_core::Plugin for JsHooksAdapterPlugin { Ok(()) } + + fn clear_cache(&self) { + self.register_compiler_this_compilation_taps.clear_cache(); + self.register_compiler_compilation_taps.clear_cache(); + self.register_compiler_make_taps.clear_cache(); + self.register_compiler_finish_make_taps.clear_cache(); + self.register_compiler_should_emit_taps.clear_cache(); + self.register_compiler_emit_taps.clear_cache(); + self.register_compiler_after_emit_taps.clear_cache(); + self.register_compiler_asset_emitted_taps.clear_cache(); + self.register_compilation_build_module_taps.clear_cache(); + self + .register_compilation_still_valid_module_taps + .clear_cache(); + self.register_compilation_succeed_module_taps.clear_cache(); + self.register_compilation_execute_module_taps.clear_cache(); + self.register_compilation_finish_modules_taps.clear_cache(); + self + .register_compilation_optimize_modules_taps + .clear_cache(); + self + .register_compilation_after_optimize_modules_taps + .clear_cache(); + self.register_compilation_optimize_tree_taps.clear_cache(); + self + .register_compilation_optimize_chunk_modules_taps + .clear_cache(); + self + .register_compilation_additional_tree_runtime_requirements + .clear_cache(); + self + .register_compilation_runtime_requirement_in_tree + .clear_cache(); + self.register_compilation_runtime_module_taps.clear_cache(); + self.register_compilation_chunk_hash_taps.clear_cache(); + self.register_compilation_chunk_asset_taps.clear_cache(); + self.register_compilation_process_assets_taps.clear_cache(); + self + .register_compilation_after_process_assets_taps + .clear_cache(); + self.register_compilation_seal_taps.clear_cache(); + self.register_compilation_after_seal_taps.clear_cache(); + self + .register_normal_module_factory_before_resolve_taps + .clear_cache(); + self + .register_normal_module_factory_factorize_taps + .clear_cache(); + self + .register_normal_module_factory_resolve_taps + .clear_cache(); + self + .register_normal_module_factory_resolve_for_scheme_taps + .clear_cache(); + self + .register_normal_module_factory_after_resolve_taps + .clear_cache(); + self + .register_normal_module_factory_create_module_taps + .clear_cache(); + self + .register_context_module_factory_before_resolve_taps + .clear_cache(); + self + .register_context_module_factory_after_resolve_taps + .clear_cache(); + self + .register_javascript_modules_chunk_hash_taps + .clear_cache(); + self + .register_html_plugin_before_asset_tag_generation_taps + .clear_cache(); + self + .register_html_plugin_alter_asset_tags_taps + .clear_cache(); + self + .register_html_plugin_alter_asset_tag_groups_taps + .clear_cache(); + self + .register_html_plugin_after_template_execution_taps + .clear_cache(); + self.register_html_plugin_before_emit_taps.clear_cache(); + self.register_html_plugin_after_emit_taps.clear_cache(); + } } #[plugin_hook(CompilerCompilation for JsHooksAdapterPlugin)] diff --git a/crates/rspack_core/src/compiler/hmr.rs b/crates/rspack_core/src/compiler/hmr.rs index 0d16538b92f..abe00600203 100644 --- a/crates/rspack_core/src/compiler/hmr.rs +++ b/crates/rspack_core/src/compiler/hmr.rs @@ -61,7 +61,8 @@ impl Compiler { self .old_cache .set_modified_files(all_files.into_iter().collect()); - self.plugin_driver.resolver_factory.clear_cache(); + + self.plugin_driver.clear_cache(); let mut new_compilation = Compilation::new( self.options.clone(), diff --git a/crates/rspack_core/src/compiler/mod.rs b/crates/rspack_core/src/compiler/mod.rs index 57b8effe126..074e0f72e8d 100644 --- a/crates/rspack_core/src/compiler/mod.rs +++ b/crates/rspack_core/src/compiler/mod.rs @@ -143,7 +143,7 @@ impl Compiler { self.old_cache.end_idle(); // TODO: clear the outdated cache entries in resolver, // TODO: maybe it's better to use external entries. - self.plugin_driver.resolver_factory.clear_cache(); + self.plugin_driver.clear_cache(); let module_executor = ModuleExecutor::default(); fast_set( diff --git a/crates/rspack_core/src/plugin/mod.rs b/crates/rspack_core/src/plugin/mod.rs index 48603828a35..860aef72129 100644 --- a/crates/rspack_core/src/plugin/mod.rs +++ b/crates/rspack_core/src/plugin/mod.rs @@ -22,6 +22,8 @@ pub trait Plugin: fmt::Debug + Send + Sync { ) -> Result<()> { Ok(()) } + + fn clear_cache(&self) {} } pub type BoxPlugin = Box; diff --git a/crates/rspack_core/src/plugin/plugin_driver.rs b/crates/rspack_core/src/plugin/plugin_driver.rs index 2fff5e9dbf4..919bf01d312 100644 --- a/crates/rspack_core/src/plugin/plugin_driver.rs +++ b/crates/rspack_core/src/plugin/plugin_driver.rs @@ -76,4 +76,11 @@ impl PluginDriver { let mut diagnostic = self.diagnostics.lock().expect("TODO:"); std::mem::take(&mut diagnostic) } + + pub fn clear_cache(&self) { + self.resolver_factory.clear_cache(); + for plugin in &self.plugins { + plugin.clear_cache(); + } + } } diff --git a/packages/rspack-test-tools/tests/compilerCases/hooks-closure.js b/packages/rspack-test-tools/tests/compilerCases/hooks-closure.js new file mode 100644 index 00000000000..68e42a9f86c --- /dev/null +++ b/packages/rspack-test-tools/tests/compilerCases/hooks-closure.js @@ -0,0 +1,134 @@ +let globalId = 0; + +const buildModule = jest.fn(); +const succeedModule = jest.fn(); +const finishModules = jest.fn(); +const optimizeModules = jest.fn(); +const afterOptimizeModules = jest.fn(); +const optimizeTree = jest.fn(); +const optimizeChunkModules = jest.fn(); +const additionalTreeRuntimeRequirements = jest.fn(); +const runtimeModule = jest.fn(); +const chunkHash = jest.fn(); +const chunkAsset = jest.fn(); +const processAssets = jest.fn(); +const afterProcessAssets = jest.fn(); +const seal = jest.fn(); +const afterSeal = jest.fn(); + +class MyPlugin { + apply(compiler) { + compiler.hooks.compilation.tap("PLUGIN", compilation => { + const localId = globalId += 1; + + compilation.hooks.buildModule.tap("PLUGIN", () => { + buildModule(); + expect(localId).toBe(globalId); + }); + compilation.hooks.succeedModule.tap("PLUGIN", () => { + succeedModule(); + expect(localId).toBe(globalId); + }); + compilation.hooks.finishModules.tap("PLUGIN", () => { + finishModules(); + expect(localId).toBe(globalId); + }); + compilation.hooks.optimizeModules.tap("PLUGIN", () => { + optimizeModules(); + expect(localId).toBe(globalId); + }); + compilation.hooks.afterOptimizeModules.tap("PLUGIN", () => { + afterOptimizeModules(); + expect(localId).toBe(globalId); + }); + compilation.hooks.optimizeTree.tap("PLUGIN", () => { + optimizeTree(); + expect(localId).toBe(globalId); + }); + compilation.hooks.optimizeChunkModules.tap("PLUGIN", () => { + optimizeChunkModules(); + expect(localId).toBe(globalId); + }); + compilation.hooks.additionalTreeRuntimeRequirements.tap("PLUGIN", () => { + additionalTreeRuntimeRequirements(); + expect(localId).toBe(globalId); + }); + compilation.hooks.runtimeModule.tap("PLUGIN", () => { + runtimeModule(); + expect(localId).toBe(globalId); + }); + compilation.hooks.chunkHash.tap("PLUGIN", () => { + chunkHash(); + expect(localId).toBe(globalId); + }); + compilation.hooks.chunkAsset.tap("PLUGIN", () => { + chunkAsset(); + expect(localId).toBe(globalId); + }); + compilation.hooks.processAssets.tap("PLUGIN", () => { + processAssets(); + expect(localId).toBe(globalId); + }); + compilation.hooks.afterProcessAssets.tap("PLUGIN", () => { + afterProcessAssets(); + expect(localId).toBe(globalId); + }); + compilation.hooks.seal.tap("PLUGIN", () => { + seal(); + expect(localId).toBe(globalId); + }); + compilation.hooks.afterSeal.tap("PLUGIN", () => { + afterSeal(); + expect(localId).toBe(globalId); + }); + }); + } +} + +/** @type {import('../..').TCompilerCaseConfig} */ +module.exports = { + description: "The hooks should access the correct closure", + options(context) { + return { + context: context.getSource(), + entry: "./d", + plugins: [new MyPlugin()] + }; + }, + async build(_, compiler) { + await new Promise((resolve, reject) => { + compiler.run(err => { + if (err) { + reject(err); + return; + } + + compiler.run(err => { + if (err) { + reject(err); + return; + } + + resolve(); + }); + }); + }); + }, + async check() { + expect(buildModule.mock.calls.length).toBeGreaterThanOrEqual(2); + expect(succeedModule.mock.calls.length).toBeGreaterThanOrEqual(2); + expect(finishModules.mock.calls.length).toBeGreaterThanOrEqual(2); + expect(optimizeModules.mock.calls.length).toBeGreaterThanOrEqual(2); + expect(afterOptimizeModules.mock.calls.length).toBeGreaterThanOrEqual(2); + expect(optimizeTree.mock.calls.length).toBeGreaterThanOrEqual(2); + expect(optimizeChunkModules.mock.calls.length).toBeGreaterThanOrEqual(2); + expect(additionalTreeRuntimeRequirements.mock.calls.length).toBeGreaterThanOrEqual(2); + expect(runtimeModule.mock.calls.length).toBeGreaterThanOrEqual(2); + expect(chunkHash.mock.calls.length).toBeGreaterThanOrEqual(2); + expect(chunkAsset.mock.calls.length).toBeGreaterThanOrEqual(2); + expect(processAssets.mock.calls.length).toBeGreaterThanOrEqual(2); + expect(afterProcessAssets.mock.calls.length).toBeGreaterThanOrEqual(2); + expect(seal.mock.calls.length).toBeGreaterThanOrEqual(2); + expect(afterSeal.mock.calls.length).toBeGreaterThanOrEqual(2); + } +};