diff --git a/src/bundler/BundleThread.rs b/src/bundler/BundleThread.rs index cef2a0487354..dede892e9482 100644 --- a/src/bundler/BundleThread.rs +++ b/src/bundler/BundleThread.rs @@ -78,9 +78,8 @@ pub trait CompletionStruct: Node + Send + 'static { /// `FileMap` layout stays in T6. fn file_map(&mut self) -> Option>; /// Returns a §Dispatch handle (erased owner + `&'static` vtable) the impl - /// provides, so the bundler can read `result == .err` / `is_cancelled`, - /// and post plugin hops to the owning VM, without naming the concrete - /// struct. + /// provides, so the bundler can read `is_cancelled` and post plugin hops + /// to the owning VM, without naming the concrete struct. fn as_js_bundle_completion_task(&mut self) -> dispatch::CompletionHandle; /// `Transpiler<'a>` has borrow-carrying fields (`arena: &'a Arena`, diff --git a/src/bundler/DeferredBatchTask.rs b/src/bundler/DeferredBatchTask.rs index e2920d2d0550..f56fc385cefc 100644 --- a/src/bundler/DeferredBatchTask.rs +++ b/src/bundler/DeferredBatchTask.rs @@ -5,80 +5,37 @@ //! for every onLoad callback which called `.defer()`. use crate::BundleV2; +use crate::bundle_v2::JSBundlerPlugin; // Task is `(tag: u8, ptr: *mut ())` owned by bun_event_loop; // runtime owns the match-loop. See PORTING.md §Dispatch. use bun_event_loop::ConcurrentTask::ConcurrentTask; use bun_event_loop::{Task, task_tag}; +use core::ptr::NonNull; -#[derive(Default)] +/// One per drain, allocated from the pass's arena like `Resolve` / `Load`. pub struct DeferredBatchTask { - // Debug-only flag; zero-sized in release. - #[cfg(debug_assertions)] - running: bool, + /// `BundleV2::plugins` as of `schedule`. + plugins: Option>, } impl bun_event_loop::Taskable for DeferredBatchTask { const TAG: bun_event_loop::TaskTag = task_tag::BundleV2DeferredBatchTask; - /// Embedded in its `BundleV2`, which outlives the queue entry and owns - /// everything the drain would have touched; nothing to free. + /// As `Resolve`: arena-owned by its (cancelled) bundle pass; nothing to free. unsafe fn release_unrun(_: *mut Self) {} } impl DeferredBatchTask { - pub(crate) fn init(&mut self) { - // Kept as `&mut self` (not `-> Self`) — this struct is embedded - // by value in BundleV2 (recovered via container_of in `get_bundle_v2`), so - // it is reset in place, never separately constructed. - #[cfg(debug_assertions)] - debug_assert!(!self.running); - // No Drop / no owned fields — pure reset. - let _ = core::mem::take(self); + /// Bundle thread; `arena` is the pass's `graph.heap`. + pub(crate) fn schedule(bv2: &mut BundleV2<'_>, arena: &bun_alloc::Arena) { + let this = arena.alloc(Self { + plugins: bv2.plugins, + }); + let task = ConcurrentTask::create(Task::init(std::ptr::from_mut::(this))); + bv2.enqueue_on_js_loop_for_plugins(task); } - pub(crate) fn get_bundle_v2(&mut self) -> &mut BundleV2<'static> { - // SAFETY: `self` is always the `drain_defer_task` field of a live `BundleV2`; - // this struct is never instantiated standalone. Lifetime erased to 'static; - // callers must not outlive the owning bundle. - unsafe { - &mut *bun_core::from_field_ptr!( - BundleV2<'static>, - drain_defer_task, - std::ptr::from_mut::(self) - ) - } - } - - pub(crate) fn schedule(&mut self) { - #[cfg(debug_assertions)] - { - debug_assert!(!self.running); - self.running = false; - } - let task = ConcurrentTask::create(Task::init(std::ptr::from_mut::(self))); - - self.get_bundle_v2().enqueue_on_js_loop_for_plugins(task); - } - - pub fn run_on_js_thread(&mut self) { - // `deinit` only resets - // the debug `running` flag; nothing follows `drain_deferred`, so - // resetting the flag afterwards covers both paths. - { - let bv2 = self.get_bundle_v2(); - let rejected = bv2.completion.map(|c| c.result_is_err()).unwrap_or(false); - // The void result is discarded — see - // `Plugin::drain_deferred` for the exception-scope note. - bv2.plugins_mut().expect("plugins").drain_deferred(rejected); - } - self.deinit(); - } - - // Not `impl Drop` — this struct is an intrusive field of `BundleV2` - // and `deinit` is a debug-flag reset, not resource teardown. - fn deinit(&mut self) { - #[cfg(debug_assertions)] - { - self.running = false; - } + /// Plugins' JS thread. + pub fn run_on_js_thread(&self) { + JSBundlerPlugin::opaque_mut(self.plugins.expect("plugins").as_ptr()).drain_deferred(); } } diff --git a/src/bundler/Graph.rs b/src/bundler/Graph.rs index aa13706b0826..b30f81841956 100644 --- a/src/bundler/Graph.rs +++ b/src/bundler/Graph.rs @@ -5,6 +5,7 @@ use bun_ast::server_component_boundary; use bun_collections::MultiArrayList; use enum_map::EnumMap; +use crate::DeferredBatchTask::DeferredBatchTask; use crate::IndexStringMap::IndexStringMap; use crate::PathToSourceIndexMap::PathToSourceIndexMap; use crate::options; @@ -243,8 +244,7 @@ impl<'a> Graph<'a> { } } - transpiler.drain_defer_task.init(); - transpiler.drain_defer_task.schedule(); + DeferredBatchTask::schedule(transpiler, self.heap); return true; } diff --git a/src/bundler/bundle_v2.rs b/src/bundler/bundle_v2.rs index 0af0e0ce2659..64ad15c8ddd2 100644 --- a/src/bundler/bundle_v2.rs +++ b/src/bundler/bundle_v2.rs @@ -30,7 +30,6 @@ pub use bv2_impl::{ OnDependenciesAnalyze, singleton, }; -pub use crate::DeferredBatchTask::DeferredBatchTask; use crate::Graph::Graph; use crate::PathToSourceIndexMap::PathToSourceIndexMap; use crate::barrel_imports::RequestedExports; @@ -118,8 +117,6 @@ pub struct BundleV2<'a> { pub(crate) finalizers: Vec, - pub(crate) drain_defer_task: DeferredBatchTask, - /// Set true by DevServer. Currently every usage of the transpiler (Bun.build /// and `bun build` CLI) runs at the top of an event loop. When this is true, /// a callback is executed after all work is complete (`finishFromBakeDevServer`). @@ -205,15 +202,6 @@ impl<'a> BundleV2<'a> { self.plugins.map(|p| unsafe { p.as_ref() }) } - /// Mutable projection of the `plugins` backref for FFI calls that take - /// `*mut` (`drain_deferred`). The pointee is disjoint from `self` storage. - #[inline] - pub(crate) fn plugins_mut(&mut self) -> Option<&mut JSBundlerPlugin> { - // SAFETY: BACKREF — see `plugins_ref`. `&mut self` ensures no other - // `&JSBundlerPlugin` projection from this `BundleV2` overlaps. - self.plugins.map(|mut p| unsafe { p.as_mut() }) - } - /// Mutable projection of the `bun_watcher` backref for `Watcher::add_file`. /// Centralises the two open-coded `unsafe { ptr.as_mut() }` sites so the /// liveness/exclusivity argument lives in one place. @@ -738,7 +726,7 @@ pub mod bv2_impl { kind: u8, ); #[link_name = "JSBundlerPlugin__drainDeferred"] - safe fn JSBundlerPlugin__drainDeferred(this: &mut Plugin, rejected: bool); + safe fn JSBundlerPlugin__drainDeferred(this: &mut Plugin); #[link_name = "JSBundlerPlugin__hasOnBeforeParsePlugins"] safe fn JSBundlerPlugin__hasOnBeforeParsePlugins(this: &Plugin) -> i32; // `ctx`/`args`/`result` are opaque cookies the C++ side round-trips @@ -765,8 +753,8 @@ pub mod bv2_impl { /// only bundler caller (`DeferredBatchTask::run_on_js_thread`) /// ignores failures, so the void FFI call is the observable /// behaviour at this tier. - pub(crate) fn drain_deferred(&mut self, rejected: bool) { - JSBundlerPlugin__drainDeferred(self, rejected) + pub(crate) fn drain_deferred(&mut self) { + JSBundlerPlugin__drainDeferred(self) } #[inline] @@ -1083,7 +1071,10 @@ pub mod bv2_impl { /// are the real lower-tier `bun_event_loop` types, so `dispatch()` / /// `run_on_js_thread()` are implemented inherently (no T6 hook). pub struct Resolve { + /// Owning loop only (`dispatch()`, the answer); `run_on_js_thread` uses `plugins`. pub bv2: *mut BundleV2<'static>, + /// `BundleV2::plugins` as of `init`. + pub(crate) plugins: Option>, pub import_record: MiniImportRecord, pub value: ResolveValue, /// `jsc.AnyEventLoop.Task` — intrusive node for the Mini-loop queue. @@ -1096,6 +1087,7 @@ pub mod bv2_impl { fn default() -> Self { Self { bv2: core::ptr::null_mut(), + plugins: None, import_record: MiniImportRecord::default(), value: ResolveValue::Pending, task: bun_event_loop::AnyTaskWithExtraContext::AnyTaskWithExtraContext::default(), @@ -1116,6 +1108,7 @@ pub mod bv2_impl { // SAFETY: lifetime erased — Resolve is owned by the dispatch // chain and never outlives `bv2`. bv2: std::ptr::from_mut::>(bv2).cast::>(), + plugins: bv2.plugins, import_record: record, value: ResolveValue::Pending, task: bun_event_loop::AnyTaskWithExtraContext::AnyTaskWithExtraContext::default(), @@ -1143,25 +1136,19 @@ pub mod bv2_impl { bv2.enqueue_on_js_loop_for_plugins(task); } } + /// Plugins' JS thread. pub fn run_on_js_thread(&mut self) { let kind = self.import_record.kind; // reshaped for borrowck — capture the erased self // pointer before borrowing fields immutably for the FFI call. let self_ptr = std::ptr::from_mut::(self).cast::(); - // SAFETY: `bv2` is a valid backref set by `init`; the plugin - // storage is disjoint from `self`, so the `&mut JSBundlerPlugin` - // returned by `plugins_mut()` does not alias the - // `&self.import_record.*` borrows below. - unsafe { &mut *self.bv2 } - .plugins_mut() - .expect("plugins") - .match_on_resolve( - &self.import_record.specifier, - &self.import_record.namespace, - &self.import_record.source_file, - self_ptr, - kind, - ); + Plugin::opaque_mut(self.plugins.expect("plugins").as_ptr()).match_on_resolve( + &self.import_record.specifier, + &self.import_record.namespace, + &self.import_record.source_file, + self_ptr, + kind, + ); } } @@ -1190,7 +1177,10 @@ pub mod bv2_impl { /// Task driving an onLoad plugin invocation for one source file. pub struct Load { + /// See `Resolve::bv2`. pub bv2: *mut BundleV2<'static>, + /// `BundleV2::plugins` as of `init`. + pub(crate) plugins: Option>, pub(crate) source_index: bun_ast::Index, pub(crate) default_loader: Loader, pub path: Box<[u8]>, @@ -1219,6 +1209,7 @@ pub mod bv2_impl { .unwrap_or(Loader::Js); Self { bv2: std::ptr::from_mut::>(bv2).cast::>(), + plugins: bv2.plugins, parse_task: bun_ptr::BackRef::new_mut(parse), source_index: parse.source_index, default_loader, @@ -1278,26 +1269,20 @@ pub mod bv2_impl { bv2.enqueue_on_js_loop_for_plugins(concurrent_task); } } + /// Plugins' JS thread. pub fn run_on_js_thread(&mut self) { let is_server_side = self.bake_graph() != crate::bake_types::Graph::Client; let default_loader = self.default_loader; // reshaped for borrowck — capture the erased self // pointer before borrowing fields immutably for the FFI call. let self_ptr = std::ptr::from_mut::(self).cast::(); - // SAFETY: `bv2` is a valid backref set by `init`; the plugin - // storage is disjoint from `self`, so the `&mut JSBundlerPlugin` - // returned by `plugins_mut()` does not alias the - // `&self.path` / `&self.namespace` borrows below. - unsafe { &mut *self.bv2 } - .plugins_mut() - .expect("plugins") - .match_on_load( - &self.path, - &self.namespace, - self_ptr, - default_loader, - is_server_side, - ); + Plugin::opaque_mut(self.plugins.expect("plugins").as_ptr()).match_on_load( + &self.path, + &self.namespace, + self_ptr, + default_loader, + is_server_side, + ); } } impl bun_event_loop::Taskable for Load { @@ -1321,7 +1306,6 @@ pub mod bv2_impl { use bun_sourcemap as SourceMap; use crate::AstBuilder::AstBuilder; - use crate::DeferredBatchTask::DeferredBatchTask; use crate::Graph::Graph; use crate::LinkerContext; use crate::PathToSourceIndexMap::PathToSourceIndexMap; @@ -1420,13 +1404,11 @@ pub mod bv2_impl { /// CYCLEBREAK GENUINE: `JSBundleCompletionTask` — the /// concrete struct lives in `bun_runtime` (its fields name `Config`/ - /// `Plugin`/`HTMLBundle::Route`). The bundler reads exactly two things - /// from it (whether the result is an error, and the concurrent-task + /// `Plugin`/`HTMLBundle::Route`). The bundler needs exactly two things + /// from it (whether it was cancelled, and the concurrent-task /// enqueue), so the high tier hands the bundler an erased owner + /// `&'static` vtable pair (same shape as [`DevServerHandle`]). pub struct CompletionDispatch { - /// Whether the completion result is an error. - pub result_is_err: unsafe fn(core::ptr::NonNull) -> bool, /// Whether the VM that owns the plugins is shutting down: stop /// waiting for their answers and fail the build (any thread). pub is_cancelled: unsafe fn(core::ptr::NonNull) -> bool, @@ -1443,22 +1425,16 @@ pub mod bv2_impl { pub vtable: &'static CompletionDispatch, } // SAFETY: erased `*mut JSBundleCompletionTask` backref — set by the JS - // thread, read by the bundle thread; `enqueue_task_concurrent` is the only - // cross-thread call and it goes through `jsc::EventLoop`'s lock-free queue. + // thread, read by the bundle thread; the two cross-thread calls are an + // atomic load (`is_cancelled`) and a push onto `jsc::EventLoop`'s + // lock-free queue (`enqueue_task_concurrent`). unsafe impl Send for CompletionHandle {} // Intentionally not `Sync`: the opaque owner (`JSBundleCompletionTask`) - // is modeled as `!Sync`, and this wrapper exposes `result_is_err(&self)` - // in addition to the lock-free enqueue path, so blanket `&CompletionHandle` - // sharing across threads is not justified. The handle only needs to *move* - // to the bundle thread (`Send`), not be shared. If a cross-thread `&` ever + // is modeled as `!Sync`, and the handle only needs to *move* to the + // bundle thread (`Send`), not be shared. If a cross-thread `&` ever // becomes necessary, split out an enqueue-only wrapper and make only that // type `Sync`. impl CompletionHandle { - #[inline] - pub(crate) fn result_is_err(&self) -> bool { - // SAFETY: vtable contract. - unsafe { (self.vtable.result_is_err)(self.owner) } - } #[inline] pub(crate) fn is_cancelled(&self) -> bool { // SAFETY: vtable contract. @@ -2857,7 +2833,6 @@ pub mod bv2_impl { unique_key: 0, dynamic_import_entry_points: ArrayHashMap::new(), finalizers: Vec::new(), - drain_defer_task: DeferredBatchTask::default(), asynchronous: false, has_any_top_level_await_modules: false, requested_exports: Vec::new(), diff --git a/src/jsc/bindings/JSBundlerPlugin.cpp b/src/jsc/bindings/JSBundlerPlugin.cpp index 8c87b28467d3..40e12025203e 100644 --- a/src/jsc/bindings/JSBundlerPlugin.cpp +++ b/src/jsc/bindings/JSBundlerPlugin.cpp @@ -665,7 +665,7 @@ extern "C" void JSBundlerPlugin__setConfig(Bun::JSBundlerPlugin* plugin, void* c plugin->plugin.config = config; } -extern "C" void JSBundlerPlugin__drainDeferred(Bun::JSBundlerPlugin* pluginObject, bool rejected) +extern "C" void JSBundlerPlugin__drainDeferred(Bun::JSBundlerPlugin* pluginObject) { auto* globalObject = pluginObject->globalObject(); MarkedArgumentBuffer arguments; @@ -676,11 +676,7 @@ extern "C" void JSBundlerPlugin__drainDeferred(Bun::JSBundlerPlugin* pluginObjec auto scope = DECLARE_THROW_SCOPE(vm); for (auto promiseValue : arguments) { JSPromise* promise = uncheckedDowncast(promiseValue); - if (rejected) { - promise->reject(vm, JSC::jsUndefined()); - } else { - promise->resolve(globalObject, vm, JSC::jsUndefined()); - } + promise->resolve(globalObject, vm, JSC::jsUndefined()); RETURN_IF_EXCEPTION(scope, ); } RETURN_IF_EXCEPTION(scope, ); diff --git a/src/runtime/api/js_bundle_completion_task.rs b/src/runtime/api/js_bundle_completion_task.rs index 27657f0da198..2323190b6399 100644 --- a/src/runtime/api/js_bundle_completion_task.rs +++ b/src/runtime/api/js_bundle_completion_task.rs @@ -855,7 +855,6 @@ fn from_completion_handle<'a>(c: NonNull) -> &'a JSBundleCo } static COMPLETION_VTABLE: dispatch::CompletionDispatch = dispatch::CompletionDispatch { - result_is_err: |c| matches!(from_completion_handle(c).result, BundleV2Result::Err(_)), is_cancelled: |c| { from_completion_handle(c) .cancelled diff --git a/test/internal/source-lints/bundler-plugin-hops-no-pass.test.ts b/test/internal/source-lints/bundler-plugin-hops-no-pass.test.ts new file mode 100644 index 000000000000..924adc061366 --- /dev/null +++ b/test/internal/source-lints/bundler-plugin-hops-no-pass.test.ts @@ -0,0 +1,115 @@ +import { file } from "bun"; +import { expect, test } from "bun:test"; +import { realpathSync } from "fs"; +import path from "path"; +import { globAllSources } from "../../../scripts/glob-sources.ts"; + +// The bundler's plugin hops must not touch the bundle pass that posted them. +// +// `Resolve::run_on_js_thread`, `Load::run_on_js_thread` (src/bundler/bundle_v2.rs) +// and `DeferredBatchTask::run_on_js_thread` are the tasks a `BundleV2` posts to +// the plugins' JS thread. For `Bun.build` the pass stays on the bundle thread, +// which is inside `wait_for_parse` holding `&mut BundleV2` and mutating it while +// the hop runs, so everything a hop needs (the plugin handle, the import record, +// the path) is copied into the hop when it is built, and its body reads only its +// own fields. This tree used to have all three reach back into the pass +// (`&mut *self.bv2` / a `from_field_ptr!` walk-back, then `plugins_mut()`), i.e. +// form a second `&mut` to a struct another thread was writing through its own. +// +// A hop body therefore may not mention any route back to the pass: the `bv2` +// backref (kept for `dispatch()` and the answer, which run on the pass's own +// loop), `BundleV2` itself, the parse task's `ctx` backref, a `from_field_ptr!` / +// `container_of` walk-back, an accessor named after the pass, or the pass's +// `plugins_ref()` / `plugins_mut()` projections. Something new a hop needs gets +// copied in at construction like the rest. +// +// Out of scope: the answer thunks in src/runtime/api/JSBundler.rs, which post +// back to the pass's loop and are their own population. + +const root = path.resolve(import.meta.dir, "..", "..", ".."); +const bundlerSources = globAllSources().rust.filter( + p => p.endsWith(".rs") && path.relative(root, p).replaceAll(path.sep, "/").startsWith("src/bundler/"), +); + +// Only scan files tracked in HEAD (a `git stash` round-trip can leave stray +// `.rs` files in the working tree; CI runs on a clean checkout). Same guard as +// dead-code-escapes.test.ts. +const tracked: Set | null = (() => { + const r = Bun.spawnSync({ + cmd: ["git", "-C", root, "ls-tree", "-r", "--name-only", "-z", "HEAD"], + stdout: "pipe", + stderr: "ignore", + }); + if (!r.success) return null; + return new Set(r.stdout.toString().split("\0").filter(Boolean)); +})(); + +const HOP = /\bfn\s+run_on_js_thread\b/g; +const ROUTE_TO_PASS = + /\bBundleV2\b|\b\w*(?:bv2|bundle_v2)\w*\b|\bfrom_field_ptr\b|\bcontainer_of\b|\bctx\b|\bplugins_(?:ref|mut)\b/g; + +function lineOf(text: string, index: number): number { + return text.slice(0, index).split("\n").length; +} + +/** The `{ .. }` block starting at the first `{` at or after `from`. */ +function blockAfter(text: string, from: number): { start: number; body: string } | null { + const start = text.indexOf("{", from); + if (start < 0) return null; + let depth = 0; + for (let i = start; i < text.length; i++) { + const c = text[i]; + if (c === "{") depth++; + else if (c === "}" && --depth === 0) return { start, body: text.slice(start, i + 1) }; + } + return null; +} + +const hops: string[] = []; +const hits: { source: string; line: number; text: string }[] = []; +let scanned = 0; +for (const abs of bundlerSources) { + const source = path.relative(root, abs).replaceAll(path.sep, "/"); + if (path.relative(root, realpathSync(abs)).replaceAll(path.sep, "/") !== source) continue; + if (tracked !== null && !tracked.has(source)) continue; + scanned++; + const content = await file(abs).text(); + // Strip comments (full-line ones first, keeping the newline so line numbers + // hold; then trailing ones) so prose does not count. The hop bodies contain + // no braces or `//` inside string literals, which is what this would misread. + const stripped = content.replace(/^[ \t]*\/\/.*$/gm, "").replace(/[ \t]\/\/.*$/gm, ""); + for (const m of stripped.matchAll(HOP)) { + hops.push(source); + const block = blockAfter(stripped, m.index + m[0].length); + if (block === null) { + hits.push({ source, line: lineOf(stripped, m.index), text: "could not find the body of run_on_js_thread" }); + continue; + } + for (const hit of block.body.matchAll(ROUTE_TO_PASS)) { + hits.push({ source, line: lineOf(stripped, block.start + hit.index), text: hit[0] }); + } + } +} +const offenders = hits + .sort((a, b) => a.source.localeCompare(b.source) || a.line - b.line) + .map(h => `${h.source}:${h.line}: ${h.text}`); + +test("scans the tracked sources of the bundler crate", () => { + // Guards against the tracked/realpath filters above over-firing and leaving + // nothing to scan, which would make the assertions below pass vacuously. + expect(scanned).toBeGreaterThan(0); +}); + +test("the three plugin hops are still where this lint looks for them", () => { + // If this changes, a hop was added, removed or moved: update this list (and + // the header), not the ban below. + expect(hops.sort()).toEqual([ + "src/bundler/DeferredBatchTask.rs", + "src/bundler/bundle_v2.rs", + "src/bundler/bundle_v2.rs", + ]); +}); + +test("plugin hop bodies do not reach back into the pass that posted them", () => { + expect(offenders).toEqual([]); +});