Skip to content

fix(windows): close worker libuv loop #24811

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cirospaciari merged 6 commits into from
Nov 20, 2025
Merged

fix(windows): close worker libuv loop #24811

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7b72aa5
close threadlocal loop
dylan-conway Nov 18, 2025
51717ff
update
dylan-conway Nov 18, 2025
437d265
fix build
dylan-conway Nov 18, 2025
b38682e
[autofix.ci] apply automated fixes
autofix-ci[bot] Nov 18, 2025
771fdf5
update
dylan-conway Nov 18, 2025
7b32474
fixup
zackradisic Nov 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/bun.js/web_worker.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -611,10 +611,19 @@ pub fn exitAndDeinit(this: *WebWorker) noreturn {
loop_.internal_loop_data.jsc_vm = null;
}

if (vm_to_deinit) |vm| {
// this deinit needs to happen before `Loop.shutdown`
// in order to not call uv_close on the gc timer twice.
vm.gc_controller.deinit();
}

if (comptime Environment.isWindows) {
bun.windows.libuv.Loop.shutdown();
}

this.deinit();

if (vm_to_deinit) |vm| {
vm.gc_controller.deinit();
vm.deinit(); // NOTE: deinit here isn't implemented, so freeing workers will leak the vm.
}
bun.deleteAllPoolsForThreadExit();
Expand All @@ -637,6 +646,7 @@ const WTFStringImpl = @import("../string.zig").WTFStringImpl;

const bun = @import("bun");
const Async = bun.Async;
const Environment = bun.Environment;
const Output = bun.Output;
const assert = bun.assert;

Expand Down
31 changes: 20 additions & 11 deletions src/deps/libuv.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,19 +654,28 @@ pub const Loop = extern struct {
return null;
}

pub fn close(ptr: *Loop) void {
_ = uv_loop_close(ptr);
fn closeWalkCb(handle: ?*uv_handle_t, data: ?*anyopaque) callconv(.c) void {
_ = data;
const h = handle.?;
if (uv_is_closing(h) == 0) {
uv_close(h, null);
}
}
Comment on lines +657 to 663

@coderabbitai coderabbitai Bot Nov 20, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

LGTM: Cleanup callback correctly guards against double-close.

The closeWalkCb properly checks uv_is_closing before calling uv_close, preventing double-close attempts. The null callback is acceptable for cleanup scenarios.

Consider adding debug-only logging to track which handles are being cleaned up, which could aid troubleshooting:

 fn closeWalkCb(handle: ?*uv_handle_t, data: ?*anyopaque) callconv(.c) void {
     _ = data;
     const h = handle.?;
     if (uv_is_closing(h) == 0) {
+        if (comptime bun.Environment.isDebug) {
+            log("closeWalkCb: closing handle type {d}", .{h.type});
+        }
         uv_close(h, null);
     }
 }

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In src/deps/libuv.zig around lines 657 to 663, the cleanup callback currently
skips logging; add debug-only logging before calling uv_close to aid
troubleshooting by printing which handle is being closed (e.g., pointer address
and/or handle type) using a comptime debug guard so it’s only emitted in debug
builds (use std.debug.print guarded by @compileError? no — use if (builtin.mode
== .Debug) or a comptime std.build.Mode check appropriate for this repo), ensure
std is imported if needed, and keep the existing uv_is_closing check and null
callback behavior unchanged.


pub fn new() ?*Loop {
const ptr = bun.default_allocator.create(Loop) catch return null;
if (init(ptr) != null) return null;
return ptr;
}
pub fn shutdown() void {
const loop = threadlocal_loop orelse {
return;
};

if (uv_loop_close(loop).errEnum()) |err| {
if (err == .BUSY) {
uv_walk(loop, &closeWalkCb, null);
_ = uv_run(loop, .default);
bun.debugAssert(uv_loop_close(loop) == .zero);
}
}

pub fn delete(ptr: *Loop) void {
close(ptr);
bun.default_allocator.destroy(ptr);
threadlocal_loop = null;
}
Comment on lines +665 to 679

@coderabbitai coderabbitai Bot Nov 20, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider release-build error handling for final close failure.

The shutdown sequence correctly follows the libuv pattern: close → walk handles → run loop → close again. The debugAssert at line 674 assumes the second close will succeed after cleanup.

In release builds where assertions are disabled, if uv_loop_close unexpectedly fails after cleanup, the error is silently ignored and threadlocal_loop is still cleared. Consider whether release builds should log or handle this case:

if (uv_loop_close(loop).errEnum()) |err| {
    if (err == .BUSY) {
        uv_walk(loop, &closeWalkCb, null);
        _ = uv_run(loop, .default);
        if (uv_loop_close(loop).errEnum()) |final_err| {
            if (comptime bun.Environment.allow_assert) {
                bun.assert(false); // Should not happen
            } else {
                log("uv_loop_close failed after cleanup: {s}", .{@tagName(final_err)});
            }
        }
    } else {
        // Non-BUSY error, log it
        log("uv_loop_close failed: {s}", .{@tagName(err)});
    }
}
🤖 Prompt for AI Agents 
In src/deps/libuv.zig around lines 665-679, the shutdown path only uses
debugAssert after the second uv_loop_close call, which means release builds
ignore a potential final close failure; update the logic to (1) handle the
initial uv_loop_close error when it's not BUSY by logging the error, (2) after
walking and running the loop, re-call uv_loop_close and if that still errors
then either assert when assertions are enabled or log the final error in release
mode (use the existing bun assert/log facilities and include the error tagName
for clarity), and (3) ensure threadlocal_loop is only cleared after
handling/logging the final close result.


threadlocal var threadlocal_loop_data: Loop = undefined;
Expand Down Expand Up @@ -2120,7 +2129,7 @@ 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_loop_init(loop: *uv_loop_t) ReturnCode;
pub extern fn uv_loop_close(loop: *uv_loop_t) c_int;
pub extern fn uv_loop_close(loop: *uv_loop_t) ReturnCode;
pub extern fn uv_loop_new() *uv_loop_t;
pub extern fn uv_loop_delete(*uv_loop_t) void;
pub extern fn uv_loop_size() usize;
Expand Down