fs: buffer dir entries in opendir()

benchmark/fs/bench-opendir.js

@@ -0,0 +1,51 @@
+'use strict';
+
+const common = require('../common');
+const fs = require('fs');
+const path = require('path');
+
+const bench = common.createBenchmark(main, {
+  n: [100],
+  dir: [ 'lib', 'test/parallel'],
+  mode: [ 'async', 'sync', 'callback' ]
+});
+
+async function main({ n, dir, mode }) {
+  const fullPath = path.resolve(__dirname, '../../', dir);
+
+  bench.start();
+
+  let counter = 0;
+  for (let i = 0; i < n; i++) {
+    if (mode === 'async') {
+      // eslint-disable-next-line no-unused-vars
+      for await (const entry of await fs.promises.opendir(fullPath))
+        counter++;
+    } else if (mode === 'callback') {
+      const dir = await fs.promises.opendir(fullPath);
+      await new Promise((resolve, reject) => {
+        function read() {
+          dir.read((err, entry) => {
+            if (err) {
+              reject(err);
+            } else if (entry === null) {
+              resolve(dir.close());
+            } else {
+              counter++;
+              read();
+            }
+          });
+        }
+
+        read();
+      });
+    } else {
+      const dir = fs.opendirSync(fullPath);
+      while (dir.readSync() !== null)
+        counter++;
+      dir.closeSync();
+    }
+  }
+
+  bench.end(counter);
+}

doc/api/fs.md

@@ -362,8 +362,10 @@ Asynchronously read the next directory entry via readdir(3) as an
 After the read is completed, a `Promise` is returned that will be resolved with
 an [`fs.Dirent`][], or `null` if there are no more directory entries to read.
 
-_Directory entries returned by this function are in no particular order as
-provided by the operating system's underlying directory mechanisms._
+Directory entries returned by this function are in no particular order as
+provided by the operating system's underlying directory mechanisms.
+Entries added or removed while iterating over the directory may or may not be
+included in the iteration results.
 
 ### dir.read(callback)
 <!-- YAML
@@ -380,8 +382,10 @@ Asynchronously read the next directory entry via readdir(3) as an
 After the read is completed, the `callback` will be called with an
 [`fs.Dirent`][], or `null` if there are no more directory entries to read.
 
-_Directory entries returned by this function are in no particular order as
-provided by the operating system's underlying directory mechanisms._
+Directory entries returned by this function are in no particular order as
+provided by the operating system's underlying directory mechanisms.
+Entries added or removed while iterating over the directory may or may not be
+included in the iteration results.
 
 ### dir.readSync()
 <!-- YAML
@@ -395,8 +399,10 @@ Synchronously read the next directory entry via readdir(3) as an
 
 If there are no more directory entries to read, `null` will be returned.
 
-_Directory entries returned by this function are in no particular order as
-provided by the operating system's underlying directory mechanisms._
+Directory entries returned by this function are in no particular order as
+provided by the operating system's underlying directory mechanisms.
+Entries added or removed while iterating over the directory may or may not be
+included in the iteration results.
 
 ### dir\[Symbol.asyncIterator\]()
 <!-- YAML
@@ -413,8 +419,10 @@ The `null` case from `dir.read()` is handled internally.
 
 See [`fs.Dir`][] for an example.
 
-_Directory entries returned by this iterator are in no particular order as
-provided by the operating system's underlying directory mechanisms._
+Directory entries returned by this iterator are in no particular order as
+provided by the operating system's underlying directory mechanisms.
+Entries added or removed while iterating over the directory may or may not be
+included in the iteration results.
 
 ## Class: fs.Dirent
 <!-- YAML

lib/internal/fs/dir.js

@@ -24,23 +24,27 @@ const {
 
 const kDirHandle = Symbol('kDirHandle');
 const kDirPath = Symbol('kDirPath');
+const kDirBufferedEntries = Symbol('kDirBufferedEntries');
 const kDirClosed = Symbol('kDirClosed');
 const kDirOptions = Symbol('kDirOptions');
+const kDirReadImpl = Symbol('kDirReadImpl');
 const kDirReadPromisified = Symbol('kDirReadPromisified');
 const kDirClosePromisified = Symbol('kDirClosePromisified');
 
 class Dir {
   constructor(handle, path, options) {
     if (handle == null) throw new ERR_MISSING_ARGS('handle');
     this[kDirHandle] = handle;
+    this[kDirBufferedEntries] = [];
     this[kDirPath] = path;
     this[kDirClosed] = false;
 
     this[kDirOptions] = getOptions(options, {
       encoding: 'utf8'
     });
 
-    this[kDirReadPromisified] = internalUtil.promisify(this.read).bind(this);
+    this[kDirReadPromisified] =
+        internalUtil.promisify(this[kDirReadImpl]).bind(this, false);
     this[kDirClosePromisified] = internalUtil.promisify(this.close).bind(this);
   }
 
@@ -49,6 +53,10 @@ class Dir {
   }
 
   read(callback) {
+    return this[kDirReadImpl](true, callback);
+  }
+
+  [kDirReadImpl](maybeSync, callback) {
     if (this[kDirClosed] === true) {
       throw new ERR_DIR_CLOSED();
     }
@@ -59,11 +67,22 @@ class Dir {
       throw new ERR_INVALID_CALLBACK(callback);
     }
 
+    if (this[kDirBufferedEntries].length > 0) {
+      const [ name, type ] = this[kDirBufferedEntries].splice(0, 2);
+      if (maybeSync)
+        process.nextTick(getDirent, this[kDirPath], name, type, callback);
+      else
+        getDirent(this[kDirPath], name, type, callback);
+      return;
+    }
+
     const req = new FSReqCallback();
     req.oncomplete = (err, result) => {
       if (err || result === null) {
         return callback(err, result);
       }
+
+      this[kDirBufferedEntries] = result.slice(2);
       getDirent(this[kDirPath], result[0], result[1], callback);
     };
 
@@ -78,6 +97,11 @@ class Dir {
       throw new ERR_DIR_CLOSED();
     }
 
+    if (this[kDirBufferedEntries].length > 0) {
+      const [ name, type ] = this[kDirBufferedEntries].splice(0, 2);
+      return getDirent(this[kDirPath], name, type);
+    }
+
     const ctx = { path: this[kDirPath] };
     const result = this[kDirHandle].read(
       this[kDirOptions].encoding,
@@ -90,6 +114,7 @@ class Dir {
       return result;
     }
 
+    this[kDirBufferedEntries] = result.slice(2);
     return getDirent(this[kDirPath], result[0], result[1]);
   }
 

src/node_dir.cc

@@ -59,8 +59,8 @@ DirHandle::DirHandle(Environment* env, Local<Object> obj, uv_dir_t* dir)
       dir_(dir) {
   MakeWeak();
 
-  dir_->nentries = 1;
-  dir_->dirents = &dirent_;
+  dir_->nentries = arraysize(dirents_);
+  dir_->dirents = dirents_;
 }
 
 DirHandle* DirHandle::New(Environment* env, uv_dir_t* dir) {
@@ -160,7 +160,37 @@ void DirHandle::Close(const FunctionCallbackInfo<Value>& args) {
   }
 }
 
-void AfterDirReadSingle(uv_fs_t* req) {
+static MaybeLocal<Array> DirentListToArray(
+    Environment* env,
+    uv_dirent_t* ents,
+    int num,
+    enum encoding encoding,
+    Local<Value>* err_out) {
+  MaybeStackBuffer<Local<Value>, 96> entries(num * 3);
+
+  // Return an array of all read filenames.
+  int j = 0;
+  for (int i = 0; i < num; i++) {
+    Local<Value> filename;
+    Local<Value> error;
+    const size_t namelen = strlen(ents[i].name);
+    if (!StringBytes::Encode(env->isolate(),
+                             ents[i].name,
+                             namelen,
+                             encoding,
+                             &error).ToLocal(&filename)) {
+      *err_out = error;
+      return MaybeLocal<Array>();
+    }
+
+    entries[j++] = filename;
+    entries[j++] = Integer::New(env->isolate(), ents[i].type);
+  }
+
+  return Array::New(env->isolate(), entries.out(), j);
+}
+
+static void AfterDirRead(uv_fs_t* req) {
   FSReqBase* req_wrap = FSReqBase::from_req(req);
   FSReqAfterScope after(req_wrap, req);
 
@@ -170,7 +200,6 @@ void AfterDirReadSingle(uv_fs_t* req) {
 
   Environment* env = req_wrap->env();
   Isolate* isolate = env->isolate();
-  Local<Value> error;
 
   if (req->result == 0) {
     // Done
@@ -182,26 +211,17 @@ void AfterDirReadSingle(uv_fs_t* req) {
   uv_dir_t* dir = static_cast<uv_dir_t*>(req->ptr);
   req->ptr = nullptr;
 
-  // Single entries are returned without an array wrapper
-  const uv_dirent_t& ent = dir->dirents[0];
-
-  MaybeLocal<Value> filename =
-    StringBytes::Encode(isolate,
-                        ent.name,
-                        req_wrap->encoding(),
-                        &error);
-  if (filename.IsEmpty())
+  Local<Value> error;
+  Local<Array> js_array;
+  if (!DirentListToArray(env,
+                         dir->dirents,
+                         req->result,
+                         req_wrap->encoding(),
+                         &error).ToLocal(&js_array)) {
     return req_wrap->Reject(error);
+  }
 
-
-  Local<Array> result = Array::New(isolate, 2);
-  result->Set(env->context(),
-              0,
-              filename.ToLocalChecked()).FromJust();
-  result->Set(env->context(),
-              1,
-              Integer::New(isolate, ent.type)).FromJust();
-  req_wrap->Resolve(result);
+  req_wrap->Resolve(js_array);
 }
 
 
@@ -217,10 +237,10 @@ void DirHandle::Read(const FunctionCallbackInfo<Value>& args) {
   DirHandle* dir;
   ASSIGN_OR_RETURN_UNWRAP(&dir, args.Holder());
 
-  FSReqBase* req_wrap_async = static_cast<FSReqBase*>(GetReqWrap(env, args[1]));
+  FSReqBase* req_wrap_async = GetReqWrap(env, args[1]);
   if (req_wrap_async != nullptr) {  // dir.read(encoding, req)
     AsyncCall(env, req_wrap_async, args, "readdir", encoding,
-              AfterDirReadSingle, uv_fs_readdir, dir->dir());
+              AfterDirRead, uv_fs_readdir, dir->dir());
   } else {  // dir.read(encoding, undefined, ctx)
     CHECK_EQ(argc, 3);
     FSReqWrapSync req_wrap_sync;
@@ -240,28 +260,20 @@ void DirHandle::Read(const FunctionCallbackInfo<Value>& args) {
     }
 
     CHECK_GE(req_wrap_sync.req.result, 0);
-    const uv_dirent_t& ent = dir->dir()->dirents[0];
 
     Local<Value> error;
-    MaybeLocal<Value> filename =
-      StringBytes::Encode(isolate,
-                          ent.name,
-                          encoding,
-                          &error);
-    if (filename.IsEmpty()) {
+    Local<Array> js_array;
+    if (!DirentListToArray(env,
+                           dir->dir()->dirents,
+                           req_wrap_sync.req.result,
+                           encoding,
+                           &error).ToLocal(&js_array)) {
       Local<Object> ctx = args[2].As<Object>();
-      ctx->Set(env->context(), env->error_string(), error).FromJust();
+      USE(ctx->Set(env->context(), env->error_string(), error));
       return;
     }
 
-    Local<Array> result = Array::New(isolate, 2);
-    result->Set(env->context(),
-                0,
-                filename.ToLocalChecked()).FromJust();
-    result->Set(env->context(),
-                1,
-                Integer::New(isolate, ent.type)).FromJust();
-    args.GetReturnValue().Set(result);
+    args.GetReturnValue().Set(js_array);
   }
 }
 

src/node_dir.h

@@ -25,7 +25,6 @@ class DirHandle : public AsyncWrap {
   static void Close(const v8::FunctionCallbackInfo<Value>& args);
 
   inline uv_dir_t* dir() { return dir_; }
-  AsyncWrap* GetAsyncWrap() { return this; }
 
   void MemoryInfo(MemoryTracker* tracker) const override {
     tracker->TrackFieldWithSize("dir", sizeof(*dir_));
@@ -46,7 +45,8 @@ class DirHandle : public AsyncWrap {
   void GCClose();
 
   uv_dir_t* dir_;
-  uv_dirent_t dirent_;
+  // Up to 32 directory entries are read through a single libuv call.
+  uv_dirent_t dirents_[32];
   bool closing_ = false;
   bool closed_ = false;
 };

test/parallel/test-fs-opendir.js

@@ -58,17 +58,27 @@ const dirclosedError = {
 // Check the opendir async version
 fs.opendir(testDir, common.mustCall(function(err, dir) {
   assert.ifError(err);
-  dir.read(common.mustCall(function(err, dirent) {
+  let sync = true;
+  dir.read(common.mustCall((err, dirent) => {
+    assert(!sync);
     assert.ifError(err);
 
     // Order is operating / file system dependent
     assert(files.includes(dirent.name), `'files' should include ${dirent}`);
     assertDirent(dirent);
 
-    dir.close(common.mustCall(function(err) {
+    let syncInner = true;
+    dir.read(common.mustCall((err, dirent) => {
+      assert(!syncInner);
       assert.ifError(err);
+
+      dir.close(common.mustCall(function(err) {
+        assert.ifError(err);
+      }));
     }));
+    syncInner = false;
   }));
+  sync = false;
 }));
 
 // opendir() on file should throw ENOTDIR