Skip to content

Commit

Permalink
fix(windows): initialize uv allocators earlier (#8631)
Browse files Browse the repository at this point in the history
* 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]>
  • Loading branch information
3 people authored Feb 2, 2024
1 parent 625b172 commit 647b15e
Show file tree
Hide file tree
Showing 5 changed files with 30 additions and 32 deletions.
7 changes: 0 additions & 7 deletions src/bun.js/event_loop.zig
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
7 changes: 6 additions & 1 deletion src/bun.js/web_worker.zig
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);

Expand All @@ -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?
Expand All @@ -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)
Expand Down
27 changes: 8 additions & 19 deletions src/deps/libuv.zig
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
Expand Down
9 changes: 9 additions & 0 deletions src/main.zig
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
12 changes: 7 additions & 5 deletions test/js/web/workers/worker.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down Expand Up @@ -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 () => {
Expand All @@ -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
Expand Down Expand Up @@ -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,
});
Expand Down Expand Up @@ -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,
});

Expand Down

0 comments on commit 647b15e

Please sign in to comment.