From 6bc62c6cfbfef42202bb6a99ad696898b63dd776 Mon Sep 17 00:00:00 2001 From: dave caruso Date: Fri, 2 Feb 2024 00:18:28 -0800 Subject: [PATCH] fix(windows): initialize uv allocators earlier (#8631) * uv loop is thread local * hi * stuff so far * [autofix.ci] apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Jarred Sumner --- src/bun.js/event_loop.zig | 7 ------- src/bun.js/web_worker.zig | 7 ++++++- src/deps/libuv.zig | 27 ++++++++------------------- src/main.zig | 9 +++++++++ test/js/web/workers/worker.test.ts | 12 +++++++----- 5 files changed, 30 insertions(+), 32 deletions(-) diff --git a/src/bun.js/event_loop.zig b/src/bun.js/event_loop.zig index 04308920282dd3..c24fa3484a9c3c 100644 --- a/src/bun.js/event_loop.zig +++ b/src/bun.js/event_loop.zig @@ -1344,13 +1344,6 @@ pub const EventLoop = struct { if (comptime Environment.isWindows) { this.uws_loop = bun.uws.Loop.init(); this.virtual_machine.event_loop_handle = Async.Loop.get(); - - _ = bun.windows.libuv.uv_replace_allocator( - @ptrCast(&bun.Mimalloc.mi_malloc), - @ptrCast(&bun.Mimalloc.mi_realloc), - @ptrCast(&bun.Mimalloc.mi_calloc), - @ptrCast(&bun.Mimalloc.mi_free), - ); } else { this.virtual_machine.event_loop_handle = bun.Async.Loop.get(); } diff --git a/src/bun.js/web_worker.zig b/src/bun.js/web_worker.zig index e2e3908d4cea7b..420ec2aa296e72 100644 --- a/src/bun.js/web_worker.zig +++ b/src/bun.js/web_worker.zig @@ -259,6 +259,8 @@ pub const WebWorker = struct { } fn spin(this: *WebWorker) void { + log("[{d}] spin start", .{this.execution_context_id}); + var vm = this.vm.?; std.debug.assert(this.status.load(.Acquire) == .start); this.setStatus(.starting); @@ -280,7 +282,7 @@ pub const WebWorker = struct { _ = promise.result(vm.global.vm()); this.flushLogs(); - JSC.markBinding(@src()); + log("[{d}] event loop start", .{this.execution_context_id}); WebWorker__dispatchOnline(this.cpp_worker, vm.global); this.setStatus(.running); @@ -303,6 +305,8 @@ pub const WebWorker = struct { if (this.requested_terminate) break; } + log("[{d}] before exit {s}", .{ this.execution_context_id, if (this.requested_terminate) "(terminated)" else "(event loop dead)" }); + // Only call "beforeExit" if we weren't from a .terminate if (!this.requested_terminate) { // TODO: is this able to allow the event loop to continue? @@ -311,6 +315,7 @@ pub const WebWorker = struct { this.flushLogs(); this.exitAndDeinit(); + log("[{d}] spin done", .{this.execution_context_id}); } /// This is worker.ref()/.unref() from JS (Caller thread) diff --git a/src/deps/libuv.zig b/src/deps/libuv.zig index 2267766fedd344..15ddce606e8c39 100644 --- a/src/deps/libuv.zig +++ b/src/deps/libuv.zig @@ -616,26 +616,16 @@ pub const Loop = extern struct { bun.default_allocator.destroy(ptr); } - // threadlocal var threadlocal_loop_data: Loop = undefined; - // threadlocal var threadlocal_loop: ?*Loop = null; + threadlocal var threadlocal_loop_data: Loop = undefined; + threadlocal var threadlocal_loop: ?*Loop = null; - /// UV loop is not thread local. pub fn get() *Loop { - // TODO(@paperdave): - // This should not work. UV is not threadsafe. Repeat, UV is NOT THREADSAFE. - // but... this on average seems to be more stable than having a threadlocal loop ._. - // really, the solution is to fix many other places like node_fs to not use - // the `bun.sys.sys_uv` wrapper api, as i think there is issue doing these - // cross-thread sync calls. - return uv_default_loop(); - - // the correct code looks more like?: - // if (threadlocal_loop) |loop| return loop; - // if (bun.windows.libuv.Loop.init(&threadlocal_loop_data)) |e| { - // std.debug.panic("Failed to initialize libuv loop: {s}", .{@tagName(e)}); - // } - // threadlocal_loop = &threadlocal_loop_data; - // return &threadlocal_loop_data; + if (threadlocal_loop) |loop| return loop; + if (bun.windows.libuv.Loop.init(&threadlocal_loop_data)) |e| { + std.debug.panic("Failed to initialize libuv loop: {s}", .{@tagName(e)}); + } + threadlocal_loop = &threadlocal_loop_data; + return &threadlocal_loop_data; } pub fn tick(this: *Loop) void { @@ -1802,7 +1792,6 @@ pub const uv_calloc_func = ?*const fn (usize, usize) callconv(.C) ?*anyopaque; pub const uv_free_func = ?*const fn (?*anyopaque) callconv(.C) void; pub extern fn uv_library_shutdown() void; pub extern fn uv_replace_allocator(malloc_func: uv_malloc_func, realloc_func: uv_realloc_func, calloc_func: uv_calloc_func, free_func: uv_free_func) c_int; -pub extern fn uv_default_loop() *uv_loop_t; pub extern fn uv_loop_init(loop: *uv_loop_t) ReturnCode; pub extern fn uv_loop_close(loop: *uv_loop_t) c_int; pub extern fn uv_loop_new() *uv_loop_t; diff --git a/src/main.zig b/src/main.zig index 349332e08b7f82..624f8f33c78543 100644 --- a/src/main.zig +++ b/src/main.zig @@ -70,5 +70,14 @@ pub fn main() void { bun_warn_avx_missing(@import("./cli/upgrade_command.zig").Version.Bun__githubBaselineURL.ptr); } + if (Environment.isWindows) { + _ = bun.windows.libuv.uv_replace_allocator( + @ptrCast(&bun.Mimalloc.mi_malloc), + @ptrCast(&bun.Mimalloc.mi_realloc), + @ptrCast(&bun.Mimalloc.mi_calloc), + @ptrCast(&bun.Mimalloc.mi_free), + ); + } + bun.CLI.Cli.start(bun.default_allocator, stdout, stderr, MainPanicHandler); } diff --git a/test/js/web/workers/worker.test.ts b/test/js/web/workers/worker.test.ts index 17e40bae3de5b7..c9799c6bdca3d0 100644 --- a/test/js/web/workers/worker.test.ts +++ b/test/js/web/workers/worker.test.ts @@ -4,6 +4,8 @@ import { bunEnv, bunExe } from "harness"; import path from "path"; import wt from "worker_threads"; +const todoIfWindows = process.platform === "win32" ? test.todo : test; + describe("web worker", () => { async function waitForWorkerResult(worker: Worker, message: any): Promise { const promise = new Promise((resolve, reject) => { @@ -105,7 +107,7 @@ describe("web worker", () => { const result = await waitForWorkerResult(worker, "hello"); expect(result.argv).toHaveLength(2); - expect(result.execArgv).toHaveLength(0); + expect(result.execArgv).toEqual(process.execArgv); }); test("argv / execArgv options", async () => { @@ -120,7 +122,7 @@ describe("web worker", () => { const result = await waitForWorkerResult(worker, "hello"); expect(result).toEqual({ - argv: [original_argv[0], original_argv[1].replace(/\/[^/]+$/, "/worker-fixture-argv.js"), ...worker_argv], + argv: [original_argv[0], original_argv[1].replace(import.meta.file, "worker-fixture-argv.js"), ...worker_argv], execArgv: worker_execArgv, }); // ensure they didn't change for the main thread @@ -230,7 +232,7 @@ describe("worker_threads", () => { }); }); - test("worker terminate", async () => { + todoIfWindows("worker terminate", async () => { const worker = new wt.Worker(new URL("worker-fixture-hang.js", import.meta.url).href, { smol: true, }); @@ -238,7 +240,7 @@ describe("worker_threads", () => { expect(code).toBe(0); }); - test("worker with process.exit (delay) and terminate", async () => { + todoIfWindows("worker with process.exit (delay) and terminate", async () => { const worker = new wt.Worker(new URL("worker-fixture-process-exit.js", import.meta.url).href, { smol: true, }); @@ -280,7 +282,7 @@ describe("worker_threads", () => { const result = await promise; expect(result).toEqual({ - argv: [original_argv[0], original_argv[1].replace(/\/[^/]+$/, "/worker-fixture-argv.js"), ...worker_argv], + argv: [original_argv[0], original_argv[1].replace(import.meta.file, "worker-fixture-argv.js"), ...worker_argv], execArgv: worker_execArgv, });