fix(windows): initialize uv allocators earlier (oven-sh#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 <[email protected]>
Hanaasagi · Feb 3, 2024 · 6bc62c6 · 6bc62c6
1 parent 929dc2f
commit 6bc62c6
Show file tree

Hide file tree

Showing 5 changed files with 30 additions and 32 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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<any> {
     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,15 +232,15 @@ 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,
     });
     const code = await worker.terminate();
     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,
     });