From 4091ec586e9281f84f291f9a0a95cbb76e5f0501 Mon Sep 17 00:00:00 2001 From: dave caruso Date: Mon, 11 Mar 2024 12:57:36 -0700 Subject: [PATCH 01/20] run-eval.test.ts --- test/cli/run/run-eval.test.ts | 41 +++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/test/cli/run/run-eval.test.ts b/test/cli/run/run-eval.test.ts index 7aab96f7cca94..5ea6f40cb65ea 100644 --- a/test/cli/run/run-eval.test.ts +++ b/test/cli/run/run-eval.test.ts @@ -1,3 +1,4 @@ +import { SpawnOptions, Subprocess, SyncSubprocess } from "bun"; import { describe, expect, test } from "bun:test"; import { mkdirSync, realpathSync } from "fs"; import { bunEnv, bunExe } from "harness"; @@ -6,8 +7,7 @@ import { join, sep } from "path"; describe("bun -e", () => { test("it works", async () => { - let { stdout } = Bun.spawnSync({ - cmd: [bunExe(), "-e", 'console.log("hello world")'], + const { stdout } = Bun.spawnSync([bunExe(), "-e", 'console.log("hello world")'], { env: bunEnv, }); expect(stdout.toString("utf8")).toEqual("hello world\n"); @@ -15,14 +15,16 @@ describe("bun -e", () => { test("import, tsx, require in esm, import.meta", async () => { const ref = await import("react"); - let { stdout } = Bun.spawnSync({ - cmd: [ + const { stdout } = Bun.spawnSync( + [ bunExe(), "-e", 'import {version} from "react"; console.log(JSON.stringify({version,file:import.meta.path,require:require("react").version})); console.log(world);', ], - env: bunEnv, - }); + { + env: bunEnv, + }, + ); const json = { version: ref.version, file: join(process.cwd(), "[eval]"), @@ -32,8 +34,7 @@ describe("bun -e", () => { }); test("error has source map info 1", async () => { - let { stdout, stderr } = Bun.spawnSync({ - cmd: [bunExe(), "-e", '(throw new Error("hi" as 2))'], + let { stderr } = Bun.spawnSync([bunExe(), "-e", '(throw new Error("hi" as 2))'], { env: bunEnv, }); expect(stderr.toString("utf8")).toInclude('"hi" as 2'); @@ -41,7 +42,7 @@ describe("bun -e", () => { }); }); -function group(run: (code: string) => ReturnType) { +function group(run: (code: string) => SyncSubprocess<"pipe", "inherit">) { test("it works", async () => { const { stdout } = run('console.log("hello world")'); expect(stdout.toString("utf8")).toEqual("hello world\n"); @@ -74,12 +75,20 @@ describe("bun run - < file-path.js", () => { const file = join(tmpdir(), "bun-run-eval-test.js"); require("fs").writeFileSync(file, code); try { - const result = Bun.spawnSync({ - cmd: ["bash", "-c", `${bunExe()} run - < ${file}`], - env: bunEnv, - stderr: "inherit", - }); + let result; + if (process.platform === "win32") { + result = Bun.spawnSync(["powershell", "-c", `Get-Content ${file} | ${bunExe()} run -`], { + env: bunEnv, + stderr: "inherit", + }); + } else { + result = Bun.spawnSync(["bash", "-c", `${bunExe()} run - < ${file}`], { + env: bunEnv, + stderr: "inherit", + }); + } + console.log(result); if (!result.success) { queueMicrotask(() => { throw new Error("bun run - < file-path.js failed"); @@ -99,10 +108,10 @@ describe("bun run - < file-path.js", () => { describe("echo | bun run -", () => { function run(code: string) { - const result = Bun.spawnSync({ - cmd: [bunExe(), "run", "-"], + const result = Bun.spawnSync([bunExe(), "run", "-"], { env: bunEnv, stdin: Buffer.from(code), + stderr: "inherit", }); if (!result.success) { queueMicrotask(() => { From b3c1a9f0957ef0a2274086bfd1848aaba42f1866 Mon Sep 17 00:00:00 2001 From: dave caruso Date: Mon, 11 Mar 2024 13:28:17 -0700 Subject: [PATCH 02/20] transpiler-cache.test.ts --- src/bun.js/RuntimeTranspilerCache.zig | 4 ++-- test/cli/run/run-eval.test.ts | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/bun.js/RuntimeTranspilerCache.zig b/src/bun.js/RuntimeTranspilerCache.zig index bac64f6d8f928..0c17858d905a2 100644 --- a/src/bun.js/RuntimeTranspilerCache.zig +++ b/src/bun.js/RuntimeTranspilerCache.zig @@ -597,7 +597,7 @@ pub const RuntimeTranspilerCache = struct { debug("get(\"{s}\") = {s}", .{ source.path.text, @errorName(err) }); return false; }; - if (comptime bun.Environment.allow_assert) { + if (comptime bun.Environment.isDebug) { if (bun_debug_restore_from_cache) { debug("get(\"{s}\") = {d} bytes, restored", .{ source.path.text, this.entry.?.output_code.byteSlice().len }); } else { @@ -606,7 +606,7 @@ pub const RuntimeTranspilerCache = struct { } bun.Analytics.Features.transpiler_cache = true; - if (comptime bun.Environment.allow_assert) { + if (comptime bun.Environment.isDebug) { if (!bun_debug_restore_from_cache) { if (this.entry) |*entry| { entry.deinit(this.sourcemap_allocator, this.output_code_allocator); diff --git a/test/cli/run/run-eval.test.ts b/test/cli/run/run-eval.test.ts index 5ea6f40cb65ea..5c5c7af0969f7 100644 --- a/test/cli/run/run-eval.test.ts +++ b/test/cli/run/run-eval.test.ts @@ -88,7 +88,6 @@ describe("bun run - < file-path.js", () => { }); } - console.log(result); if (!result.success) { queueMicrotask(() => { throw new Error("bun run - < file-path.js failed"); From 170d143f1cd29ac23d02643edd50aaf1114f21bf Mon Sep 17 00:00:00 2001 From: dave caruso Date: Mon, 11 Mar 2024 16:14:03 -0700 Subject: [PATCH 03/20] node net --- packages/bun-usockets/src/eventing/libuv.c | 14 ++++++---- src/async/windows_event_loop.zig | 2 ++ src/deps/libuv.zig | 30 +++++++++++++++++++--- test/js/node/net/node-net-server.test.ts | 12 ++++++--- test/js/node/net/node-unref-fixture.js | 1 + 5 files changed, 47 insertions(+), 12 deletions(-) diff --git a/packages/bun-usockets/src/eventing/libuv.c b/packages/bun-usockets/src/eventing/libuv.c index b5c3d8c8d2d27..e737cd8b6135c 100644 --- a/packages/bun-usockets/src/eventing/libuv.c +++ b/packages/bun-usockets/src/eventing/libuv.c @@ -91,6 +91,10 @@ void us_poll_start(struct us_poll_t *p, struct us_loop_t *loop, int events) { ((events & LIBUS_SOCKET_WRITABLE) ? POLL_TYPE_POLLING_OUT : 0); uv_poll_init_socket(loop->uv_loop, p->uv_p, p->fd); + // This unref is okay in the context of Bun's event loop, because sockets have + // a `Async.KeepAlive` associated with them, which is used instead of the + // usockets internals. usockets doesnt have a notion of ref-counted handles. + uv_unref((uv_handle_t *)p->uv_p); uv_poll_start(p->uv_p, events, poll_cb); } @@ -211,7 +215,7 @@ struct us_poll_t *us_create_poll(struct us_loop_t *loop, int fallthrough, return p; } -/* If we update our block position we have to updarte the uv_poll data to point +/* If we update our block position we have to update the uv_poll data to point * to us */ struct us_poll_t *us_poll_resize(struct us_poll_t *p, struct us_loop_t *loop, unsigned int ext_size) { @@ -225,8 +229,8 @@ struct us_poll_t *us_poll_resize(struct us_poll_t *p, struct us_loop_t *loop, // timer struct us_timer_t *us_create_timer(struct us_loop_t *loop, int fallthrough, unsigned int ext_size) { - struct us_internal_callback_t *cb = us_calloc(1, - sizeof(struct us_internal_callback_t) + sizeof(uv_timer_t) + ext_size); + struct us_internal_callback_t *cb = us_calloc( + 1, sizeof(struct us_internal_callback_t) + sizeof(uv_timer_t) + ext_size); cb->loop = loop; cb->cb_expects_the_loop = 0; // never read? @@ -288,8 +292,8 @@ struct us_loop_t *us_timer_loop(struct us_timer_t *t) { struct us_internal_async *us_internal_create_async(struct us_loop_t *loop, int fallthrough, unsigned int ext_size) { - struct us_internal_callback_t *cb = us_calloc(1, - sizeof(struct us_internal_callback_t) + sizeof(uv_async_t) + ext_size); + struct us_internal_callback_t *cb = us_calloc( + 1, sizeof(struct us_internal_callback_t) + sizeof(uv_async_t) + ext_size); cb->loop = loop; return (struct us_internal_async *)cb; diff --git a/src/async/windows_event_loop.zig b/src/async/windows_event_loop.zig index 4897ea9c9a98d..30a5c54a67136 100644 --- a/src/async/windows_event_loop.zig +++ b/src/async/windows_event_loop.zig @@ -259,12 +259,14 @@ pub const FilePoll = struct { pub fn deactivate(this: *FilePoll, loop: *Loop) void { std.debug.assert(this.flags.contains(.has_incremented_poll_count)); loop.active_handles -= @as(u32, @intFromBool(this.flags.contains(.has_incremented_poll_count))); + log("deactivate - {d}", .{loop.active_handles}); this.flags.remove(.has_incremented_poll_count); } /// Only intended to be used from EventLoop.Pollable pub fn activate(this: *FilePoll, loop: *Loop) void { loop.active_handles += @as(u32, @intFromBool(!this.flags.contains(.closed) and !this.flags.contains(.has_incremented_poll_count))); + log("activate - {d}", .{loop.active_handles}); this.flags.insert(.has_incremented_poll_count); } diff --git a/src/deps/libuv.zig b/src/deps/libuv.zig index 228033dca7249..ebf4debfdb194 100644 --- a/src/deps/libuv.zig +++ b/src/deps/libuv.zig @@ -597,10 +597,12 @@ pub const Loop = extern struct { wq_async: uv_async_t, pub fn subActive(this: *Loop, value: u32) void { + log("subActive({d}) - {d}", .{ value, this.active_handles }); this.active_handles -= value; } pub fn addActive(this: *Loop, value: u32) void { + log("addActive({d})", .{value}); this.active_handles += value; } @@ -608,15 +610,28 @@ pub const Loop = extern struct { pub const unref = dec; pub fn inc(this: *Loop) void { + log("inc - {d}", .{this.active_handles + 1}); + + // This log may be helpful if you are curious where KeepAlives are being created from + // if (Env.isDebug) { + // std.debug.dumpCurrentStackTrace(@returnAddress()); + // } this.active_handles += 1; } pub fn dec(this: *Loop) void { + log("dec", .{}); this.active_handles -= 1; } - pub fn isActive(this: *const Loop) bool { - return uv_loop_alive(this) != 0; + pub fn isActive(this: *Loop) bool { + const loop_alive = uv_loop_alive(this) != 0; + // This log may be helpful if you are curious what exact handles are active + // if (Env.isDebug and loop_alive) { + // bun.Output.debug("Active Handles:", .{}); + // dumpActiveHandles(this, null); + // } + return loop_alive; } pub fn init(ptr: *Loop) ?bun.C.E { @@ -668,16 +683,23 @@ pub const Loop = extern struct { } pub fn refConcurrently(this: *Loop) void { + log("refConcurrently", .{}); _ = @atomicRmw(c_uint, &this.active_handles, std.builtin.AtomicRmwOp.Add, 1, .Monotonic); } pub fn unrefConcurrently(this: *Loop) void { + log("unrefConcurrently", .{}); _ = @atomicRmw(c_uint, &this.active_handles, std.builtin.AtomicRmwOp.Sub, 1, .Monotonic); } pub fn unrefCount(this: *Loop, count: i32) void { + log("unrefCount({d})", .{count}); this.active_handles -= @intCast(count); } + + pub fn dumpActiveHandles(this: *Loop, stream: ?*FILE) void { + uv_print_active_handles(this, stream); + } }; pub const struct_uv__work = extern struct { work: ?*const fn ([*c]struct_uv__work) callconv(.C) void, @@ -2024,8 +2046,8 @@ pub extern fn uv_req_get_type(req: [*c]const uv_req_t) uv_req_type; pub extern fn uv_req_type_name(@"type": uv_req_type) [*]const u8; pub extern fn uv_is_active(handle: *const uv_handle_t) c_int; pub extern fn uv_walk(loop: *uv_loop_t, walk_cb: uv_walk_cb, arg: ?*anyopaque) void; -pub extern fn uv_print_all_handles(loop: *uv_loop_t, stream: [*c]FILE) void; -pub extern fn uv_print_active_handles(loop: *uv_loop_t, stream: [*c]FILE) void; +pub extern fn uv_print_all_handles(loop: *uv_loop_t, stream: ?*FILE) void; +pub extern fn uv_print_active_handles(loop: *uv_loop_t, stream: ?*FILE) void; pub extern fn uv_close(handle: *uv_handle_t, close_cb: uv_close_cb) void; pub extern fn uv_send_buffer_size(handle: *uv_handle_t, value: [*c]c_int) c_int; pub extern fn uv_recv_buffer_size(handle: *uv_handle_t, value: [*c]c_int) c_int; diff --git a/test/js/node/net/node-net-server.test.ts b/test/js/node/net/node-net-server.test.ts index 1ad14400bd435..1ceb6d588bbf0 100644 --- a/test/js/node/net/node-net-server.test.ts +++ b/test/js/node/net/node-net-server.test.ts @@ -282,9 +282,15 @@ describe("net.createServer listen", () => { } catch (e) { err = e as Error; } - expect(err).not.toBeNull(); - expect(err!.message).toBe("Failed to connect"); - expect(err!.name).toBe("ECONNREFUSED"); + + if (process.platform !== "win32") { + expect(err).not.toBeNull(); + expect(err!.message).toBe("Failed to connect"); + expect(err!.name).toBe("ECONNREFUSED"); + } else { + // Bun allows this to work on Windows + expect(err).toBeNull(); + } server.close(); done(); diff --git a/test/js/node/net/node-unref-fixture.js b/test/js/node/net/node-unref-fixture.js index 7a03de7011dff..283048bd41498 100644 --- a/test/js/node/net/node-unref-fixture.js +++ b/test/js/node/net/node-unref-fixture.js @@ -8,6 +8,7 @@ const socket = connect( }, () => { socket.on("data", () => { + console.error("Received data. FAIL"); process.exit(1); }); socket.write("GET / HTTP/1.1\r\nHost: www.example.com\r\n\r\n\r\n"); From 5d310959c33831f105f08b81f8c477b450071c3c Mon Sep 17 00:00:00 2001 From: dave caruso Date: Mon, 11 Mar 2024 18:04:25 -0700 Subject: [PATCH 04/20] some open things --- src/bun.js/node/node_fs.zig | 10 +++++----- src/sys.zig | 26 ++++++++++++++++++++++---- test/js/node/fs/fs.test.ts | 31 ++++++++++++++++++------------- 3 files changed, 45 insertions(+), 22 deletions(-) diff --git a/src/bun.js/node/node_fs.zig b/src/bun.js/node/node_fs.zig index c0c9b8bc46696..79215dd804a2f 100644 --- a/src/bun.js/node/node_fs.zig +++ b/src/bun.js/node/node_fs.zig @@ -3779,7 +3779,7 @@ pub const NodeFS = struct { .path => |path_| { const path = path_.sliceZ(&this.sync_error_buf); - const fd = switch (Syscall.open(path, @intFromEnum(FileSystemFlags.a), 0o000666)) { + const fd = switch (bun.sys.open(path, @intFromEnum(FileSystemFlags.a), 0o666)) { .result => |result| result, .err => |err| return .{ .err = err }, }; @@ -4518,7 +4518,7 @@ pub const NodeFS = struct { else args.path.sliceZ(&this.sync_error_buf); - return switch (Syscall.open(path, @intFromEnum(args.flags), args.mode)) { + return switch (bun.sys.open(path, @intFromEnum(args.flags), args.mode)) { .err => |err| .{ .err = err.withPath(args.path.slice()), }, @@ -5156,7 +5156,7 @@ pub const NodeFS = struct { } } - break :brk switch (Syscall.open( + break :brk switch (bun.sys.open( path, os.O.RDONLY | os.O.NOCTTY, 0, @@ -5533,7 +5533,7 @@ pub const NodeFS = struct { else std.os.O.RDONLY; - const fd = switch (Syscall.open(path, flags, 0)) { + const fd = switch (bun.sys.open(path, flags, 0)) { .err => |err| return .{ .err = err.withPath(path) }, .result => |fd_| fd_, }; @@ -5783,7 +5783,7 @@ pub const NodeFS = struct { fn _truncate(this: *NodeFS, path: PathLike, len: JSC.WebCore.Blob.SizeType, flags: i32, comptime _: Flavor) Maybe(Return.Truncate) { if (comptime Environment.isWindows) { - const file = Syscall.open( + const file = bun.sys.open( path.sliceZ(&this.sync_error_buf), os.O.WRONLY | flags, 0o644, diff --git a/src/sys.zig b/src/sys.zig index a9b441dc8a710..35cd0a781253c 100644 --- a/src/sys.zig +++ b/src/sys.zig @@ -771,6 +771,8 @@ pub fn openFileAtWindowsNtPath( }; var io: windows.IO_STATUS_BLOCK = undefined; + var attributes: w.DWORD = w.FILE_ATTRIBUTE_NORMAL; + while (true) { const rc = windows.ntdll.NtCreateFile( &result, @@ -778,7 +780,7 @@ pub fn openFileAtWindowsNtPath( &attr, &io, null, - w.FILE_ATTRIBUTE_NORMAL, + attributes, w.FILE_SHARE_WRITE | w.FILE_SHARE_READ | w.FILE_SHARE_DELETE, disposition, options, @@ -802,6 +804,25 @@ pub fn openFileAtWindowsNtPath( } } + if (rc == .ACCESS_DENIED and + attributes == w.FILE_ATTRIBUTE_NORMAL and + (access_mask & (w.GENERIC_READ | w.GENERIC_WRITE)) == w.GENERIC_WRITE) + { + // > If CREATE_ALWAYS and FILE_ATTRIBUTE_NORMAL are specified, + // > CreateFile fails and sets the last error to ERROR_ACCESS_DENIED + // > if the file exists and has the FILE_ATTRIBUTE_HIDDEN or + // > FILE_ATTRIBUTE_SYSTEM attribute. To avoid the error, specify the + // > same attributes as the existing file. + // + // The above also applies to NtCreateFile. In order to make this work, + // we retry but only in the case that the file was opened for writing. + // + // See https://github.com/oven-sh/bun/issues/6820 + // https://github.com/libuv/libuv/pull/3380 + attributes = w.FILE_ATTRIBUTE_HIDDEN; + continue; + } + switch (windows.Win32Error.fromNTStatus(rc)) { .SUCCESS => { if (access_mask & w.FILE_APPEND_DATA != 0) { @@ -1003,9 +1024,6 @@ pub fn openA(file_path: []const u8, flags: bun.Mode, perm: bun.Mode) Maybe(bun.F } pub fn open(file_path: [:0]const u8, flags: bun.Mode, perm: bun.Mode) Maybe(bun.FileDescriptor) { - if (comptime Environment.isWindows) { - return sys_uv.open(file_path, flags, perm); - } // this is what open() does anyway. return openat(bun.toFD((std.fs.cwd().fd)), file_path, flags, perm); } diff --git a/test/js/node/fs/fs.test.ts b/test/js/node/fs/fs.test.ts index 755348522327f..9ded0e8c7db58 100644 --- a/test/js/node/fs/fs.test.ts +++ b/test/js/node/fs/fs.test.ts @@ -1,4 +1,3 @@ -// @known-failing-on-windows: 1 failing import { describe, expect, it } from "bun:test"; import { dirname, resolve, relative } from "node:path"; import { promisify } from "node:util"; @@ -61,13 +60,15 @@ function mkdirForce(path: string) { } it("fdatasyncSync", () => { - const fd = openSync(import.meta.path, "w", 0o664); + const temp = tmpdir(); + const fd = openSync(join(temp, "test.blob"), "w", 0o664); fdatasyncSync(fd); closeSync(fd); }); it("fdatasync", done => { - const fd = openSync(import.meta.path, "w", 0o664); + const temp = tmpdir(); + const fd = openSync(join(temp, "test.blob"), "w", 0o664); fdatasync(fd, function () { done(...arguments); closeSync(fd); @@ -94,7 +95,7 @@ it("writeFileSync in append should not truncate the file", () => { expect(readFileSync(path, "utf8")).toBe(str); }); -it.skipIf(isWindows)("await readdir #3931", async () => { +it("await readdir #3931", async () => { const { exitCode } = spawnSync({ cmd: [bunExe(), join(import.meta.dir, "./repro-3931.js")], env: bunEnv, @@ -960,8 +961,6 @@ describe("writeSync", () => { it("works with a position set to 0", () => { const fd = openSync(import.meta.dir + "/writeFileSync.txt", "w+"); - const four = new Uint8Array(4); - { const count = writeSync(fd, new TextEncoder().encode("File"), 0, 4, 0); expect(count).toBe(4); @@ -970,7 +969,6 @@ describe("writeSync", () => { }); it("works without position set", () => { const fd = openSync(import.meta.dir + "/writeFileSync.txt", "w+"); - const four = new Uint8Array(4); { const count = writeSync(fd, new TextEncoder().encode("File")); expect(count).toBe(4); @@ -1627,7 +1625,7 @@ describe("createReadStream", () => { }); }); -describe.skipIf(isWindows)("fs.WriteStream", () => { +describe("fs.WriteStream", () => { it("should be exported", () => { expect(fs.WriteStream).toBeDefined(); }); @@ -1722,7 +1720,7 @@ describe.skipIf(isWindows)("fs.WriteStream", () => { }); }); -describe.skipIf(isWindows)("fs.ReadStream", () => { +describe("fs.ReadStream", () => { it("should be exported", () => { expect(fs.ReadStream).toBeDefined(); }); @@ -1835,7 +1833,7 @@ describe.skipIf(isWindows)("fs.ReadStream", () => { }); }); -describe.skipIf(isWindows)("createWriteStream", () => { +describe("createWriteStream", () => { it("simple write stream finishes", async () => { const path = `${tmpdir()}/fs.test.js/${Date.now()}.createWriteStream.txt`; const stream = createWriteStream(path); @@ -2364,6 +2362,7 @@ describe("utimesSync", () => { expect(finalStats.atime).toEqual(prevAccessTime); }); + // TODO: make this work on Windows it.skipIf(isWindows)("works after 2038", () => { const tmp = join(tmpdir(), "utimesSync-test-file-" + Math.random().toString(36).slice(2)); writeFileSync(tmp, "test"); @@ -2742,9 +2741,15 @@ it("test syscall errno, issue#4198", () => { }); it.if(isWindows)("writing to windows hidden file is possible", () => { - Bun.spawnSync(["cmd", "/C", "touch file.txt && attrib +h file.txt"], { stdio: ["ignore", "ignore", "ignore"] }); - writeFileSync("file.txt", "Hello World"); - const content = readFileSync("file.txt", "utf8"); + const temp = tmpdir(); + writeFileSync(join(temp, "file.txt"), "FAIL"); + const status = Bun.spawnSync(["cmd", "/C", "attrib +h file.txt"], { + stdio: ["ignore", "ignore", "ignore"], + cwd: temp, + }); + expect(status.exitCode).toBe(0); + writeFileSync(join(temp, "file.txt"), "Hello World"); + const content = readFileSync(join(temp, "file.txt"), "utf8"); expect(content).toBe("Hello World"); }); From 9d768e6f76a5362d66fa49538c0b8b3c394c63bf Mon Sep 17 00:00:00 2001 From: dave caruso Date: Mon, 11 Mar 2024 18:19:41 -0700 Subject: [PATCH 05/20] a --- test/js/node/fs/writeFileSync.txt | 1 - 1 file changed, 1 deletion(-) delete mode 100644 test/js/node/fs/writeFileSync.txt diff --git a/test/js/node/fs/writeFileSync.txt b/test/js/node/fs/writeFileSync.txt deleted file mode 100644 index a0fe4515f9038..0000000000000 --- a/test/js/node/fs/writeFileSync.txt +++ /dev/null @@ -1 +0,0 @@ -File \ No newline at end of file From 5b8653aaecd1ae996a0b3f059b395e2840cfdf01 Mon Sep 17 00:00:00 2001 From: dave caruso Date: Mon, 11 Mar 2024 18:19:53 -0700 Subject: [PATCH 06/20] a --- test/js/node/fs/writeFileSync.txt | 1 + 1 file changed, 1 insertion(+) create mode 100644 test/js/node/fs/writeFileSync.txt diff --git a/test/js/node/fs/writeFileSync.txt b/test/js/node/fs/writeFileSync.txt new file mode 100644 index 0000000000000..a0fe4515f9038 --- /dev/null +++ b/test/js/node/fs/writeFileSync.txt @@ -0,0 +1 @@ +File \ No newline at end of file From aa1c9899110b1d1ca96a1f76503cdfd1bd2bc056 Mon Sep 17 00:00:00 2001 From: dave caruso Date: Mon, 11 Mar 2024 19:03:46 -0700 Subject: [PATCH 07/20] yikes --- src/sys.zig | 5 +++++ test/js/node/fs/fs.test.ts | 6 +++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/sys.zig b/src/sys.zig index 35cd0a781253c..6063ebf9ef357 100644 --- a/src/sys.zig +++ b/src/sys.zig @@ -1024,6 +1024,11 @@ pub fn openA(file_path: []const u8, flags: bun.Mode, perm: bun.Mode) Maybe(bun.F } pub fn open(file_path: [:0]const u8, flags: bun.Mode, perm: bun.Mode) Maybe(bun.FileDescriptor) { + // TODO(@paperdave): this should not need to use libuv + if (comptime Environment.isWindows) { + return sys_uv.open(file_path, flags, perm); + } + // this is what open() does anyway. return openat(bun.toFD((std.fs.cwd().fd)), file_path, flags, perm); } diff --git a/test/js/node/fs/fs.test.ts b/test/js/node/fs/fs.test.ts index 9ded0e8c7db58..e729ccbf639b0 100644 --- a/test/js/node/fs/fs.test.ts +++ b/test/js/node/fs/fs.test.ts @@ -2159,7 +2159,7 @@ describe("fs/promises", () => { }); }); -it("stat on a large file", () => { +it("fstat on a large file", () => { var dest: string = "", fd; try { @@ -2171,13 +2171,13 @@ it("stat on a large file", () => { while (offset < 5 * 1024 * 1024 * 1024) { offset += writeSync(fd, bigBuffer, 0, bigBuffer.length, offset); } - + fdatasyncSync(fd); expect(fstatSync(fd).size).toEqual(offset); } finally { if (fd) closeSync(fd); unlinkSync(dest); } -}); +}, 20_000); it("fs.constants", () => { if (isWindows) { From ad61bd50e8823a601910c6b320270a04abcd1cb2 Mon Sep 17 00:00:00 2001 From: dave caruso Date: Tue, 12 Mar 2024 17:35:19 -0700 Subject: [PATCH 08/20] incredible --- src/bun.js/RuntimeTranspilerCache.zig | 5 +++-- src/bun.js/module_loader.zig | 13 +++++------ src/bun.js/node/node_fs.zig | 17 ++++++++++++--- src/bun.js/webcore/streams.zig | 31 +++++++++++++++++++++------ src/bun.zig | 6 +++--- src/cli.zig | 2 +- src/cli/install.ps1 | 10 ++++----- src/fd.zig | 30 +++++++++++++++++--------- src/fs.zig | 7 ++++-- src/io/PipeReader.zig | 10 ++++----- src/io/source.zig | 9 +++++--- src/js/node/fs.js | 13 ++++++----- src/js/node/stream.js | 3 ++- src/libarchive/libarchive.zig | 5 ++++- src/shell/interpreter.zig | 16 +++++++------- src/sys.zig | 29 ++++++++++++++++++++++++- src/tmp.zig | 10 +++++++-- 17 files changed, 152 insertions(+), 64 deletions(-) diff --git a/src/bun.js/RuntimeTranspilerCache.zig b/src/bun.js/RuntimeTranspilerCache.zig index 0c17858d905a2..80b445ecfeeed 100644 --- a/src/bun.js/RuntimeTranspilerCache.zig +++ b/src/bun.js/RuntimeTranspilerCache.zig @@ -543,8 +543,9 @@ pub const RuntimeTranspilerCache = struct { const cache_dir_fd = brk: { if (std.fs.path.dirname(cache_file_path)) |dirname| { - const dir = try std.fs.cwd().makeOpenPath(dirname, .{ .access_sub_paths = true }); - break :brk bun.toLibUVOwnedFD(dir.fd); + var dir = try std.fs.cwd().makeOpenPath(dirname, .{ .access_sub_paths = true }); + errdefer dir.close(); + break :brk try bun.toLibUVOwnedFD(dir.fd); } break :brk bun.toFD(std.fs.cwd().fd); diff --git a/src/bun.js/module_loader.zig b/src/bun.js/module_loader.zig index ec3fea4cab4f4..8454cf16e866b 100644 --- a/src/bun.js/module_loader.zig +++ b/src/bun.js/module_loader.zig @@ -2009,19 +2009,20 @@ pub const ModuleLoader = struct { else => { var stack_buf = std.heap.stackFallback(4096, jsc_vm.allocator); const allocator = stack_buf.get(); - var buf = MutableString.init2048(allocator) catch unreachable; + var buf = MutableString.init2048(allocator) catch bun.outOfMemory(); defer buf.deinit(); var writer = buf.writer(); if (!jsc_vm.origin.isEmpty()) { - writer.writeAll("export default `") catch unreachable; + writer.writeAll("export default `") catch bun.outOfMemory(); // TODO: escape backtick char, though we might already do that JSC.API.Bun.getPublicPath(specifier, jsc_vm.origin, @TypeOf(&writer), &writer); - writer.writeAll("`;\n") catch unreachable; + writer.writeAll("`;\n") catch bun.outOfMemory(); } else { - writer.writeAll("export default ") catch unreachable; - buf = js_printer.quoteForJSON(specifier, buf, true) catch @panic("out of memory"); + // search keywords: "export default \"{}\";" + writer.writeAll("export default ") catch bun.outOfMemory(); + buf = js_printer.quoteForJSON(specifier, buf, true) catch bun.outOfMemory(); writer = buf.writer(); - writer.writeAll(";\n") catch unreachable; + writer.writeAll(";\n") catch bun.outOfMemory(); } const public_url = bun.String.createUTF8(buf.toOwnedSliceLeaky()); diff --git a/src/bun.js/node/node_fs.zig b/src/bun.js/node/node_fs.zig index 79215dd804a2f..b082519a3f72b 100644 --- a/src/bun.js/node/node_fs.zig +++ b/src/bun.js/node/node_fs.zig @@ -5124,7 +5124,7 @@ pub const NodeFS = struct { pub fn readFileWithOptions(this: *NodeFS, args: Arguments.ReadFile, comptime _: Flavor, comptime string_type: StringType) Maybe(Return.ReadFileWithOptions) { var path: [:0]const u8 = undefined; - const fd: FileDescriptor = bun.toLibUVOwnedFD(switch (args.path) { + const fd_maybe_windows: FileDescriptor = switch (args.path) { .path => brk: { path = args.path.path.sliceZ(&this.sync_error_buf); if (this.vm) |vm| { @@ -5168,7 +5168,18 @@ pub const NodeFS = struct { }; }, .fd => |fd| fd, - }); + }; + const fd = bun.toLibUVOwnedFD(fd_maybe_windows) catch { + if (args.path == .path) + _ = Syscall.close(fd_maybe_windows); + + return .{ + .err = .{ + .errno = @intFromEnum(os.E.MFILE), + .syscall = .open, + }, + }; + }; defer { if (args.path == .path) @@ -5187,6 +5198,7 @@ pub const NodeFS = struct { _ = Syscall.setFileOffset(fd, args.offset); } // For certain files, the size might be 0 but the file might still have contents. + // https://github.com/oven-sh/bun/issues/1220 const size = @as( u64, @max( @@ -5224,7 +5236,6 @@ pub const NodeFS = struct { }, } } else { - // https://github.com/oven-sh/bun/issues/1220 while (true) { switch (Syscall.read(fd, buf.items.ptr[total..buf.capacity])) { .err => |err| return .{ diff --git a/src/bun.js/webcore/streams.zig b/src/bun.js/webcore/streams.zig index 2874ee3f791fb..16fc865e60dd5 100644 --- a/src/bun.js/webcore/streams.zig +++ b/src/bun.js/webcore/streams.zig @@ -3384,29 +3384,38 @@ pub const FileReader = struct { break :brk 0; })) { - .result => |_fd| if (Environment.isWindows) bun.toLibUVOwnedFD(_fd) else _fd, + .result => |fd| if (Environment.isWindows) + switch (bun.sys.toLibUVOwnedFD(fd, .dup, .close_on_fail)) { + .result => |converted_fd| converted_fd, + .err => |err| return .{ .err = err.withFd(file.pathlike.fd) }, + } + else + fd, .err => |err| { return .{ .err = err.withFd(file.pathlike.fd) }; }, } else switch (Syscall.open(file.pathlike.path.sliceZ(&file_buf), std.os.O.RDONLY | std.os.O.NONBLOCK | std.os.O.CLOEXEC, 0)) { - .result => |_fd| _fd, + .result => |fd| fd, .err => |err| { return .{ .err = err.withPath(file.pathlike.path.slice()) }; }, }; if (comptime Environment.isPosix) { - if ((file.is_atty orelse false) or (fd.int() < 3 and std.os.isatty(fd.cast())) or (file.pathlike == .fd and bun.FDTag.get(file.pathlike.fd) != .none and std.os.isatty(file.pathlike.fd.cast()))) { + if ((file.is_atty orelse false) or + (fd.int() < 3 and std.os.isatty(fd.cast())) or + (file.pathlike == .fd and + bun.FDTag.get(file.pathlike.fd) != .none and + std.os.isatty(file.pathlike.fd.cast()))) + { // var termios = std.mem.zeroes(std.os.termios); // _ = std.c.tcgetattr(fd.cast(), &termios); // bun.C.cfmakeraw(&termios); // _ = std.c.tcsetattr(fd.cast(), std.os.TCSA.NOW, &termios); file.is_atty = true; } - } - if (comptime Environment.isPosix) { const stat: bun.Stat = switch (Syscall.fstat(fd)) { .result => |result| result, .err => |err| { @@ -3421,7 +3430,12 @@ pub const FileReader = struct { } this.pollable = bun.sys.isPollable(stat.mode) or (file.is_atty orelse false); - this.file_type = if (bun.S.ISFIFO(stat.mode)) .pipe else if (bun.S.ISSOCK(stat.mode)) .socket else .file; + this.file_type = if (bun.S.ISFIFO(stat.mode)) + .pipe + else if (bun.S.ISSOCK(stat.mode)) + .socket + else + .file; this.nonblocking = this.pollable and !(file.is_atty orelse false); if (this.nonblocking and this.file_type == .pipe) { @@ -3475,6 +3489,8 @@ pub const FileReader = struct { return .{ .err = err }; }, .result => |opened| { + std.debug.assert(opened.fd.isValid()); + std.debug.assert(bun.uvfdcast(opened.fd) >= 0); this.fd = opened.fd; pollable = opened.pollable; file_type = opened.file_type; @@ -3489,9 +3505,12 @@ pub const FileReader = struct { { const reader_fd = this.reader.getFd(); if (reader_fd != bun.invalid_fd and this.fd == bun.invalid_fd) { + std.debug.assert(bun.uvfdcast(this.fd) >= 0); this.fd = reader_fd; } } + std.debug.assert(this.fd.isValid()); + std.debug.assert(bun.uvfdcast(this.fd) >= 0); this.event_loop = JSC.EventLoopHandle.init(this.parent().globalThis.bunVM().eventLoop()); diff --git a/src/bun.zig b/src/bun.zig index 45a88d7a487a8..6662b52118442 100644 --- a/src/bun.zig +++ b/src/bun.zig @@ -1880,13 +1880,13 @@ pub inline fn toFD(fd: anytype) FileDescriptor { /// Accepts either a UV descriptor (i32) or a windows handle (*anyopaque) /// /// On windows, this file descriptor will always be backed by libuv, so calling .close() is safe. -pub inline fn toLibUVOwnedFD(fd: anytype) FileDescriptor { +pub inline fn toLibUVOwnedFD(fd: anytype) !FileDescriptor { const T = @TypeOf(fd); if (Environment.isWindows) { return (switch (T) { - FDImpl.System => FDImpl.fromSystem(fd).makeLibUVOwned(), + FDImpl.System => try FDImpl.fromSystem(fd).makeLibUVOwned(), FDImpl.UV => FDImpl.fromUV(fd), - FileDescriptor => FDImpl.decode(fd).makeLibUVOwned(), + FileDescriptor => try FDImpl.decode(fd).makeLibUVOwned(), FDImpl => fd.makeLibUVOwned(), else => @compileError("toLibUVOwnedFD() does not support type \"" ++ @typeName(T) ++ "\""), }).encode(); diff --git a/src/cli.zig b/src/cli.zig index 0699666d57da4..415a7a12caad6 100644 --- a/src/cli.zig +++ b/src/cli.zig @@ -1857,7 +1857,7 @@ pub const Command = struct { const file_pathZ = script_name_buf[0..file_path.len :0]; break :brk bun.openFileZ(file_pathZ, .{ .mode = .read_only }); } - }) catch return false).handle); + }) catch return false).handle) catch return false; defer _ = bun.sys.close(file); switch (bun.sys.fstat(file)) { diff --git a/src/cli/install.ps1 b/src/cli/install.ps1 index 10f2ac6007125..1342d37b530c0 100644 --- a/src/cli/install.ps1 +++ b/src/cli/install.ps1 @@ -219,11 +219,11 @@ function Install-Bun { $env:IS_BUN_AUTO_UPDATE = "1" $null = "$(& "${BunBin}\bun.exe" completions)" - # if ($LASTEXITCODE -ne 0) { - # Write-Output "Install Failed - could not finalize installation" - # Write-Output "The command '${BunBin}\bun.exe completions' exited with code ${LASTEXITCODE}`n" - # exit 1 - # } + if ($LASTEXITCODE -ne 0) { + Write-Output "Install Failed - could not finalize installation" + Write-Output "The command '${BunBin}\bun.exe completions' exited with code ${LASTEXITCODE}`n" + exit 1 + } $env:IS_BUN_AUTO_UPDATE = $null $DisplayVersion = if ($BunRevision -like "*-canary.*") { diff --git a/src/fd.zig b/src/fd.zig index f0d516e9b5cd7..66e16a8c5620a 100644 --- a/src/fd.zig +++ b/src/fd.zig @@ -34,15 +34,13 @@ fn numberToHandle(handle: FDImpl.SystemAsInt) FDImpl.System { pub fn uv_get_osfhandle(in: c_int) libuv.uv_os_fd_t { const out = libuv.uv_get_osfhandle(in); - // TODO: this is causing a dead lock because is also used on fd format - // log("uv_get_osfhandle({d}) = {d}", .{ in, @intFromPtr(out) }); return out; } -pub fn uv_open_osfhandle(in: libuv.uv_os_fd_t) c_int { +pub fn uv_open_osfhandle(in: libuv.uv_os_fd_t) error{SystemFdQuotaExceeded}!c_int { const out = libuv.uv_open_osfhandle(in); - // TODO: this is causing a dead lock because is also used on fd format - // log("uv_open_osfhandle({d}) = {d}", .{ @intFromPtr(in), out }); + std.debug.assert(out >= -1); + if (out == -1) return error.SystemFdQuotaExceeded; return out; } @@ -200,12 +198,15 @@ pub const FDImpl = packed struct { return this.closeAllowingStdoutAndStderr(); } - pub fn makeLibUVOwned(this: FDImpl) FDImpl { + /// Assumes given a valid file descriptor + /// If error, the handle has not been closed + pub fn makeLibUVOwned(this: FDImpl) !FDImpl { + this.assertValid(); return switch (env.os) { else => this, .windows => switch (this.kind) { .system => fd: { - break :fd FDImpl.fromUV(uv_open_osfhandle(numberToHandle(this.value.as_system))); + break :fd FDImpl.fromUV(try uv_open_osfhandle(numberToHandle(this.value.as_system))); }, .uv => this, }, @@ -303,9 +304,18 @@ pub const FDImpl = packed struct { return FDImpl.fromUV(fd); } - /// After calling, the input file descriptor is no longer valid and must not be used - pub fn toJS(value: FDImpl, _: *JSC.JSGlobalObject) JSValue { - return JSValue.jsNumberFromInt32(value.makeLibUVOwned().uv()); + /// After calling, the input file descriptor is no longer valid and must not be used. + /// If an error is thrown, the file descriptor is cleaned up for you. + pub fn toJS(value: FDImpl, global: *JSC.JSGlobalObject) JSValue { + const fd = value.makeLibUVOwned() catch { + _ = value.close(); + global.throwValue((JSC.SystemError{ + .message = bun.String.static("EMFILE, too many open files"), + .code = bun.String.static("EMFILE"), + }).toErrorInstance(global)); + return .zero; + }; + return JSValue.jsNumberFromInt32(fd.uv()); } pub fn format(this: FDImpl, comptime fmt: []const u8, _: std.fmt.FormatOptions, writer: anytype) !void { diff --git a/src/fs.zig b/src/fs.zig index 317865cb6b21d..26f286d91284d 100644 --- a/src/fs.zig +++ b/src/fs.zig @@ -710,8 +710,11 @@ pub const FileSystem = struct { const flags = std.os.O.CREAT | std.os.O.WRONLY | std.os.O.CLOEXEC; - const result = try bun.sys.openat(bun.toFD(tmpdir_.fd), name, flags, 0).unwrap(); - this.fd = bun.toLibUVOwnedFD(result); + this.fd = brk: { + const fd = try bun.sys.openat(bun.toFD(tmpdir_.fd), name, flags, 0).unwrap(); + errdefer _ = bun.sys.close(fd); + break :brk try bun.toLibUVOwnedFD(fd); + }; var buf: [bun.MAX_PATH_BYTES]u8 = undefined; const existing_path = try bun.getFdPath(this.fd, &buf); this.existing_path = try bun.default_allocator.dupe(u8, existing_path); diff --git a/src/io/PipeReader.zig b/src/io/PipeReader.zig index 3ab76f645b154..3121d955a4da3 100644 --- a/src/io/PipeReader.zig +++ b/src/io/PipeReader.zig @@ -372,7 +372,7 @@ pub fn WindowsPipeReader( const nread_int = fs.result.int(); const continue_reading = !this.flags.is_paused; this.flags.is_paused = true; - bun.sys.syslog("onFileRead() = {d}", .{nread_int}); + bun.sys.syslog("onFileRead({}) = {d}", .{ bun.toFD(fs.file.fd), nread_int }); switch (nread_int) { // 0 actually means EOF too @@ -383,7 +383,7 @@ pub fn WindowsPipeReader( this.onRead(.{ .result = 0 }, "", .drained); }, else => { - if (fs.result.toError(.recv)) |err| { + if (fs.result.toError(.read)) |err| { this.onRead(.{ .err = err }, "", .progress); return; } @@ -413,6 +413,7 @@ pub fn WindowsPipeReader( if (this.flags.is_done or !this.flags.is_paused) return .{ .result = {} }; this.flags.is_paused = false; const source: Source = this.source orelse return .{ .err = bun.sys.Error.fromCode(bun.C.E.BADF, .read) }; + std.debug.assert(!source.isClosed()); switch (source) { .file => |file| { @@ -1017,7 +1018,7 @@ pub const WindowsBufferedReader = struct { } pub fn done(this: *WindowsOutputReader) void { - std.debug.assert(if (this.source) |source| source.isClosed() else true); + if (this.source) |source| std.debug.assert(source.isClosed()); this.finish(); @@ -1037,7 +1038,7 @@ pub const WindowsBufferedReader = struct { } pub fn startWithCurrentPipe(this: *WindowsOutputReader) bun.JSC.Maybe(void) { - std.debug.assert(this.source != null); + std.debug.assert(!this.source.?.isClosed()); this.buffer().clearRetainingCapacity(); this.flags.is_done = false; this.unpause(); @@ -1045,7 +1046,6 @@ pub const WindowsBufferedReader = struct { } pub fn startWithPipe(this: *WindowsOutputReader, pipe: *uv.Pipe) bun.JSC.Maybe(void) { - std.debug.assert(this.source == null); this.source = .{ .pipe = pipe }; return this.startWithCurrentPipe(); } diff --git a/src/io/source.zig b/src/io/source.zig index 44e9d2d965749..1daa3d27e6e81 100644 --- a/src/io/source.zig +++ b/src/io/source.zig @@ -130,6 +130,7 @@ pub const Source = union(enum) { } pub fn openFile(fd: bun.FileDescriptor) *Source.File { + std.debug.assert(fd.isValid() and bun.uvfdcast(fd) != -1); log("openFile (fd = {})", .{fd}); const file = bun.default_allocator.create(Source.File) catch bun.outOfMemory(); @@ -155,9 +156,11 @@ pub const Source = union(enum) { } }, else => { - return .{ .result = .{ - .file = openFile(fd), - } }; + return .{ + .result = .{ + .file = openFile(fd), + }, + }; }, } } diff --git a/src/js/node/fs.js b/src/js/node/fs.js index 54bf1b4ff5ce0..a25ef7fa9137d 100644 --- a/src/js/node/fs.js +++ b/src/js/node/fs.js @@ -548,7 +548,7 @@ ReadStream = (function (InternalReadStream) { start = defaultReadStreamOptions.start, end = defaultReadStreamOptions.end, autoDestroy = defaultReadStreamOptions.autoClose, - fs = defaultReadStreamOptions.fs, + fs: overridden_fs = defaultReadStreamOptions.fs, highWaterMark = defaultReadStreamOptions.highWaterMark, fd = defaultReadStreamOptions.fd, } = options; @@ -588,7 +588,7 @@ ReadStream = (function (InternalReadStream) { // If fd not open for this file, open it if (tempThis.fd === undefined) { // NOTE: this fs is local to constructor, from options - tempThis.fd = fs.openSync(pathOrFd, flags, mode); + tempThis.fd = overridden_fs.openSync(pathOrFd, flags, mode); } // Get FileRef from fd var fileRef = Bun.file(tempThis.fd); @@ -598,8 +598,7 @@ ReadStream = (function (InternalReadStream) { var stream = fileRef.stream(); var ptr = stream.$bunNativePtr; if (!ptr) { - $debug("no native readable stream"); - throw new Error("no native readable stream"); + throw new Error("Failed to get internal stream controller. This is a bug in Bun"); } super(ptr, { @@ -639,6 +638,9 @@ ReadStream = (function (InternalReadStream) { if (start !== undefined) { this.pos = start; } + + $assert(overridden_fs); + this.#fs = overridden_fs; } #fileRef; #fs; @@ -677,6 +679,7 @@ ReadStream = (function (InternalReadStream) { if (!fd) { cb(err); } else { + $assert(this.#fs); this.#fs.close(fd, er => { cb(er || err); }); @@ -742,7 +745,7 @@ ReadStream = (function (InternalReadStream) { #internalRead(n) { // pos is the current position in the file // by default, if a start value is provided, pos starts at this.start - var { pos, end, bytesRead, fd, encoding } = this; + var { pos, end, bytesRead, fd } = this; n = pos !== undefined // if there is a pos, then we are reading from that specific position in the file diff --git a/src/js/node/stream.js b/src/js/node/stream.js index b194f2155d763..749401b223a7f 100644 --- a/src/js/node/stream.js +++ b/src/js/node/stream.js @@ -3414,6 +3414,7 @@ var require_readable = __commonJS({ }); // node_modules/readable-stream/lib/internal/streams/writable.js +var errorOrDestroy; var require_writable = __commonJS({ "node_modules/readable-stream/lib/internal/streams/writable.js"(exports, module) { "use strict"; @@ -3444,7 +3445,7 @@ var require_writable = __commonJS({ ERR_STREAM_WRITE_AFTER_END, ERR_UNKNOWN_ENCODING, } = require_errors().codes; - var { errorOrDestroy } = destroyImpl; + ({ errorOrDestroy } = destroyImpl); function Writable(options = {}) { const isDuplex = this instanceof require_duplex(); diff --git a/src/libarchive/libarchive.zig b/src/libarchive/libarchive.zig index 542f0131750db..ed06b8d13acbb 100644 --- a/src/libarchive/libarchive.zig +++ b/src/libarchive/libarchive.zig @@ -632,7 +632,10 @@ pub const Archive = struct { }).handle; } }; - const file_handle = bun.toLibUVOwnedFD(file_handle_native); + const file_handle = brk: { + errdefer _ = bun.sys.close(file_handle_native); + break :brk try bun.toLibUVOwnedFD(file_handle_native); + }; defer if (comptime close_handles) { // On windows, AV hangs these closes really badly. diff --git a/src/shell/interpreter.zig b/src/shell/interpreter.zig index 463fb5c18bb67..4f9191247f04b 100644 --- a/src/shell/interpreter.zig +++ b/src/shell/interpreter.zig @@ -9604,13 +9604,13 @@ const ShellSyscall = struct { .err => |e| return .{ .err = e }, }; return switch (Syscall.openDirAtWindowsA(dir, p, true, flags & os.O.NOFOLLOW != 0)) { - .result => |fd| .{ .result = bun.toLibUVOwnedFD(fd) }, - .err => |e| return .{ .err = e.withPath(path) }, + .result => |fd| bun.sys.toLibUVOwnedFD(fd, .open, .close_on_fail), + .err => |e| .{ .err = e.withPath(path) }, }; } return switch (Syscall.openDirAtWindowsA(dir, path, true, flags & os.O.NOFOLLOW != 0)) { - .result => |fd| .{ .result = bun.toLibUVOwnedFD(fd) }, - .err => |e| return .{ .err = e.withPath(path) }, + .result => |fd| bun.sys.toLibUVOwnedFD(fd, .open, .close_on_fail), + .err => |e| .{ .err = e.withPath(path) }, }; } @@ -9627,7 +9627,7 @@ const ShellSyscall = struct { .err => |e| return .{ .err = e.withPath(path) }, }; if (bun.Environment.isWindows) { - return .{ .result = bun.toLibUVOwnedFD(fd) }; + return bun.sys.toLibUVOwnedFD(fd, .open, .close_on_fail); } return .{ .result = fd }; } @@ -9638,7 +9638,7 @@ const ShellSyscall = struct { .err => |e| return .{ .err = e }, }; if (bun.Environment.isWindows) { - return .{ .result = bun.toLibUVOwnedFD(fd) }; + return bun.sys.toLibUVOwnedFD(fd, .open, .close_on_fail); } return .{ .result = fd }; } @@ -9646,8 +9646,8 @@ const ShellSyscall = struct { pub fn dup(fd: bun.FileDescriptor) Maybe(bun.FileDescriptor) { if (bun.Environment.isWindows) { return switch (Syscall.dup(fd)) { - .result => |f| return .{ .result = bun.toLibUVOwnedFD(f) }, - .err => |e| return .{ .err = e }, + .result => |duped_fd| bun.sys.toLibUVOwnedFD(duped_fd, .dup, .close_on_fail), + .err => |e| .{ .err = e }, }; } return Syscall.dup(fd); diff --git a/src/sys.zig b/src/sys.zig index 6063ebf9ef357..c7e70231a0227 100644 --- a/src/sys.zig +++ b/src/sys.zig @@ -2056,12 +2056,13 @@ pub fn dupWithFlags(fd: bun.FileDescriptor, flags: i32) Maybe(bun.FileDescriptor w.TRUE, w.DUPLICATE_SAME_ACCESS, ); - log("dup({d}) = {d}", .{ fd.cast(), out }); if (out == 0) { if (Maybe(bun.FileDescriptor).errnoSysFd(0, .dup, fd)) |err| { + log("dup({}) = {}", .{ fd, err }); return err; } } + log("dup({}) = {}", .{ fd, bun.toFD(target) }); return Maybe(bun.FileDescriptor){ .result = bun.toFD(target) }; } @@ -2338,3 +2339,29 @@ pub const File = struct { _ = This.close(self.handle); } }; + +pub inline fn toLibUVOwnedFD( + maybe_windows_fd: bun.FileDescriptor, + comptime syscall: Syscall.Tag, + comptime error_case: enum { close_on_fail, leak_fd_on_fail }, +) Maybe(bun.FileDescriptor) { + if (!Environment.isWindows) { + return .{ .result = maybe_windows_fd }; + } + + return .{ + .result = bun.toLibUVOwnedFD(maybe_windows_fd) catch |err| switch (err) { + error.SystemFdQuotaExceeded => { + if (error_case == .close_on_fail) { + _ = close(maybe_windows_fd); + } + return .{ + .err = .{ + .errno = @intFromEnum(bun.C.E.MFILE), + .syscall = syscall, + }, + }; + }, + }, + }; +} diff --git a/src/tmp.zig b/src/tmp.zig index 2efd8c82ec924..22531204f960f 100644 --- a/src/tmp.zig +++ b/src/tmp.zig @@ -28,7 +28,10 @@ pub const Tmpfile = struct { if (comptime allow_tmpfile) { switch (bun.sys.openat(destination_dir, ".", O.WRONLY | O.TMPFILE | O.CLOEXEC, perm)) { .result => |fd| { - tmpfile.fd = bun.toLibUVOwnedFD(fd); + tmpfile.fd = switch (bun.sys.makeLibUVOwnedFD(fd, .open, .close_on_fail)) { + .result => |owned_fd| owned_fd, + .err => |err| return .{ .err = err }, + }; break :open; }, .err => |err| { @@ -43,7 +46,10 @@ pub const Tmpfile = struct { } tmpfile.fd = switch (bun.sys.openat(destination_dir, tmpfilename, O.CREAT | O.CLOEXEC | O.WRONLY, perm)) { - .result => |fd| bun.toLibUVOwnedFD(fd), + .result => |fd| switch (bun.sys.toLibUVOwnedFD(fd, .open, .close_on_fail)) { + .result => |owned_fd| owned_fd, + .err => |err| return .{ .err = err }, + }, .err => |err| return .{ .err = err }, }; break :open; From 77cf5570f0cf1eff181f1c7b3c14299c1e794d85 Mon Sep 17 00:00:00 2001 From: dave caruso Date: Tue, 12 Mar 2024 18:46:38 -0700 Subject: [PATCH 09/20] run it back --- src/bun.js/node/node_fs.zig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/bun.js/node/node_fs.zig b/src/bun.js/node/node_fs.zig index b082519a3f72b..28824ee73df8f 100644 --- a/src/bun.js/node/node_fs.zig +++ b/src/bun.js/node/node_fs.zig @@ -3779,7 +3779,7 @@ pub const NodeFS = struct { .path => |path_| { const path = path_.sliceZ(&this.sync_error_buf); - const fd = switch (bun.sys.open(path, @intFromEnum(FileSystemFlags.a), 0o666)) { + const fd = switch (Syscall.open(path, @intFromEnum(FileSystemFlags.a), 0o666)) { .result => |result| result, .err => |err| return .{ .err = err }, }; @@ -4518,7 +4518,7 @@ pub const NodeFS = struct { else args.path.sliceZ(&this.sync_error_buf); - return switch (bun.sys.open(path, @intFromEnum(args.flags), args.mode)) { + return switch (Syscall.open(path, @intFromEnum(args.flags), args.mode)) { .err => |err| .{ .err = err.withPath(args.path.slice()), }, From 5ad751c04ff8f6a457c1ee406bbba11627ac00d5 Mon Sep 17 00:00:00 2001 From: dave caruso Date: Tue, 12 Mar 2024 18:51:15 -0700 Subject: [PATCH 10/20] a --- src/bun.js/webcore/blob.zig | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/bun.js/webcore/blob.zig b/src/bun.js/webcore/blob.zig index 6e630aa79aa7a..14a1f8d758b55 100644 --- a/src/bun.js/webcore/blob.zig +++ b/src/bun.js/webcore/blob.zig @@ -2298,7 +2298,13 @@ pub const Blob = struct { open_source_flags, 0, )) { - .result => |result| bun.toLibUVOwnedFD(result), + .result => |result| switch (bun.sys.toLibUVOwnedFD(result, .open, .close_on_fail)) { + .result => |result_fd| result_fd, + .err => |errno| { + this.system_error = errno.toSystemError(); + return bun.errnoToZigErr(errno.errno); + }, + }, .err => |errno| { this.system_error = errno.toSystemError(); return bun.errnoToZigErr(errno.errno); @@ -2314,7 +2320,13 @@ pub const Blob = struct { open_destination_flags, JSC.Node.default_permission, )) { - .result => |result| bun.toLibUVOwnedFD(result), + .result => |result| switch (bun.sys.toLibUVOwnedFD(result, .open, .close_on_fail)) { + .result => |result_fd| result_fd, + .err => |errno| { + this.system_error = errno.toSystemError(); + return bun.errnoToZigErr(errno.errno); + }, + }, .err => |errno| { switch (mkdirIfNotExists(this, errno, dest, dest)) { .@"continue" => continue, From 94196e8c1440b9d574a786cd0e1c4b57db08d7d4 Mon Sep 17 00:00:00 2001 From: dave caruso Date: Tue, 12 Mar 2024 20:15:45 -0700 Subject: [PATCH 11/20] this code is what i like to call, incorrect --- src/bun.js/webcore/streams.zig | 1 - 1 file changed, 1 deletion(-) diff --git a/src/bun.js/webcore/streams.zig b/src/bun.js/webcore/streams.zig index 16fc865e60dd5..0ba405105cd1f 100644 --- a/src/bun.js/webcore/streams.zig +++ b/src/bun.js/webcore/streams.zig @@ -3505,7 +3505,6 @@ pub const FileReader = struct { { const reader_fd = this.reader.getFd(); if (reader_fd != bun.invalid_fd and this.fd == bun.invalid_fd) { - std.debug.assert(bun.uvfdcast(this.fd) >= 0); this.fd = reader_fd; } } From aa051544373eff18db6dc1486ce41e65faf2fb4d Mon Sep 17 00:00:00 2001 From: dave caruso Date: Tue, 12 Mar 2024 20:17:04 -0700 Subject: [PATCH 12/20] ok its all worng --- src/bun.js/webcore/streams.zig | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/bun.js/webcore/streams.zig b/src/bun.js/webcore/streams.zig index 0ba405105cd1f..a99fcc1ff7454 100644 --- a/src/bun.js/webcore/streams.zig +++ b/src/bun.js/webcore/streams.zig @@ -3490,7 +3490,6 @@ pub const FileReader = struct { }, .result => |opened| { std.debug.assert(opened.fd.isValid()); - std.debug.assert(bun.uvfdcast(opened.fd) >= 0); this.fd = opened.fd; pollable = opened.pollable; file_type = opened.file_type; @@ -3509,7 +3508,6 @@ pub const FileReader = struct { } } std.debug.assert(this.fd.isValid()); - std.debug.assert(bun.uvfdcast(this.fd) >= 0); this.event_loop = JSC.EventLoopHandle.init(this.parent().globalThis.bunVM().eventLoop()); From b81e6397fc0b298d846f0351af348b2c1a5c54ec Mon Sep 17 00:00:00 2001 From: dave caruso Date: Wed, 13 Mar 2024 13:02:26 -0700 Subject: [PATCH 13/20] remove an assertion that is wrong again --- src/bun.js/webcore/streams.zig | 1 - 1 file changed, 1 deletion(-) diff --git a/src/bun.js/webcore/streams.zig b/src/bun.js/webcore/streams.zig index a99fcc1ff7454..090efa1c6653f 100644 --- a/src/bun.js/webcore/streams.zig +++ b/src/bun.js/webcore/streams.zig @@ -3507,7 +3507,6 @@ pub const FileReader = struct { this.fd = reader_fd; } } - std.debug.assert(this.fd.isValid()); this.event_loop = JSC.EventLoopHandle.init(this.parent().globalThis.bunVM().eventLoop()); From 1d61dd28bb5d55a5060a0cec9b262a38767b4760 Mon Sep 17 00:00:00 2001 From: dave caruso Date: Wed, 13 Mar 2024 15:04:34 -0700 Subject: [PATCH 14/20] update test things and rebase --- test/js/bun/test/test-test.test.ts | 1 - test/js/bun/util/mmap.test.js | 4 +--- test/js/node/net/node-net-server.test.ts | 1 - test/js/node/net/node-net.test.ts | 1 - 4 files changed, 1 insertion(+), 6 deletions(-) diff --git a/test/js/bun/test/test-test.test.ts b/test/js/bun/test/test-test.test.ts index aabc3882c6d83..b91fe56833b39 100644 --- a/test/js/bun/test/test-test.test.ts +++ b/test/js/bun/test/test-test.test.ts @@ -1,4 +1,3 @@ -// @known-failing-on-windows: 1 failing // @ts-nocheck import { spawn, spawnSync } from "bun"; import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, test } from "bun:test"; diff --git a/test/js/bun/util/mmap.test.js b/test/js/bun/util/mmap.test.js index dae1b8de1bc8f..303103258de5f 100644 --- a/test/js/bun/util/mmap.test.js +++ b/test/js/bun/util/mmap.test.js @@ -1,9 +1,7 @@ -// @known-failing-on-windows: 1 failing import { describe, it, expect } from "bun:test"; import { gcTick, isWindows } from "harness"; -// We do not support mmap() on Windows -// Maybe we can add it later. +// TODO: We do not support mmap() on Windows. Maybe we can add it later. describe.skipIf(isWindows)("Bun.mmap", async () => { await gcTick(); const path = `/tmp/bun-mmap-test_${Math.random()}.txt`; diff --git a/test/js/node/net/node-net-server.test.ts b/test/js/node/net/node-net-server.test.ts index 1ceb6d588bbf0..8303b636716da 100644 --- a/test/js/node/net/node-net-server.test.ts +++ b/test/js/node/net/node-net-server.test.ts @@ -1,4 +1,3 @@ -// @known-failing-on-windows: 1 failing import { createServer, Server, AddressInfo, Socket } from "net"; import { realpathSync } from "fs"; import { tmpdir } from "os"; diff --git a/test/js/node/net/node-net.test.ts b/test/js/node/net/node-net.test.ts index d77771c168345..5095e8376b86a 100644 --- a/test/js/node/net/node-net.test.ts +++ b/test/js/node/net/node-net.test.ts @@ -1,4 +1,3 @@ -// @known-failing-on-windows: 1 failing import { ServerWebSocket, TCPSocket, Socket as _BunSocket, TCPSocketListener } from "bun"; import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it } from "bun:test"; import { connect, isIP, isIPv4, isIPv6, Socket, createConnection } from "net"; From a7b65ad95568e417c0d605547fe83a1892ad7e93 Mon Sep 17 00:00:00 2001 From: dave caruso Date: Wed, 13 Mar 2024 18:40:34 -0700 Subject: [PATCH 15/20] performance test --- src/bun.js/javascript.zig | 2 +- test/harness.ts | 3 --- test/js/web/timers/performance.test.js | 28 ++++++++++++++++++-------- 3 files changed, 21 insertions(+), 12 deletions(-) diff --git a/src/bun.js/javascript.zig b/src/bun.js/javascript.zig index dc445c041f046..0c9e036532315 100644 --- a/src/bun.js/javascript.zig +++ b/src/bun.js/javascript.zig @@ -1215,7 +1215,7 @@ pub const VirtualMachine = struct { .source_mappings = undefined, .macros = MacroMap.init(allocator), .macro_entry_points = @TypeOf(vm.macro_entry_points).init(allocator), - .origin_timer = std.time.Timer.start() catch @panic("Please don't mess with timers."), + .origin_timer = std.time.Timer.start() catch @panic("Timers are not supported on this system."), .origin_timestamp = getOriginTimestamp(), .ref_strings = JSC.RefString.Map.init(allocator), .ref_strings_mutex = Lock.init(), diff --git a/test/harness.ts b/test/harness.ts index 97dfe5cec855d..7be552d70b0d6 100644 --- a/test/harness.ts +++ b/test/harness.ts @@ -321,9 +321,6 @@ export async function toBeWorkspaceLink(actual: string, expectedLinkPath: string } export function getMaxFD(): number { - if (isWindows) { - return 0; - } const maxFD = openSync("/dev/null", "r"); closeSync(maxFD); return maxFD; diff --git a/test/js/web/timers/performance.test.js b/test/js/web/timers/performance.test.js index 0d6cd577bb35f..eb3fa41af4a11 100644 --- a/test/js/web/timers/performance.test.js +++ b/test/js/web/timers/performance.test.js @@ -1,4 +1,5 @@ import { expect, it } from "bun:test"; +import { isWindows } from "harness"; it("performance.now() should be monotonic", () => { const first = performance.now(); @@ -7,14 +8,25 @@ it("performance.now() should be monotonic", () => { const fourth = performance.now(); const fifth = performance.now(); const sixth = performance.now(); - expect(first < second).toBe(true); - expect(second < third).toBe(true); - expect(third < fourth).toBe(true); - expect(fourth < fifth).toBe(true); - expect(fifth < sixth).toBe(true); - expect(Bun.nanoseconds() > 0).toBe(true); - expect(Bun.nanoseconds() > sixth).toBe(true); - expect(typeof Bun.nanoseconds() === "number").toBe(true); + if (isWindows) { + // Timer precision is monotonic on Windows, but it is 100ns of precision + // making it extremely easy to hit overlapping timer values here. + expect(first).toBeLessThanOrEqual(second); + expect(second).toBeLessThanOrEqual(third); + expect(third).toBeLessThanOrEqual(fourth); + expect(fourth).toBeLessThanOrEqual(fifth); + expect(fifth).toBeLessThanOrEqual(sixth); + Bun.sleepSync(0.001); + } else { + expect(first).toBeLessThan(second); + expect(second).toBeLessThan(third); + expect(third).toBeLessThan(fourth); + expect(fourth).toBeLessThan(fifth); + expect(fifth).toBeLessThan(sixth); + } + expect(Bun.nanoseconds()).toBeGreaterThan(0); + expect(Bun.nanoseconds()).toBeGreaterThan(sixth); + expect(Bun.nanoseconds()).toBeNumber(true); }); it("performance.timeOrigin + performance.now() should be similar to Date.now()", () => { From 3ce3538c69ddf43e0d66dd141332245a1a56af7c Mon Sep 17 00:00:00 2001 From: dave caruso Date: Wed, 13 Mar 2024 18:45:42 -0700 Subject: [PATCH 16/20] mark filesink with mkfifo as todo. see #8166 --- test/js/bun/util/filesink.test.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/js/bun/util/filesink.test.ts b/test/js/bun/util/filesink.test.ts index b76a98cac0476..e581ecfe64a7f 100644 --- a/test/js/bun/util/filesink.test.ts +++ b/test/js/bun/util/filesink.test.ts @@ -1,6 +1,7 @@ // @known-failing-on-windows: 1 failing import { ArrayBufferSink } from "bun"; import { describe, expect, it } from "bun:test"; +import { isWindows } from "harness"; import { mkfifo } from "mkfifo"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -77,7 +78,9 @@ describe("FileSink", () => { } for (let isPipe of [true, false] as const) { - describe(isPipe ? "pipe" : "file", () => { + // TODO: fix the `mkfifo` function for windows. They do have an API but calling it from bun:ffi didn't get great results. + // once #8166 is merged, this can be written using it's 'bun:iternals-for-testing' feature + describe.skipIf(isPipe && isWindows)(isPipe ? "pipe" : "file", () => { fixtures.forEach(([input, expected, label]) => { const getPathOrFd = () => (isPipe ? getFd(label, expected.byteLength) : getPath(label)); From a35961cc55bba597c3b6bdb83574dfe1ba35a72b Mon Sep 17 00:00:00 2001 From: dave caruso Date: Thu, 21 Mar 2024 10:33:59 -0700 Subject: [PATCH 17/20] hehe --- src/sys_uv.zig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sys_uv.zig b/src/sys_uv.zig index 86bb9eedb09ea..4b50b41a33ec7 100644 --- a/src/sys_uv.zig +++ b/src/sys_uv.zig @@ -350,9 +350,9 @@ pub fn preadv(fd: FileDescriptor, bufs: []const bun.PlatformIOVec, position: i64 return .{ .result = @as(usize, @intCast(rc.int())) }; } } + pub fn pwritev(fd: FileDescriptor, bufs: []const bun.PlatformIOVecConst, position: i64) Maybe(usize) { - // TODO: @paperdave is this bad? - const uv_fd = bun.uvfdcast(bun.toLibUVOwnedFD(fd)); + const uv_fd = bun.uvfdcast(fd); comptime std.debug.assert(bun.PlatformIOVec == uv.uv_buf_t); const debug_timer = bun.Output.DebugTimer.start(); From e7c27b41d422f0874ec83ef896393e2682c1bf51 Mon Sep 17 00:00:00 2001 From: dave caruso Date: Thu, 21 Mar 2024 16:34:56 -0700 Subject: [PATCH 18/20] not done --- src/bun.js/webcore/streams.zig | 29 ++++++++++++---- src/cli/install.ps1 | 19 +++++++---- src/fd.zig | 3 ++ src/file_reader.zig | 0 src/js/node/fs.js | 61 +++++++++++++++------------------- src/js/node/fs.promises.ts | 47 ++++++++++++++++---------- test/js/node/fs/fs.test.ts | 1 - 7 files changed, 95 insertions(+), 65 deletions(-) delete mode 100644 src/file_reader.zig diff --git a/src/bun.js/webcore/streams.zig b/src/bun.js/webcore/streams.zig index cc3586032e4e8..158b7a752de79 100644 --- a/src/bun.js/webcore/streams.zig +++ b/src/bun.js/webcore/streams.zig @@ -3399,15 +3399,32 @@ pub const FileReader = struct { file_type: bun.io.FileType = .file, }; - pub fn openFileBlob( - file: *Blob.FileStore, - ) JSC.Maybe(OpenedFileBlob) { + pub fn openFileBlob(file: *Blob.FileStore) JSC.Maybe(OpenedFileBlob) { var this = OpenedFileBlob{ .fd = bun.invalid_fd }; var file_buf: [std.fs.MAX_PATH_BYTES]u8 = undefined; - const fd = if (file.pathlike != .path) - // We will always need to close the file descriptor. - file.pathlike.fd + const fd = if (file.pathlike == .fd) + if (file.pathlike.fd.isStdio()) + file.pathlike.fd + else switch (Syscall.dupWithFlags(file.pathlike.fd, brk: { + if (comptime Environment.isPosix) { + if (bun.FDTag.get(file.pathlike.fd) == .none and !(file.is_atty orelse false)) { + break :brk std.os.O.NONBLOCK; + } + } + + break :brk 0; + })) { + .result => |fd| switch (bun.sys.toLibUVOwnedFD(fd, .dup, .close_on_fail)) { + .result => |owned_fd| owned_fd, + .err => |err| { + return .{ .err = err }; + }, + }, + .err => |err| { + return .{ .err = err.withFd(file.pathlike.fd) }; + }, + } else switch (Syscall.open(file.pathlike.path.sliceZ(&file_buf), std.os.O.RDONLY | std.os.O.NONBLOCK | std.os.O.CLOEXEC, 0)) { .result => |fd| fd, .err => |err| { diff --git a/src/cli/install.ps1 b/src/cli/install.ps1 index 1342d37b530c0..8fa12631920fc 100644 --- a/src/cli/install.ps1 +++ b/src/cli/install.ps1 @@ -217,12 +217,17 @@ function Install-Bun { exit 1 } - $env:IS_BUN_AUTO_UPDATE = "1" - $null = "$(& "${BunBin}\bun.exe" completions)" - if ($LASTEXITCODE -ne 0) { - Write-Output "Install Failed - could not finalize installation" - Write-Output "The command '${BunBin}\bun.exe completions' exited with code ${LASTEXITCODE}`n" - exit 1 + try { + $env:IS_BUN_AUTO_UPDATE = "1" + $output = "$(& "${BunBin}\bun.exe" completions 2>&1)" + if ($LASTEXITCODE -ne 0) { + Write-Output $output + Write-Output "Install Failed - could not finalize installation" + Write-Output "The command '${BunBin}\bun.exe completions' exited with code ${LASTEXITCODE}`n" + exit 1 + } + } catch { + # it is possible on powershell 5 that an error } $env:IS_BUN_AUTO_UPDATE = $null @@ -238,7 +243,7 @@ function Install-Bun { Write-Output "${C_GREEN}Bun ${DisplayVersion} was installed successfully!${C_RESET}" Write-Output "The binary is located at ${BunBin}\bun.exe`n" - Write-Warning "Bun for Windows is currently experimental.`nFor a more stable experience, please install Bun within WSL:`nhttps://bun.sh/docs/installation`n" + Write-Warning "Bun for Windows is currently experimental and must not be used in production.`nFor a more stable experience, install Bun within WSL:`nhttps://bun.sh/docs/installation`n" $hasExistingOther = $false; try { diff --git a/src/fd.zig b/src/fd.zig index 4873be9f0dd3d..bc691cf5a7b46 100644 --- a/src/fd.zig +++ b/src/fd.zig @@ -245,6 +245,9 @@ pub const FDImpl = packed struct { .windows => result: { switch (this.kind) { .uv => { + std.debug.getStderrMutex().lock(); + std.debug.dumpCurrentStackTrace(@returnAddress()); + std.debug.getStderrMutex().unlock(); var req: libuv.fs_t = libuv.fs_t.uninitialized; defer req.deinit(); const rc = libuv.uv_fs_close(libuv.Loop.get(), &req, this.value.as_uv, null); diff --git a/src/file_reader.zig b/src/file_reader.zig deleted file mode 100644 index e69de29bb2d1d..0000000000000 diff --git a/src/js/node/fs.js b/src/js/node/fs.js index a25ef7fa9137d..3c185db9aa145 100644 --- a/src/js/node/fs.js +++ b/src/js/node/fs.js @@ -511,6 +511,8 @@ var defaultReadStreamOptions = { autoDestroy: true, }; +let { FileHandle, kRef, kUnref, kFd } = promises.$data; + var ReadStreamClass; ReadStream = (function (InternalReadStream) { @@ -559,11 +561,22 @@ ReadStream = (function (InternalReadStream) { // This is kinda hacky but we create a temporary object to assign props that we will later pull into the `this` context after we call super var tempThis = {}; + let handle = null; if (fd != null) { if (typeof fd !== "number") { - throw new TypeError("Expected options.fd to be a number"); + if (fd instanceof FileHandle) { + tempThis.fd = fd[kFd]; + if (tempThis.fd < 0) { + throw new Error("Expected a valid file descriptor"); + } + fd[kRef](); + handle = fd; + } else { + throw new TypeError("Expected options.fd to be a number or FileHandle"); + } + } else { + tempThis.fd = tempThis[readStreamPathOrFdSymbol] = fd; } - tempThis.fd = tempThis[readStreamPathOrFdSymbol] = fd; tempThis.autoClose = false; } else if (typeof pathOrFd === "string") { if (pathOrFd.startsWith("file://")) { @@ -590,6 +603,7 @@ ReadStream = (function (InternalReadStream) { // NOTE: this fs is local to constructor, from options tempThis.fd = overridden_fs.openSync(pathOrFd, flags, mode); } + // Get FileRef from fd var fileRef = Bun.file(tempThis.fd); @@ -612,8 +626,7 @@ ReadStream = (function (InternalReadStream) { // Assign the tempThis props to this Object.assign(this, tempThis); - this.#fileRef = fileRef; - + this.#handle = handle; this.end = end; this._read = this.#internalRead; this.start = start; @@ -642,7 +655,7 @@ ReadStream = (function (InternalReadStream) { $assert(overridden_fs); this.#fs = overridden_fs; } - #fileRef; + #handle; #fs; file; path; @@ -673,9 +686,16 @@ ReadStream = (function (InternalReadStream) { _destroy(err, cb) { super._destroy(err, cb); try { - var fd = this.fd; this[readStreamPathFastPathSymbol] = false; + var handle = this.#handle; + if (handle) { + handle[kUnref](); + this.fd = null; + this.#handle = null; + return; + } + var fd = this.fd; if (!fd) { cb(err); } else { @@ -1156,25 +1176,6 @@ function createWriteStream(path, options) { return new WriteStream(path, options); } -// NOTE: This was too smart and doesn't actually work -// WriteStream = Object.defineProperty( -// function WriteStream(path, options) { -// var _InternalWriteStream = getLazyWriteStream(); -// return new _InternalWriteStream(path, options); -// }, -// Symbol.hasInstance, -// { value: (instance) => instance[writeStreamSymbol] === true }, -// ); - -// ReadStream = Object.defineProperty( -// function ReadStream(path, options) { -// var _InternalReadStream = getLazyReadStream(); -// return new _InternalReadStream(path, options); -// }, -// Symbol.hasInstance, -// { value: (instance) => instance[readStreamSymbol] === true }, -// ); - Object.defineProperties(fs, { createReadStream: { value: createReadStream, @@ -1188,12 +1189,6 @@ Object.defineProperties(fs, { WriteStream: { value: WriteStream, }, - // ReadStream: { - // get: () => getLazyReadStream(), - // }, - // WriteStream: { - // get: () => getLazyWriteStream(), - // }, }); // lol @@ -1201,7 +1196,6 @@ Object.defineProperties(fs, { realpath.native = realpath; realpathSync.native = realpathSync; -let lazy_cpSync = null; // attempt to use the native code version if possible // and on MacOS, simple cases of recursive directory trees can be done in a single `clonefile()` // using filter and other options uses a lazily loaded js fallback ported from node.js @@ -1211,8 +1205,7 @@ function cpSync(src, dest, options) { throw new TypeError("options must be an object"); } if (options.dereference || options.filter || options.preserveTimestamps || options.verbatimSymlinks) { - if (!lazy_cpSync) lazy_cpSync = require("../internal/fs/cp-sync"); - return lazy_cpSync(src, dest, options); + return require("../internal/fs/cp-sync")(src, dest, options); } return fs.cpSync(src, dest, options.recursive, options.errorOnExist, options.force ?? true, options.mode); } diff --git a/src/js/node/fs.promises.ts b/src/js/node/fs.promises.ts index c9f2d0d37636a..11bb73ccde54c 100644 --- a/src/js/node/fs.promises.ts +++ b/src/js/node/fs.promises.ts @@ -5,7 +5,6 @@ const constants = $processBindingConstants.fs; const EventEmitter = require("node:events"); var PromisePrototypeThen = Promise.prototype.then; -var PromiseResolve = Promise.resolve; var SafePromisePrototypeFinally = Promise.prototype.finally; //TODO var SymbolAsyncDispose = Symbol.asyncDispose; var ObjectFreeze = Object.freeze; @@ -24,11 +23,6 @@ const kEmptyObject = ObjectFreeze({ __proto__: null }); var fs = Bun.fs(); -// note: this is not quite the same as how node does it -// in some cases, node swaps around arguments or makes small tweaks to the return type -// this is just better than nothing. -const notrace = "::bunternal::"; - function watch( filename: string | Buffer | URL, options: { encoding?: BufferEncoding; persistent?: boolean; recursive?: boolean; signal?: AbortSignal } = {}, @@ -102,7 +96,6 @@ function watch( }; } -let lazy_cp: any = null; // attempt to use the native code version if possible // and on MacOS, simple cases of recursive directory trees can be done in a single `clonefile()` // using filter and other options uses a lazily loaded js fallback ported from node.js @@ -112,8 +105,7 @@ function cp(src, dest, options) { throw new TypeError("options must be an object"); } if (options.dereference || options.filter || options.preserveTimestamps || options.verbatimSymlinks) { - if (!lazy_cp) lazy_cp = require("../internal/fs/cp-sync"); - return lazy_cp!(src, dest, options); + return require("../internal/fs/cp")(src, dest, options); } return fs.cp(src, dest, options.recursive, options.errorOnExist, options.force ?? true, options.mode); } @@ -149,12 +141,20 @@ class Dir { } } } + async function opendir(dir: string) { const entries = await fs.readdir(dir, { withFileTypes: true }); return new Dir(entries, dir); } -const real_export = { +const private_symbols = { + kRef, + kUnref, + kFd, + FileHandle: null, +}; + +const exports = { access: fs.access.bind(fs), appendFile: fs.appendFile.bind(fs), close: fs.close.bind(fs), @@ -213,10 +213,13 @@ const real_export = { }, constants, watch, - opendir, + + // "$data" is reuse of private symbol + // this is used to export the private symbols to 'fs.js' without making it public. + $data: private_symbols, }; -export default real_export; +export default exports; { const { @@ -234,11 +237,12 @@ export default real_export; write, writev, close, - } = real_export; + } = exports; + // Partially taken from https://github.com/nodejs/node/blob/c25878d370/lib/internal/fs/promises.js#L148 // These functions await the result so that errors propagate correctly with // async stack traces and so that the ref counting is correct. - var FileHandle = class FileHandle extends EventEmitter { + var FileHandle = (private_symbols.FileHandle = class FileHandle extends EventEmitter { constructor(fd) { super(); this[kFd] = fd ? fd : -1; @@ -440,6 +444,7 @@ export default real_export; this[kClosePromise] = SafePromisePrototypeFinally.$call(close(this[kFd]), () => { this[kClosePromise] = undefined; }); + this[kFd] = -1; } else { this[kClosePromise] = SafePromisePrototypeFinally.$call( new Promise((resolve, reject) => { @@ -472,13 +477,21 @@ export default real_export; createReadStream(options) { const fd = this[kFd]; throwEBADFIfNecessary(fs.createReadStream, fd); - return require("node:fs").createReadStream("", { fd, highWaterMark: 64 * 1024, ...(options || {}) }); + let stream = require("node:fs").createReadStream("", { + fd: this, + highWaterMark: 64 * 1024, + ...(options || {}), + }); + this[kRef](); + return stream; } createWriteStream(options) { const fd = this[kFd]; throwEBADFIfNecessary(fs.createWriteStream, fd); - return require("node:fs").createWriteStream("", { fd, ...(options || {}) }); + let stream = require("node:fs").createWriteStream("", { fd, ...(options || {}) }); + this[kRef](); + return stream; } [kTransfer]() { @@ -503,7 +516,7 @@ export default real_export; PromisePrototypeThen.$call(this.close(), this[kCloseResolve], this[kCloseReject]); } } - }; + }); } function throwEBADFIfNecessary(fn, fd) { diff --git a/test/js/node/fs/fs.test.ts b/test/js/node/fs/fs.test.ts index 14e96eb226680..bdc0b344ee7af 100644 --- a/test/js/node/fs/fs.test.ts +++ b/test/js/node/fs/fs.test.ts @@ -116,7 +116,6 @@ describe("FileHandle", () => { const chunk = await reader.read(); expect(chunk.value instanceof Uint8Array).toBe(true); reader.releaseLock(); - await stream.cancel(); }); it("FileHandle#createReadStream", async () => { From 642c63ce84e77b1f9152779851c11a07f431b575 Mon Sep 17 00:00:00 2001 From: dave caruso Date: Fri, 22 Mar 2024 14:10:01 -0700 Subject: [PATCH 19/20] awa --- src/cli/install.ps1 | 64 +++++++++++------ src/fd.zig | 3 - src/js/internal/fs/cp.ts | 8 +-- src/js/node/fs.js | 42 +++++++---- src/js/node/fs.promises.ts | 50 ++++++------- src/js/node/stream.js | 22 +----- src/main.zig | 11 +-- test/js/node/fs/fs-leak.test.js | 122 ++++++++++++++++++++++++++++++++ test/js/node/fs/fs.test.ts | 31 ++++++-- 9 files changed, 260 insertions(+), 93 deletions(-) create mode 100644 test/js/node/fs/fs-leak.test.js diff --git a/src/cli/install.ps1 b/src/cli/install.ps1 index 8fa12631920fc..3f8454485e3bc 100644 --- a/src/cli/install.ps1 +++ b/src/cli/install.ps1 @@ -7,14 +7,19 @@ param( # Skips adding the bun.exe directory to the user's %PATH% [Switch]$NoPathUpdate = $false, # Skips adding the bun to the list of installed programs - [Switch]$NoRegisterInstallation = $false + [Switch]$NoRegisterInstallation = $false, + # Skips installing powershell completions to your profile + [Switch]$NoCompletions = $false, + + # Debugging: Always download with 'Invoke-RestMethod' instead of 'curl.exe' + [Switch]$DownloadWithoutCurl = $false ); # filter out 32 bit + ARM if ($env:PROCESSOR_ARCHITECTURE -ne "AMD64") { Write-Output "Install Failed:" Write-Output "Bun for Windows is only available for x86 64-bit Windows.`n" - exit 1 + return 1 } # This corresponds to .win10_rs5 in build.zig @@ -24,7 +29,7 @@ $MinBuildName = "Windows 10 1809" $WinVer = [System.Environment]::OSVersion.Version if ($WinVer.Major -lt 10 -or ($WinVer.Major -eq 10 -and $WinVer.Build -lt $MinBuild)) { Write-Warning "Bun requires at ${MinBuildName} or newer.`n`nThe install will still continue but it may not work.`n" - exit 1 + return 1 } $ErrorActionPreference = "Stop" @@ -123,15 +128,15 @@ function Install-Bun { $openProcesses = Get-Process -Name bun | Where-Object { $_.Path -eq "${BunBin}\bun.exe" } if ($openProcesses.Count -gt 0) { Write-Output "Install Failed - An older installation exists and is open. Please close open Bun processes and try again." - exit 1 + return 1 } Write-Output "Install Failed - An unknown error occurred while trying to remove the existing installation" Write-Output $_ - exit 1 + return 1 } catch { Write-Output "Install Failed - An unknown error occurred while trying to remove the existing installation" Write-Output $_ - exit 1 + return 1 } $Target = "bun-windows-$Arch" @@ -155,17 +160,26 @@ function Install-Bun { # curl.exe is faster than PowerShell 5's 'Invoke-WebRequest' # note: 'curl' is an alias to 'Invoke-WebRequest'. so the exe suffix is required - curl.exe "-#SfLo" "$ZipPath" "$URL" - if ($LASTEXITCODE -ne 0) { - Write-Output "Install Failed - could not download $URL" - Write-Output "The command 'curl.exe $URL -o $ZipPath' exited with code ${LASTEXITCODE}`n" - exit 1 + if (-not $DownloadWithoutCurl) { + curl.exe "-#SfLo" "$ZipPath" "$URL" + } + if ($DownloadWithoutCurl -or ($LASTEXITCODE -ne 0)) { + Write-Warning "The command 'curl.exe $URL -o $ZipPath' exited with code ${LASTEXITCODE}`nTrying an alternative download method..." + try { + # Use Invoke-RestMethod instead of Invoke-WebRequest because Invoke-WebRequest breaks on + # some machines, see + Invoke-RestMethod -Uri $URL -OutFile $ZipPath + } catch { + Write-Output "Install Failed - could not download $URL" + Write-Output "The command 'Invoke-RestMethod $URL -OutFile $ZipPath' exited with code ${LASTEXITCODE}`n" + return 1 + } } if (!(Test-Path $ZipPath)) { Write-Output "Install Failed - could not download $URL" Write-Output "The file '$ZipPath' does not exist. Did an antivirus delete it?`n" - exit 1 + return 1 } try { @@ -179,7 +193,7 @@ function Install-Bun { } catch { Write-Output "Install Failed - could not unzip $ZipPath" Write-Error $_ - exit 1 + return 1 } Move-Item "${BunBin}\$Target\bun.exe" "${BunBin}\bun.exe" -Force @@ -192,14 +206,14 @@ function Install-Bun { if ($IsBaseline) { Write-Output "Install Failed - bun.exe (baseline) is not compatible with your CPU.`n" Write-Output "Please open a GitHub issue with your CPU model:`nhttps://github.com/oven-sh/bun/issues/new/choose`n" - exit 1 + return 1 } Write-Output "Install Failed - bun.exe is not compatible with your CPU. This should have been detected before downloading.`n" Write-Output "Attempting to download bun.exe (baseline) instead.`n" Install-Bun -Version $Version -ForceBaseline $True - exit 1 + return 1 } # '-1073741515' was spotted in the wild, but not clearly documented as a status code: # https://discord.com/channels/876711213126520882/1149339379446325248/1205194965383250081 @@ -209,27 +223,34 @@ function Install-Bun { Write-Output "Install Failed - You are missing a DLL required to run bun.exe" Write-Output "This can be solved by installing the Visual C++ Redistributable from Microsoft:`nSee https://learn.microsoft.com/cpp/windows/latest-supported-vc-redist`nDirect Download -> https://aka.ms/vs/17/release/vc_redist.x64.exe`n`n" Write-Output "The command '${BunBin}\bun.exe --revision' exited with code ${LASTEXITCODE}`n" - exit 1 + return 1 } if ($LASTEXITCODE -ne 0) { Write-Output "Install Failed - could not verify bun.exe" Write-Output "The command '${BunBin}\bun.exe --revision' exited with code ${LASTEXITCODE}`n" - exit 1 + return 1 } try { $env:IS_BUN_AUTO_UPDATE = "1" + # TODO: When powershell completions are added, make this switch actually do something + if ($NoCompletions) { + $env:BUN_NO_INSTALL_COMPLETIONS = "1" + } + # This completions script in general will install some extra stuff, mainly the `bunx` link. + # It also installs completions. $output = "$(& "${BunBin}\bun.exe" completions 2>&1)" if ($LASTEXITCODE -ne 0) { Write-Output $output Write-Output "Install Failed - could not finalize installation" Write-Output "The command '${BunBin}\bun.exe completions' exited with code ${LASTEXITCODE}`n" - exit 1 + return 1 } } catch { - # it is possible on powershell 5 that an error + # it is possible on powershell 5 that an error happens, but it is probably fine? } $env:IS_BUN_AUTO_UPDATE = $null + $env:BUN_NO_INSTALL_COMPLETIONS = $null $DisplayVersion = if ($BunRevision -like "*-canary.*") { "${BunRevision}" @@ -243,7 +264,7 @@ function Install-Bun { Write-Output "${C_GREEN}Bun ${DisplayVersion} was installed successfully!${C_RESET}" Write-Output "The binary is located at ${BunBin}\bun.exe`n" - Write-Warning "Bun for Windows is currently experimental and must not be used in production.`nFor a more stable experience, install Bun within WSL:`nhttps://bun.sh/docs/installation`n" + Write-Warning "Bun for Windows is currently experimental.`nFor a more stable experience, install Bun within WSL:`nhttps://bun.sh/docs/installation`n" $hasExistingOther = $false; try { @@ -277,6 +298,7 @@ function Install-Bun { if (-not $NoPathUpdate) { $Path += $BunBin Write-Env -Key 'Path' -Value ($Path -join ';') + $env:PATH = $Path; } else { Write-Output "Skipping adding '${BunBin}' to the user's %PATH%`n" } @@ -284,6 +306,8 @@ function Install-Bun { Write-Output "To get started, restart your terminal/editor, then type `"bun`"`n" } + + $LASTEXITCODE = 0; } Install-Bun -Version $Version -ForceBaseline $ForceBaseline diff --git a/src/fd.zig b/src/fd.zig index bc691cf5a7b46..4873be9f0dd3d 100644 --- a/src/fd.zig +++ b/src/fd.zig @@ -245,9 +245,6 @@ pub const FDImpl = packed struct { .windows => result: { switch (this.kind) { .uv => { - std.debug.getStderrMutex().lock(); - std.debug.dumpCurrentStackTrace(@returnAddress()); - std.debug.getStderrMutex().unlock(); var req: libuv.fs_t = libuv.fs_t.uninitialized; defer req.deinit(); const rc = libuv.uv_fs_close(libuv.Loop.get(), &req, this.value.as_uv, null); diff --git a/src/js/internal/fs/cp.ts b/src/js/internal/fs/cp.ts index 4e361f7934b21..6c3a0170bf457 100644 --- a/src/js/internal/fs/cp.ts +++ b/src/js/internal/fs/cp.ts @@ -17,7 +17,6 @@ const { chmod, copyFile, lstat, mkdir, opendir, readlink, stat, symlink, unlink, utimes } = require("node:fs/promises"); const { dirname, isAbsolute, join, parse, resolve, sep } = require("node:path"); -const SafePromiseAll = Promise.all; const PromisePrototypeThen = Promise.prototype.then; const PromiseReject = Promise.reject; const ArrayPrototypeFilter = Array.prototype.filter; @@ -82,7 +81,7 @@ function areIdentical(srcStat, destStat) { function getStats(src, dest, opts) { const statFunc = opts.dereference ? file => stat(file, { bigint: true }) : file => lstat(file, { bigint: true }); - return SafePromiseAll([ + return Promise.all([ statFunc(src), PromisePrototypeThen.$call(statFunc(dest), undefined, err => { if (err.code === "ENOENT") return null; @@ -100,7 +99,7 @@ async function checkParentDir(destStat, src, dest, opts) { } function pathExists(dest) { - return PromisePrototypeThen( + return PromisePrototypeThen.$call( stat(dest), () => true, err => (err.code === "ENOENT" ? false : PromiseReject(err)), @@ -137,7 +136,8 @@ async function checkParentPaths(src, srcStat, dest) { return checkParentPaths(src, srcStat, destParent); } -const normalizePathToArray = path => ArrayPrototypeFilter.$call(StringPrototypeSplit(resolve(path), sep), Boolean); +const normalizePathToArray = path => + ArrayPrototypeFilter.$call(StringPrototypeSplit.$call(resolve(path), sep), Boolean); // Return true if dest is a subdir of src, otherwise false. // It only checks the path strings. diff --git a/src/js/node/fs.js b/src/js/node/fs.js index 3c185db9aa145..854239666e69c 100644 --- a/src/js/node/fs.js +++ b/src/js/node/fs.js @@ -512,6 +512,7 @@ var defaultReadStreamOptions = { }; let { FileHandle, kRef, kUnref, kFd } = promises.$data; +let kHandle = Symbol("kHandle"); var ReadStreamClass; @@ -684,7 +685,6 @@ ReadStream = (function (InternalReadStream) { } _destroy(err, cb) { - super._destroy(err, cb); try { this[readStreamPathFastPathSymbol] = false; var handle = this.#handle; @@ -692,16 +692,17 @@ ReadStream = (function (InternalReadStream) { handle[kUnref](); this.fd = null; this.#handle = null; + super._destroy(err, cb); return; } var fd = this.fd; if (!fd) { - cb(err); + super._destroy(err, cb); } else { $assert(this.#fs); this.#fs.close(fd, er => { - cb(er || err); + super._destroy(er || err, cb); }); this.fd = null; } @@ -912,11 +913,22 @@ var WriteStreamClass = (WriteStream = function WriteStream(path, options = defau } = options; var tempThis = {}; + var handle = null; if (fd != null) { if (typeof fd !== "number") { - throw new Error("Expected options.fd to be a number"); + if (fd instanceof FileHandle) { + tempThis.fd = fd[kFd]; + if (tempThis.fd < 0) { + throw new Error("Expected a valid file descriptor"); + } + fd[kRef](); + handle = fd; + } else { + throw new TypeError("Expected options.fd to be a number or FileHandle"); + } + } else { + tempThis.fd = fd; } - tempThis.fd = fd; tempThis[_writeStreamPathFastPathSymbol] = false; } else if (typeof path === "string") { if (path.length === 0) { @@ -973,6 +985,7 @@ var WriteStreamClass = (WriteStream = function WriteStream(path, options = defau this.start = start; this[_fs] = fs; + this[kHandle] = handle; this.flags = flags; this.mode = mode; this.bytesWritten = 0; @@ -994,6 +1007,7 @@ var WriteStreamClass = (WriteStream = function WriteStream(path, options = defau return this; }); + const NativeWritable = Stream.NativeWritable; const WriteStreamPrototype = (WriteStream.prototype = Object.create(NativeWritable.prototype)); @@ -1068,10 +1082,19 @@ function WriteStream_handleWrite(er, bytes) { function WriteStream_internalClose(err, cb) { this[_writeStreamPathFastPathSymbol] = false; + console.log("WriteStream_internalClose", this); + var handle = this[kHandle]; + if (handle) { + handle[kUnref](); + this.fd = null; + this[kHandle] = null; + NativeWritable.prototype._destroy.$apply(this, err, cb); + return; + } var fd = this.fd; this[_fs].close(fd, er => { this.fd = null; - cb(err || er); + NativeWritable.prototype._destroy.$apply(this, er || err, cb); }); } @@ -1088,7 +1111,7 @@ WriteStreamPrototype._construct = function _construct(callback) { WriteStreamPrototype._destroy = function _destroy(err, cb) { if (this.fd === null) { - return cb(err); + return NativeWritable.prototype._destroy.$apply(this, err, cb); } if (this[kIoDone]) { @@ -1153,10 +1176,6 @@ WriteStreamPrototype.end = function end(chunk, encoding, cb) { return NativeWritable.prototype.end.$call(this, chunk, encoding, cb, native); }; -WriteStreamPrototype._destroy = function _destroy(err, cb) { - this.close(err, cb); -}; - function WriteStream_errorOrDestroy(err) { var { _readableState: r = { destroyed: false, autoDestroy: false }, @@ -1192,7 +1211,6 @@ Object.defineProperties(fs, { }); // lol -// @ts-ignore realpath.native = realpath; realpathSync.native = realpathSync; diff --git a/src/js/node/fs.promises.ts b/src/js/node/fs.promises.ts index 11bb73ccde54c..4519f1c7d808a 100644 --- a/src/js/node/fs.promises.ts +++ b/src/js/node/fs.promises.ts @@ -5,7 +5,7 @@ const constants = $processBindingConstants.fs; const EventEmitter = require("node:events"); var PromisePrototypeThen = Promise.prototype.then; -var SafePromisePrototypeFinally = Promise.prototype.finally; //TODO +var PromisePrototypeFinally = Promise.prototype.finally; //TODO var SymbolAsyncDispose = Symbol.asyncDispose; var ObjectFreeze = Object.freeze; @@ -177,8 +177,7 @@ const exports = { mkdir: fs.mkdir.bind(fs), mkdtemp: fs.mkdtemp.bind(fs), open: async (path, flags, mode) => { - const fd = await fs.open(path, flags, mode); - return new FileHandle(fd); + return new FileHandle(await fs.open(path, flags, mode)); }, read: fs.read.bind(fs), write: fs.write.bind(fs), @@ -430,23 +429,23 @@ export default exports; } } - async close() { - if (this[kFd] === -1) { - return; + close = () => { + const fd = this[kFd]; + if (fd === -1) { + return Promise.resolve(); } if (this[kClosePromise]) { return this[kClosePromise]; } - this[kRefs]--; - if (this[kRefs] === 0) { - this[kClosePromise] = SafePromisePrototypeFinally.$call(close(this[kFd]), () => { + if (--this[kRefs] === 0) { + this[kFd] = -1; + this[kClosePromise] = PromisePrototypeFinally.$call(close(fd), () => { this[kClosePromise] = undefined; }); - this[kFd] = -1; } else { - this[kClosePromise] = SafePromisePrototypeFinally.$call( + this[kClosePromise] = PromisePrototypeFinally.$call( new Promise((resolve, reject) => { this[kCloseResolve] = resolve; this[kCloseReject] = reject; @@ -461,7 +460,7 @@ export default exports; this.emit("close"); return this[kClosePromise]; - } + }; async [SymbolAsyncDispose]() { return this.close(); @@ -474,24 +473,23 @@ export default exports; return Bun.file(fd).stream(); } - createReadStream(options) { + createReadStream(options = kEmptyObject) { const fd = this[kFd]; throwEBADFIfNecessary(fs.createReadStream, fd); - let stream = require("node:fs").createReadStream("", { + return require("node:fs").createReadStream("", { fd: this, highWaterMark: 64 * 1024, - ...(options || {}), + ...options, }); - this[kRef](); - return stream; } - createWriteStream(options) { + createWriteStream(options = kEmptyObject) { const fd = this[kFd]; throwEBADFIfNecessary(fs.createWriteStream, fd); - let stream = require("node:fs").createWriteStream("", { fd, ...(options || {}) }); - this[kRef](); - return stream; + return require("node:fs").createWriteStream("", { + fd: this, + ...options, + }); } [kTransfer]() { @@ -507,13 +505,15 @@ export default exports; } [kRef]() { + console.log("ref"); this[kRefs]++; + console.log(this[kRefs]); } [kUnref]() { - const refCount = this[kRefs]--; - if (refCount === 1) { - PromisePrototypeThen.$call(this.close(), this[kCloseResolve], this[kCloseReject]); + if (--this[kRefs] === 0) { + this[kFd] = -1; + this.close().$then(this[kCloseResolve], this[kCloseReject]); } } }); @@ -522,7 +522,7 @@ export default exports; function throwEBADFIfNecessary(fn, fd) { if (fd === -1) { // eslint-disable-next-line no-restricted-syntax - const err = new Error("file closed"); + const err = new Error("Bad file descriptor"); err.code = "EBADF"; err.name = "SystemError"; err.syscall = fn.name; diff --git a/src/js/node/stream.js b/src/js/node/stream.js index 80726a8d6ca8b..8c75fbdf8a2f7 100644 --- a/src/js/node/stream.js +++ b/src/js/node/stream.js @@ -5316,23 +5316,6 @@ function createNativeStreamReadable(nativeType, Readable) { } return this.#internalRead(this.#getRemainingChunk(maxToRead), ptr); - // const internalReadRes = this.#internalRead( - // this.#getRemainingChunk(), - // ptr, - // ); - // // REVERT ME - // const wrap = new Promise((resolve) => { - // if (!this.internalReadRes?.then) { - // debug("internalReadRes not promise"); - // resolve(internalReadRes); - // return; - // } - // internalReadRes.then((result) => { - // debug("internalReadRes done"); - // resolve(result); - // }); - // }); - // return wrap; } #internalConstruct(ptr) { @@ -5509,7 +5492,6 @@ function NativeWritable(pathOrFdOrSink, options = {}) { this[_native] = true; this._construct = NativeWritable_internalConstruct; - this._destroy = NativeWritable_internalDestroy; this._final = NativeWritable_internalFinal; this._write = NativeWritablePrototypeWrite; @@ -5586,7 +5568,7 @@ NativeWritable.prototype.end = function end(chunk, encoding, cb, native) { return WritablePrototypeEnd.$call(this, chunk, encoding, cb, native ?? this[_native]); }; -function NativeWritable_internalDestroy(error, cb) { +NativeWritable.prototype._destroy = function (error, cb) { const w = this._writableState; const r = this._readableState; @@ -5604,7 +5586,7 @@ function NativeWritable_internalDestroy(error, cb) { if (w?.closeEmitted || r?.closeEmitted) { this.emit("close"); } -} +}; function NativeWritable_internalFinal(cb) { var sink = this[_fileSink]; diff --git a/src/main.zig b/src/main.zig index fd48f194cae9c..ba06ced75ec15 100644 --- a/src/main.zig +++ b/src/main.zig @@ -2,8 +2,12 @@ const std = @import("std"); const builtin = @import("builtin"); pub const build_options = @import("build_options"); -const panicky = @import("./panic_handler.zig"); -const MainPanicHandler = panicky.NewPanicHandler(std.builtin.default_panic); +const bun = @import("root").bun; +const Output = bun.Output; +const Environment = bun.Environment; + +const panic_handler = @import("./panic_handler.zig"); +const MainPanicHandler = panic_handler.NewPanicHandler(std.builtin.default_panic); pub const io_mode = .blocking; @@ -26,9 +30,6 @@ extern fn SetConsoleMode(console_handle: *anyopaque, mode: u32) u32; extern fn SetStdHandle(nStdHandle: u32, hHandle: *anyopaque) u32; pub extern "kernel32" fn SetConsoleCP(wCodePageID: std.os.windows.UINT) callconv(std.os.windows.WINAPI) std.os.windows.BOOL; pub fn main() void { - const bun = @import("root").bun; - const Output = bun.Output; - const Environment = bun.Environment; // This should appear before we make any calls at all to libuv. // So it's safest to put it very early in the main function. if (Environment.isWindows) { diff --git a/test/js/node/fs/fs-leak.test.js b/test/js/node/fs/fs-leak.test.js new file mode 100644 index 0000000000000..8e07348fdef54 --- /dev/null +++ b/test/js/node/fs/fs-leak.test.js @@ -0,0 +1,122 @@ +// This file is a .cjs file so you can run it in node+jest to verify node behaves exactly the same. +const fs = require("fs"); +const { tmpdir, devNull } = require("os"); + +function getMaxFd() { + const dev_null = fs.openSync(devNull, "r"); + fs.closeSync(dev_null); + return dev_null; +} + +test("createWriteStream does not leak file descriptors", async () => { + let start = getMaxFd(); + const path = `${tmpdir()}/${Date.now()}.leakTest.txt`; + + await new Promise((resolve, reject) => { + const stream = fs.createWriteStream(path, {}); + + stream.on("error", reject); + + stream.on("open", () => { + for (let i = 0; i < 100; i++) { + stream.write("hello world"); + } + stream.end(); + }); + + stream.on("close", () => { + resolve(); + }); + }); + + // If this is larger than the start value, it means that the file descriptor was not closed + expect(getMaxFd()).toBe(start); +}); + +test("createReadStream does not leak file descriptors", async () => { + let start = getMaxFd(); + const path = `${tmpdir()}/${Date.now()}.leakTest.txt`; + fs.writeFileSync(path, "hello world\n".repeat(1000)); + + let n_bytes = 0; + + await new Promise((resolve, reject) => { + const stream = fs.createReadStream(path, {}); + + stream.on("error", reject); + + stream.on("data", chunk => { + n_bytes += chunk.length; + }); + + stream.on("close", () => { + resolve(); + }); + }); + + // If this is larger than the start value, it means that the file descriptor was not closed + expect(getMaxFd()).toBe(start); + expect(n_bytes).toBe("hello world\n".repeat(1000).length); +}); + +test("createWriteStream file handle does not leak file descriptors", async () => { + let start = getMaxFd(); + const path = `${tmpdir()}/${Date.now()}.leakTest.txt`; + + const fd = await fs.promises.open(path, "w"); + + await new Promise((resolve, reject) => { + const stream = fd.createWriteStream({}); + + stream.on("error", reject); + + stream.on("open", () => { + for (let i = 0; i < 100; i++) { + stream.write("hello world"); + } + stream.end(); + }); + + stream.on("close", () => { + resolve(); + }); + }); + + console.log("fd", fd); + await fd.close(); + await fd.close(); + + // If this is larger than the start value, it means that the file descriptor was not closed + expect(getMaxFd()).toBe(start); +}); + +test("createReadStream file handle does not leak file descriptors", async () => { + let start = getMaxFd(); + const path = `${tmpdir()}/${Date.now()}.leakTest.txt`; + fs.writeFileSync(path, "hello world\n".repeat(1000)); + + let n_bytes = 0; + + const fd = await fs.promises.open(path, "r"); + + await new Promise((resolve, reject) => { + const stream = fd.createReadStream({}); + + stream.on("error", reject); + + stream.on("data", chunk => { + n_bytes += chunk.length; + }); + + stream.on("close", () => { + resolve(); + }); + }); + + await fd.close(); + await fd.close(); + + // If this is larger than the start value, it means that the file descriptor was not closed + expect(getMaxFd()).toBe(start); + expect(n_bytes).toBe("hello world\n".repeat(1000).length); +}); diff --git a/test/js/node/fs/fs.test.ts b/test/js/node/fs/fs.test.ts index bdc0b344ee7af..84dfcbf374bb7 100644 --- a/test/js/node/fs/fs.test.ts +++ b/test/js/node/fs/fs.test.ts @@ -146,8 +146,6 @@ describe("FileHandle", () => { { await using fd = await fs.promises.open(path, "w"); const stream = fd.createWriteStream(); - stream.write("Test file written successfully"); - stream.end(); await new Promise((resolve, reject) => { stream.on("error", e => { @@ -155,9 +153,34 @@ describe("FileHandle", () => { }); stream.on("finish", () => { - expect(readFileSync(path, "utf8")).toBe("Test file written successfully"); resolve(true); }); + + stream.write("Test file written successfully"); + stream.end(); + }); + } + + expect(readFileSync(path, "utf8")).toBe("Test file written successfully"); + }); + + it("FileHandle#createWriteStream fixture 2", async () => { + const path = `${tmpdir()}/${Date.now()}.createWriteStream.txt`; + { + await using fd = await fs.promises.open(path, "w"); + const stream = fd.createWriteStream(); + + await new Promise((resolve, reject) => { + stream.on("error", e => { + reject(e); + }); + + stream.on("close", () => { + resolve(true); + }); + + stream.write("Test file written successfully"); + stream.end(); }); } @@ -1120,7 +1143,7 @@ describe("readFileSync", () => { expect(text).toBe("File read successfully"); }); - it.skipIf(isWindows)("works with special files in the filesystem", () => { + it.skipIf(isWindows)("works with special posix files in the filesystem", () => { const text = readFileSync("/dev/null", "utf8"); gc(); expect(text).toBe(""); From fbdb8625993a362a681acc5f72b2f06d4f483a89 Mon Sep 17 00:00:00 2001 From: dave caruso Date: Fri, 22 Mar 2024 14:50:06 -0700 Subject: [PATCH 20/20] fs test pass --- CMakeLists.txt | 70 ++++++++++++++----------- scripts/env.sh | 3 ++ src/cli/install_completions_command.zig | 4 +- src/js/node/fs.js | 1 - src/js/node/fs.promises.ts | 2 - src/js/node/wasi.js | 2 - test/js/node/fs/fs.test.ts | 14 ++++- 7 files changed, 56 insertions(+), 40 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 37edc34bd83d7..3f2f5a4535c1f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -5,37 +5,6 @@ cmake_policy(SET CMP0067 NEW) set(Bun_VERSION "1.0.35") set(WEBKIT_TAG 089023cc9078b3aa173869fd6685f3e7bed2a994) -if(APPLE AND DEFINED ENV{CI}) - if(ARCH STREQUAL "x86_64") - set(CMAKE_OSX_DEPLOYMENT_TARGET "10.14") - else() - set(CMAKE_OSX_DEPLOYMENT_TARGET "11.0") - endif() -endif() - -if(APPLE AND NOT CMAKE_OSX_DEPLOYMENT_TARGET) - execute_process(COMMAND xcrun --show-sdk-path OUTPUT_VARIABLE SDKROOT) - string(STRIP ${SDKROOT} SDKROOT) - message(STATUS "Using SDKROOT: ${SDKROOT}") - SET(CMAKE_OSX_SYSROOT ${SDKROOT}) - - execute_process(COMMAND xcrun --sdk macosx --show-sdk-version OUTPUT_VARIABLE MACOSX_DEPLOYMENT_TARGET) - string(STRIP ${MACOSX_DEPLOYMENT_TARGET} MACOSX_DEPLOYMENT_TARGET) - set(CMAKE_OSX_DEPLOYMENT_TARGET ${MACOSX_DEPLOYMENT_TARGET}) - - # Check if current version of macOS is less than the deployment target and if so, raise an error - execute_process(COMMAND sw_vers -productVersion OUTPUT_VARIABLE MACOS_VERSION) - string(STRIP ${MACOS_VERSION} MACOS_VERSION) - - if(MACOS_VERSION VERSION_LESS ${MACOSX_DEPLOYMENT_TARGET}) - message(WARNING "\nThe current version of macOS (${MACOS_VERSION}) is less than the deployment target (${MACOSX_DEPLOYMENT_TARGET}).\nThis makes icucore fail to run at start.\nTo fix this, please either:\n- Upgrade to the latest version of macOS\n- Use `xcode-select` to switch to an SDK version <= ${MACOS_VERSION}") - endif() -endif() - -if(APPLE) - message(STATUS "Building for macOS v${CMAKE_OSX_DEPLOYMENT_TARGET}") -endif() - set(BUN_WORKDIR "${CMAKE_CURRENT_BINARY_DIR}") message(STATUS "Configuring Bun ${Bun_VERSION} in ${BUN_WORKDIR}") @@ -80,6 +49,45 @@ elseif(CMAKE_BUILD_TYPE STREQUAL "Release") endif() endif() +# --- MacOS SDK --- +if(APPLE AND DEFINED ENV{CI}) + if(ARCH STREQUAL "x86_64") + set(CMAKE_OSX_DEPLOYMENT_TARGET "10.14") + else() + set(CMAKE_OSX_DEPLOYMENT_TARGET "11.0") + endif() +endif() + +if(APPLE AND NOT CMAKE_OSX_DEPLOYMENT_TARGET) + execute_process(COMMAND xcrun --show-sdk-path OUTPUT_VARIABLE SDKROOT) + string(STRIP ${SDKROOT} SDKROOT) + message(STATUS "MacOS SDK path: ${SDKROOT}") + SET(CMAKE_OSX_SYSROOT ${SDKROOT}) + + execute_process(COMMAND xcrun --sdk macosx --show-sdk-version OUTPUT_VARIABLE MACOSX_DEPLOYMENT_TARGET) + string(STRIP ${MACOSX_DEPLOYMENT_TARGET} MACOSX_DEPLOYMENT_TARGET) + set(CMAKE_OSX_DEPLOYMENT_TARGET ${MACOSX_DEPLOYMENT_TARGET}) + + # Check if current version of macOS is less than the deployment target and if so, raise an error + execute_process(COMMAND sw_vers -productVersion OUTPUT_VARIABLE MACOS_VERSION) + string(STRIP ${MACOS_VERSION} MACOS_VERSION) + + if(MACOS_VERSION VERSION_LESS ${MACOSX_DEPLOYMENT_TARGET}) + message(WARNING + "The current version of macOS (${MACOS_VERSION}) is less than the deployment target (${MACOSX_DEPLOYMENT_TARGET}).\n" + "The build will be incompatible with your current device due to mismatches in `icucore` versions.\n" + "To fix this, please either:\n" + " - Upgrade to at least macOS ${MACOSX_DEPLOYMENT_TARGET}\n" + " - Use `xcode-select` to switch to an SDK version <= ${MACOS_VERSION}\n" + " - Set CMAKE_OSX_DEPLOYMENT_TARGET=${MACOS_VERSION} (make sure to build all dependencies with this variable set too)" + ) + endif() +endif() + +if(APPLE) + message(STATUS "Building for macOS v${CMAKE_OSX_DEPLOYMENT_TARGET}") +endif() + # --- LLVM --- # This detection is a little overkill, but it ensures that the set LLVM_VERSION matches under # any case possible. Sorry for the complexity... diff --git a/scripts/env.sh b/scripts/env.sh index b051e4464e674..e6df269a9f4c7 100755 --- a/scripts/env.sh +++ b/scripts/env.sh @@ -43,4 +43,7 @@ mkdir -p $BUN_DEPS_OUT_DIR if [[ "${BASH_SOURCE[0]}" == "${0}" ]]; then echo "C Compiler: ${CC}" echo "C++ Compiler: ${CXX}" + if [[ $(uname -s) == 'Darwin' ]]; then + echo "OSX Deployment Target: ${CMAKE_OSX_DEPLOYMENT_TARGET}" + fi fi diff --git a/src/cli/install_completions_command.zig b/src/cli/install_completions_command.zig index 362f922b46f2c..c24eabb49390b 100644 --- a/src/cli/install_completions_command.zig +++ b/src/cli/install_completions_command.zig @@ -140,13 +140,13 @@ pub const InstallCompletionsCommand = struct { fn installUninstallerWindows() !void { // This uninstaller file is only written if the current exe is within a path - // like `\bun\bin\.exe` so that it probably only runs when the + // like `bun\bin\.exe` so that it probably only runs when the // powershell `install.ps1` was used to install. const image_path = bun.windows.exePathW(); const image_dirname = image_path[0..(std.mem.lastIndexOfScalar(u16, image_path, '\\') orelse unreachable)]; - if (!std.mem.endsWith(u16, image_dirname, comptime bun.strings.literal(u16, "\\bun\\bin"))) + if (!std.mem.endsWith(u16, image_dirname, comptime bun.strings.literal(u16, "bun\\bin"))) return; const content = @embedFile("uninstall.ps1"); diff --git a/src/js/node/fs.js b/src/js/node/fs.js index 854239666e69c..caaec55eef5d4 100644 --- a/src/js/node/fs.js +++ b/src/js/node/fs.js @@ -1082,7 +1082,6 @@ function WriteStream_handleWrite(er, bytes) { function WriteStream_internalClose(err, cb) { this[_writeStreamPathFastPathSymbol] = false; - console.log("WriteStream_internalClose", this); var handle = this[kHandle]; if (handle) { handle[kUnref](); diff --git a/src/js/node/fs.promises.ts b/src/js/node/fs.promises.ts index 4519f1c7d808a..5296797fb64a4 100644 --- a/src/js/node/fs.promises.ts +++ b/src/js/node/fs.promises.ts @@ -505,9 +505,7 @@ export default exports; } [kRef]() { - console.log("ref"); this[kRefs]++; - console.log(this[kRefs]); } [kUnref]() { diff --git a/src/js/node/wasi.js b/src/js/node/wasi.js index c8b051cd3ec0c..e96485f838051 100644 --- a/src/js/node/wasi.js +++ b/src/js/node/wasi.js @@ -6,8 +6,6 @@ // // Eventually we will implement this in native code, but this is just a quick hack to get WASI working. -/** **/ -// constants is injected into the top of this file const nodeFsConstants = $processBindingConstants.fs; var __getOwnPropNames = Object.getOwnPropertyNames; diff --git a/test/js/node/fs/fs.test.ts b/test/js/node/fs/fs.test.ts index 84dfcbf374bb7..5045023653e45 100644 --- a/test/js/node/fs/fs.test.ts +++ b/test/js/node/fs/fs.test.ts @@ -2169,10 +2169,16 @@ describe("fs/promises", () => { }, 100000); for (let withFileTypes of [false, true] as const) { - const iterCount = 100; + const warmup = 1; + const iterCount = 200; + const full = resolve(import.meta.dir, "../"); + const doIt = async () => { + for (let i = 0; i < warmup; i++) { + await promises.readdir(full, { withFileTypes }); + } + const maxFD = getMaxFD(); - const full = resolve(import.meta.dir, "../"); const pending = new Array(iterCount); for (let i = 0; i < iterCount; i++) { @@ -2890,6 +2896,10 @@ it("fs.ReadStream allows functions", () => { }); describe.if(isWindows)("windows path handling", () => { + // dont call `it` because these paths wont make sense + // the `it` in this branch makes something be printed on posix' + if (!isWindows) return it("works", () => {}); + const file = import.meta.path.slice(3); const drive = import.meta.path[0]; const filenames = [