Don't use arena in node:fs (#5863)

Co-authored-by: Jarred Sumner <709451+Jarred-Sumner@users.noreply.github.com>
oven-sh · Sep 22, 2023 · e34ff61 · e34ff61
1 parent 8684a59
commit e34ff61
Show file tree

Hide file tree

Showing 5 changed files with 49 additions and 8 deletions.
diff --git a/bench/snippets/rmdir.mjs b/bench/snippets/rmdir.mjs
@@ -0,0 +1,22 @@
+import { tmpdir } from "node:os";
+import { promises, existsSync, mkdirSync } from "node:fs";
+const count = 1024 * 12;
+
+var queue = new Array(count);
+var paths = new Array(count);
+for (let i = 0; i < count; i++) {
+  const path = `${tmpdir()}/${Date.now()}.rm.dir${i}`;
+  try {
+    mkdirSync(path);
+  } catch (e) {}
+  paths[i] = path;
+  queue[i] = promises.rmdir(path);
+}
+
+await Promise.all(queue);
+
+for (let i = 0; i < count; i++) {
+  if (existsSync(paths[i])) {
+    throw new Error(`Path ${paths[i]} was not removed`);
+  }
+}
diff --git a/src/bun.js/node/node_fs.zig b/src/bun.js/node/node_fs.zig
@@ -1074,7 +1074,7 @@ pub const Arguments = struct {
         }
 
         pub fn fromJS(ctx: JSC.C.JSContextRef, arguments: *ArgumentsSlice, exception: JSC.C.ExceptionRef) ?Stat {
-            const path = PathLike.fromJSWithAllocator(ctx, arguments, bun.default_allocator, exception) orelse {
+            const path = PathLike.fromJS(ctx, arguments, exception) orelse {
                 if (exception.* == null) {
                     JSC.throwInvalidArguments(
                         "path must be a string or TypedArray",
@@ -1649,7 +1649,7 @@ pub const Arguments = struct {
         }
 
         pub fn fromJS(ctx: JSC.C.JSContextRef, arguments: *ArgumentsSlice, exception: JSC.C.ExceptionRef) ?Readdir {
-            const path = PathLike.fromJSWithAllocator(ctx, arguments, bun.default_allocator, exception) orelse {
+            const path = PathLike.fromJS(ctx, arguments, exception) orelse {
                 if (exception.* == null) {
                     JSC.throwInvalidArguments(
                         "path must be a string or TypedArray",
@@ -2917,7 +2917,7 @@ pub const Arguments = struct {
         }
 
         pub fn fromJS(ctx: JSC.C.JSContextRef, arguments: *ArgumentsSlice, exception: JSC.C.ExceptionRef) ?CopyFile {
-            const src = PathLike.fromJSWithAllocator(ctx, arguments, bun.default_allocator, exception) orelse {
+            const src = PathLike.fromJS(ctx, arguments, exception) orelse {
                 if (exception.* == null) {
                     JSC.throwInvalidArguments(
                         "src must be a string or buffer",
@@ -2931,7 +2931,7 @@ pub const Arguments = struct {
 
             if (exception.* != null) return null;
 
-            const dest = PathLike.fromJSWithAllocator(ctx, arguments, bun.default_allocator, exception) orelse {
+            const dest = PathLike.fromJS(ctx, arguments, exception) orelse {
                 src.deinit();
 
                 if (exception.* == null) {
@@ -2981,7 +2981,7 @@ pub const Arguments = struct {
         }
 
         pub fn fromJS(ctx: JSC.C.JSContextRef, arguments: *ArgumentsSlice, exception: JSC.C.ExceptionRef) ?Cp {
-            const src = PathLike.fromJSWithAllocator(ctx, arguments, bun.default_allocator, exception) orelse {
+            const src = PathLike.fromJS(ctx, arguments, exception) orelse {
                 if (exception.* == null) {
                     JSC.throwInvalidArguments(
                         "src must be a string or buffer",
@@ -2995,7 +2995,7 @@ pub const Arguments = struct {
 
             if (exception.* != null) return null;
 
-            const dest = PathLike.fromJSWithAllocator(ctx, arguments, bun.default_allocator, exception) orelse {
+            const dest = PathLike.fromJS(ctx, arguments, exception) orelse {
                 defer src.deinit();
                 if (exception.* == null) {
                     JSC.throwInvalidArguments(

diff --git a/src/bun.js/node/node_fs_stat_watcher.zig b/src/bun.js/node/node_fs_stat_watcher.zig
@@ -198,7 +198,7 @@ pub const StatWatcher = struct {
 
         pub fn fromJS(ctx: JSC.C.JSContextRef, arguments: *ArgumentsSlice, exception: JSC.C.ExceptionRef) ?Arguments {
             const vm = ctx.vm();
-            const path = PathLike.fromJS(ctx, arguments, exception) orelse {
+            const path = PathLike.fromJSWithAllocator(ctx, arguments, bun.default_allocator, exception) orelse {
                 if (exception.* == null) {
                     JSC.throwInvalidArguments(
                         "filename must be a string or TypedArray",

diff --git a/src/bun.js/node/types.zig b/src/bun.js/node/types.zig
@@ -802,7 +802,7 @@ pub const PathLike = union(Tag) {
     }
 
     pub fn fromJS(ctx: JSC.C.JSContextRef, arguments: *ArgumentsSlice, exception: JSC.C.ExceptionRef) ?PathLike {
-        return fromJSWithAllocator(ctx, arguments, arguments.arena.allocator(), exception);
+        return fromJSWithAllocator(ctx, arguments, bun.default_allocator, exception);
     }
     pub fn fromJSWithAllocator(ctx: JSC.C.JSContextRef, arguments: *ArgumentsSlice, allocator: std.mem.Allocator, exception: JSC.C.ExceptionRef) ?PathLike {
         const arg = arguments.next() orelse return null;

diff --git a/test/js/node/fs/fs.test.ts b/test/js/node/fs/fs.test.ts
@@ -1062,6 +1062,25 @@ describe("rmdir", () => {
       done();
     });
   });
+
+  it("removes a dir x 512", async () => {
+    var queue = new Array(512);
+    var paths = new Array(512);
+    for (let i = 0; i < 512; i++) {
+      const path = `${tmpdir()}/${Date.now()}.rm.dir${i}`;
+      try {
+        mkdirSync(path);
+      } catch (e) {}
+      paths[i] = path;
+      queue[i] = promises.rmdir(path);
+    }
+
+    await Promise.all(queue);
+
+    for (let i = 0; i < 512; i++) {
+      expect(existsSync(paths[i])).toBe(false);
+    }
+  });
   it("does not remove a dir with a file in it", async () => {
     const path = `${tmpdir()}/${Date.now()}.rm.dir`;
     try {