diff --git a/src/bun.js/bindings/JSBundlerPlugin.cpp b/src/bun.js/bindings/JSBundlerPlugin.cpp index 3f5cebdb84..260c6cffda 100644 --- a/src/bun.js/bindings/JSBundlerPlugin.cpp +++ b/src/bun.js/bindings/JSBundlerPlugin.cpp @@ -284,8 +284,11 @@ int BundlerPlugin::NativePluginList::call(JSC::VM& vm, int* shouldContinue, void { std::lock_guard lock(*group->at(i).second); if (group->at(i).first.match(path) > -1) { - ((OnBeforeParseArgs*) (onBeforeParseArgs))->external = callbacks[i].second->value(); - callbacks[i].first(1, onBeforeParseArgs, onBeforeParseResult); + Bun::NapiExternal* external = callbacks[i].second; + if (external) { + ((OnBeforeParseArgs*) (onBeforeParseArgs))->external = external->value(); + } + callbacks[i].first(onBeforeParseArgs, onBeforeParseResult); count++; } } @@ -306,6 +309,9 @@ JSC_DEFINE_HOST_FUNCTION(jsBundlerPluginFunction_onBeforeParse, (JSC::JSGlobalOb return JSC::JSValue::encode(JSC::jsUndefined()); } + // Clone the regexp so we don't have to worry about it being used concurrently with the JS thread. + // TODO: Should we have a regexp object for every thread in the thread pool? Then we could avoid using + // a mutex to synchronize access to the same regexp from multiple threads. JSC::RegExpObject* jsRegexp = jsCast(callFrame->argument(0)); RegExp* reggie = jsRegexp->regExp(); RegExp* newRegexp = RegExp::create(vm, reggie->pattern(), reggie->flags()); diff --git a/src/bun.js/bindings/JSBundlerPlugin.h b/src/bun.js/bindings/JSBundlerPlugin.h index 7e50e50d39..dad3f82a9f 100644 --- a/src/bun.js/bindings/JSBundlerPlugin.h +++ b/src/bun.js/bindings/JSBundlerPlugin.h @@ -10,7 +10,7 @@ typedef void (*JSBundlerPluginAddErrorCallback)(void*, void*, JSC::EncodedJSValue, JSC::EncodedJSValue); typedef void (*JSBundlerPluginOnLoadAsyncCallback)(void*, void*, JSC::EncodedJSValue, JSC::EncodedJSValue); typedef void (*JSBundlerPluginOnResolveAsyncCallback)(void*, void*, JSC::EncodedJSValue, JSC::EncodedJSValue, JSC::EncodedJSValue); -typedef void (*JSBundlerPluginNativeOnBeforeParseCallback)(int, void*, void*); +typedef void (*JSBundlerPluginNativeOnBeforeParseCallback)( void*, void*); namespace Bun { @@ -47,6 +47,7 @@ public: }; typedef struct { + size_t __struct_size; void *context; const char *path_ptr; size_t path_len; diff --git a/src/bundler/bundle_v2.zig b/src/bundler/bundle_v2.zig index 0897ef3a4b..29e72c866f 100644 --- a/src/bundler/bundle_v2.zig +++ b/src/bundler/bundle_v2.zig @@ -3818,6 +3818,7 @@ pub const ParseTask = struct { result: ?*OnBeforeParseResult = null, const OnBeforeParseArguments = extern struct { + struct_size: usize = @sizeOf(OnBeforeParseArguments), context: *OnBeforeParsePlugin, path_ptr: [*]const u8 = "", path_len: usize = 0, @@ -3828,6 +3829,7 @@ pub const ParseTask = struct { }; const BunLogOptions = extern struct { + struct_size: usize = @sizeOf(BunLogOptions), message_ptr: ?[*]const u8 = null, message_len: usize = 0, path_ptr: ?[*]const u8 = null, @@ -3906,6 +3908,7 @@ pub const ParseTask = struct { }; const OnBeforeParseResult = extern struct { + struct_size: usize = @sizeOf(OnBeforeParseResult), source_ptr: ?[*]const u8 = null, source_len: usize = 0, loader: Loader, diff --git a/test/js/bun/plugin/native-plugin.test.ts b/test/js/bun/plugin/native-plugin.test.ts index 5c51338ffb..c9d2c003b8 100644 --- a/test/js/bun/plugin/native-plugin.test.ts +++ b/test/js/bun/plugin/native-plugin.test.ts @@ -1,5 +1,5 @@ import { plugin } from "bun"; -import { afterEach, beforeAll, beforeEach, describe, expect, it, test } from "bun:test"; +import { afterEach, beforeAll, beforeEach, describe, expect, it } from "bun:test"; import path, { dirname, join, resolve } from "path"; import source from "./native_plugin.c" with { type: "file" }; import { bunEnv, bunExe, tempDirWithFiles } from "harness"; @@ -50,6 +50,8 @@ console.log(JSON.stringify(json));`, tempdir = tempDirWithFiles("native-plugins", files); outdir = path.join(tempdir, "dist"); + console.log("tempdir", tempdir); + process.chdir(tempdir); await Bun.$`${bunExe()} i && ${bunExe()} build:napi`.env(bunEnv).cwd(tempdir); @@ -60,7 +62,7 @@ console.log(JSON.stringify(json));`, process.chdir(cwd); }); - test("basic", async () => { + it("works in a basic case", async () => { await Bun.$`${bunExe()} i && ${bunExe()} build:napi`.env(bunEnv).cwd(tempdir); const result = await Bun.build({ @@ -94,7 +96,7 @@ console.log(JSON.stringify(json));`, expect(output).toStrictEqual({ fooCount: 8 }); }); - test("doesn't explode when there are a lot of concurrent files", async () => { + it("doesn't explode when there are a lot of concurrent files", async () => { // Generate 100 json files const files: [filepath: string, var_name: string][] = await Promise.all( Array.from({ length: 100 }, async (_, i) => { @@ -105,8 +107,8 @@ console.log(JSON.stringify(json));`, // Append the imports to index.ts const prelude = /* ts */ `import values from "./stuff.ts" -const many_foo = ["foo","foo","foo","foo","foo","foo","foo"] - `; + const many_foo = ["foo","foo","foo","foo","foo","foo","foo"] + `; await Bun.$`echo ${prelude} > index.ts`; await Bun.$`echo ${files.map(([fp]) => fp).join("\n")} >> index.ts`; await Bun.$`echo ${files.map(([, varname]) => `console.log(JSON.stringify(${varname}))`).join("\n")} >> index.ts`; @@ -147,4 +149,178 @@ const many_foo = ["foo","foo","foo","foo","foo","foo","foo"] expect(json).toStrictEqual({ fooCount: 8 }); } }); + + // We clone the RegExp object in the C++ code so this test ensures that there + // is no funny business regarding the filter regular expression and multiple + // threads + it("doesn't explode when there are a lot of concurrent files AND the filter regex is used on the JS thread", async () => { + const filter = /\.ts/; + // Generate 100 json files + const files: [filepath: string, var_name: string][] = await Promise.all( + Array.from({ length: 100 }, async (_, i) => { + await Bun.write(path.join(tempdir, "json_files", `lmao${i}.json`), `{}`); + return [`import json${i} from "./json_files/lmao${i}.json"`, `json${i}`]; + }), + ); + + // Append the imports to index.ts + const prelude = /* ts */ `import values from "./stuff.ts" +const many_foo = ["foo","foo","foo","foo","foo","foo","foo"] + `; + await Bun.$`echo ${prelude} > index.ts`; + await Bun.$`echo ${files.map(([fp]) => fp).join("\n")} >> index.ts`; + await Bun.$`echo ${files.map(([, varname]) => `console.log(JSON.stringify(${varname}))`).join("\n")} >> index.ts`; + + const resultPromise = Bun.build({ + outdir, + entrypoints: [path.join(tempdir, "index.ts")], + plugins: [ + { + name: "xXx123_foo_counter_321xXx", + setup(build) { + const napiModule = require(path.join(tempdir, "build/Release/xXx123_foo_counter_321xXx.node")); + const external = napiModule.createExternal(); + + build.onBeforeParse({ filter }, { napiModule, symbol: "plugin_impl", external }); + + build.onLoad({ filter: /\.json/ }, async ({ defer, path }) => { + await defer(); + const count = napiModule.getFooCount(external); + return { + contents: JSON.stringify({ fooCount: count }), + loader: "json", + }; + }); + }, + }, + ], + }); + + // Now saturate this thread with uses of the filter regex to test that nothing bad happens + // when the JS thread and the bundler thread use regexes concurrently + let dummy = 0; + for (let i = 0; i < 10000; i++) { + // Match the filter regex on some dummy string + dummy += filter.test("foo") ? 1 : 0; + } + + const result = await resultPromise; + + if (!result.success) console.log(result); + expect(result.success).toBeTrue(); + const output = await Bun.$`${bunExe()} run dist/index.js`.cwd(tempdir).text(); + const outputJsons = output + .trim() + .split("\n") + .map(s => JSON.parse(s)); + for (const json of outputJsons) { + expect(json).toStrictEqual({ fooCount: 8 }); + } + }); + + it("doesn't explode when passing invalid external", async () => { + const filter = /\.ts/; + // Generate 100 json files + const files: [filepath: string, var_name: string][] = await Promise.all( + Array.from({ length: 100 }, async (_, i) => { + await Bun.write(path.join(tempdir, "json_files", `lmao${i}.json`), `{}`); + return [`import json${i} from "./json_files/lmao${i}.json"`, `json${i}`]; + }), + ); + + // Append the imports to index.ts + const prelude = /* ts */ `import values from "./stuff.ts" +const many_foo = ["foo","foo","foo","foo","foo","foo","foo"] + `; + await Bun.$`echo ${prelude} > index.ts`; + await Bun.$`echo ${files.map(([fp]) => fp).join("\n")} >> index.ts`; + await Bun.$`echo ${files.map(([, varname]) => `console.log(JSON.stringify(${varname}))`).join("\n")} >> index.ts`; + + const resultPromise = Bun.build({ + outdir, + entrypoints: [path.join(tempdir, "index.ts")], + plugins: [ + { + name: "xXx123_foo_counter_321xXx", + setup(build) { + const napiModule = require(path.join(tempdir, "build/Release/xXx123_foo_counter_321xXx.node")); + const external = undefined; + + build.onBeforeParse({ filter }, { napiModule, symbol: "plugin_impl", external }); + + build.onLoad({ filter: /\.json/ }, async ({ defer, path }) => { + await defer(); + let count = 0; + try { + count = napiModule.getFooCount(external); + } catch (e) {} + return { + contents: JSON.stringify({ fooCount: count }), + loader: "json", + }; + }); + }, + }, + ], + }); + + const result = await resultPromise; + + if (!result.success) console.log(result); + expect(result.success).toBeTrue(); + const output = await Bun.$`${bunExe()} run dist/index.js`.cwd(tempdir).text(); + const outputJsons = output + .trim() + .split("\n") + .map(s => JSON.parse(s)); + for (const json of outputJsons) { + expect(json).toStrictEqual({ fooCount: 0 }); + } + }); + + it("works when logging an error", async () => { + const filter = /\.ts/; + + const prelude = /* ts */ `import values from "./stuff.ts" + const many_foo = ["foo","foo","foo","foo","foo","foo","foo"] + `; + await Bun.$`echo ${prelude} > index.ts`; + + const resultPromise = Bun.build({ + outdir, + entrypoints: [path.join(tempdir, "index.ts")], + plugins: [ + { + name: "xXx123_foo_counter_321xXx", + setup(build) { + const napiModule = require(path.join(tempdir, "build/Release/xXx123_foo_counter_321xXx.node")); + const external = napiModule.createExternal(); + napiModule.setThrowsErrors(external, true); + + build.onBeforeParse({ filter }, { napiModule, symbol: "plugin_impl", external }); + + build.onLoad({ filter: /\.json/ }, async ({ defer, path }) => { + await defer(); + let count = 0; + try { + count = napiModule.getFooCount(external); + } catch (e) {} + return { + contents: JSON.stringify({ fooCount: count }), + loader: "json", + }; + }); + }, + }, + ], + }); + + const result = await resultPromise; + + if (result.success) console.log(result); + expect(result.success).toBeFalse(); + const log = result.logs[0]; + expect(log.message).toContain("Throwing an error"); + expect(log.level).toBe("error"); + }); }); diff --git a/test/js/bun/plugin/native_plugin.c b/test/js/bun/plugin/native_plugin.c index c9b026b1de..f9b0f6b407 100644 --- a/test/js/bun/plugin/native_plugin.c +++ b/test/js/bun/plugin/native_plugin.c @@ -12,9 +12,12 @@ typedef struct { atomic_size_t foo_count; + // For testing logging error logic + atomic_bool throws_an_error; } External; typedef struct { + size_t __struct_size; void* bun; const uint8_t* path_ptr; size_t path_len; @@ -27,6 +30,7 @@ typedef struct { typedef struct BunLogOptions BunLogOptions; typedef struct OnBeforeParseResult { + size_t __struct_size; uint8_t* source_ptr; size_t source_len; uint8_t loader; @@ -39,7 +43,61 @@ typedef struct OnBeforeParseResult { void (*log)(const OnBeforeParseArguments* args, BunLogOptions* options); } OnBeforeParseResult; -void plugin_impl(int version, const OnBeforeParseArguments* args, OnBeforeParseResult* result) { +typedef struct BunLogOptions { + size_t __struct_size; + const uint8_t* message_ptr; + size_t message_len; + const uint8_t* path_ptr; + size_t path_len; + const uint8_t* source_line_text_ptr; + size_t source_line_text_len; + int8_t level; + int line; + int lineEnd; + int column; + int columnEnd; +} BunLogOptions; + +typedef enum { + BUN_LOG_LEVEL_VERBOSE = 0, + BUN_LOG_LEVEL_DEBUG = 1, + BUN_LOG_LEVEL_INFO = 2, + BUN_LOG_LEVEL_WARN = 3, + BUN_LOG_LEVEL_ERROR = 4, +} BunLogLevel; + + +void log_error(const OnBeforeParseArguments* args, const OnBeforeParseResult* result, BunLogLevel level, const char* message, size_t message_len) { + BunLogOptions options = (BunLogOptions) { + .message_ptr = (uint8_t*)message, + .message_len = message_len, + .path_ptr = args->path_ptr, + .path_len = args->path_len, + .source_line_text_ptr = NULL, + .source_line_text_len = 0, + .level = level, + .line = 0, + .lineEnd = 0, + .column = 0, + .columnEnd = 0, + }; + (result->log)(args, &options); +} + +void plugin_impl(const OnBeforeParseArguments* args, OnBeforeParseResult* result) { + // if (args->__struct_size < sizeof(OnBeforeParseArguments)) { + // log_error(args, result, BUN_LOG_LEVEL_ERROR, "Invalid OnBeforeParseArguments struct size", sizeof("Invalid OnBeforeParseArguments struct size") - 1); + // return; + // } + + if (args->external) { + External* external = (External*)args->external; + if (atomic_load(&external->throws_an_error)) { + log_error(args, result, BUN_LOG_LEVEL_ERROR, "Throwing an error", sizeof("Throwing an error") - 1); + return; + } + } + int fetch_result = result->fetchSourceCode(args, result); if (fetch_result != 0) { printf("FUCK\n"); @@ -74,8 +132,10 @@ void plugin_impl(int version, const OnBeforeParseArguments* args, OnBeforeParseR cursor = strnstr((const char*) cursor, "foo", (size_t) (end - cursor)); } else break; } - External *external = (External*)args->external; - atomic_fetch_add(&external->foo_count, foo_count); + if (args->external) { + External *external = (External*)args->external; + atomic_fetch_add(&external->foo_count, foo_count); + } result->source_ptr = (uint8_t*)new_source; result->source_len = result->source_len; } else { @@ -87,7 +147,6 @@ void plugin_impl(int version, const OnBeforeParseArguments* args, OnBeforeParseR } void finalizer(napi_env env, void* data, void* hint) { - printf("FREEING EXTERNAL!\n"); External* external = (External*)data; if (external != NULL) { free(external); @@ -118,6 +177,41 @@ napi_value create_external(napi_env env, napi_callback_info info) { return result; } +napi_value set_throws_errors(napi_env env, napi_callback_info info) { + napi_status status; + External* external; + + size_t argc = 1; + napi_value args[1]; + status = napi_get_cb_info(env, info, &argc, args, NULL, NULL); + if (status != napi_ok) { + napi_throw_error(env, NULL, "Failed to parse arguments"); + return NULL; + } + + if (argc < 1) { + napi_throw_error(env, NULL, "Wrong number of arguments"); + return NULL; + } + + status = napi_get_value_external(env, args[0], (void**)&external); + if (status != napi_ok) { + napi_throw_error(env, NULL, "Failed to get external"); + return NULL; + } + + bool throws; + status = napi_get_value_bool(env, args[0], &throws); + if (status != napi_ok) { + napi_throw_error(env, NULL, "Failed to get boolean value"); + return NULL; + } + + atomic_store(&external->throws_an_error, throws); + + return NULL; +} + napi_value get_foo_count(napi_env env, napi_callback_info info) { napi_status status; External* external; @@ -162,6 +256,7 @@ napi_value Init(napi_env env, napi_value exports) { napi_status status; napi_value fn_get_names; napi_value fn_create_external; + napi_value fn_set_throws_errors; // Register get_names function status = napi_create_function(env, NULL, 0, get_foo_count, NULL, &fn_get_names); @@ -175,6 +270,18 @@ napi_value Init(napi_env env, napi_value exports) { return NULL; } + // Register set_throws_errors function + status = napi_create_function(env, NULL, 0, set_throws_errors, NULL, &fn_set_throws_errors); + if (status != napi_ok) { + napi_throw_error(env, NULL, "Failed to create set_throws_errors function"); + return NULL; + } + status = napi_set_named_property(env, exports, "setThrowsErrors", fn_set_throws_errors); + if (status != napi_ok) { + napi_throw_error(env, NULL, "Failed to add set_throws_errors function to exports"); + return NULL; + } + // Register create_external function status = napi_create_function(env, NULL, 0, create_external, NULL, &fn_create_external); if (status != napi_ok) {