From 35d1c503cb5601971c07f9ed1e84c173fef1707e Mon Sep 17 00:00:00 2001 From: Jeremiah Senkpiel Date: Fri, 6 Nov 2020 13:03:09 -0800 Subject: [PATCH] fs: add `scandirSync()`, rename internals to scandir This is a follow-up to https://github.com/nodejs/node/pull/29349 and a precursor to deprecating both `fs.readdir()` and `fs.readdirSync()`. This also updates the documentation which has been incorrect for some 10 years saying that these did `readdir(3)` when in reality they do `scandir(3)`. Only `scandirSync()` is introduced as "async scandir(3)" is kind of a trap, given that it returns the whole list of entries at once, regardless of how many there are. Since in many cases we'd also want to get dirents for them (i.e. `stat`-ing each and every one), this becomes a serious problem, and Node.js should encourage users to use `fs.opendir()`, which is slightly more complex but far better. --- lib/fs.js | 13 +++++++------ lib/internal/fs/promises.js | 6 +++--- src/node_file.cc | 14 +++++++------- test/parallel/test-fs-readdir-types.js | 10 +++++----- test/parallel/test-repl-underscore.js | 4 ++-- test/parallel/test-trace-events-fs-sync.js | 2 +- tools/build-addons.js | 7 +++++-- 7 files changed, 30 insertions(+), 26 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index abd12e13190..3263967352e 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -994,7 +994,7 @@ function mkdirSync(path, options) { } } -function readdir(path, options, callback) { +function scandir(path, options, callback) { callback = makeCallback(typeof options === 'function' ? options : callback); options = getOptions(options, {}); path = getValidatedPath(path); @@ -1011,15 +1011,15 @@ function readdir(path, options, callback) { getDirents(path, result, callback); }; } - binding.readdir(pathModule.toNamespacedPath(path), options.encoding, + binding.scandir(pathModule.toNamespacedPath(path), options.encoding, !!options.withFileTypes, req); } -function readdirSync(path, options) { +function scandirSync(path, options) { options = getOptions(options, {}); path = getValidatedPath(path); const ctx = { path }; - const result = binding.readdir(pathModule.toNamespacedPath(path), + const result = binding.scandir(pathModule.toNamespacedPath(path), options.encoding, !!options.withFileTypes, undefined, ctx); handleErrorFromBinding(ctx); @@ -2061,8 +2061,8 @@ module.exports = fs = { openSync, opendir, opendirSync, - readdir, - readdirSync, + readdir: scandir, + readdirSync: scandirSync, read, readSync, readv, @@ -2079,6 +2079,7 @@ module.exports = fs = { rmSync, rmdir, rmdirSync, + scandirSync, stat, statSync, symlink, diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 497605aaefa..e844afdc624 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -461,10 +461,10 @@ async function mkdir(path, options) { kUsePromises); } -async function readdir(path, options) { +async function scandir(path, options) { options = getOptions(options, {}); path = getValidatedPath(path); - const result = await binding.readdir(pathModule.toNamespacedPath(path), + const result = await binding.scandir(pathModule.toNamespacedPath(path), options.encoding, !!options.withFileTypes, kUsePromises); @@ -646,7 +646,7 @@ module.exports = { rm, rmdir, mkdir, - readdir, + readdir: scandir, readlink, symlink, lstat, diff --git a/src/node_file.cc b/src/node_file.cc index de5c455c7a2..c2c59ea98db 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1613,7 +1613,7 @@ static void RealPath(const FunctionCallbackInfo& args) { } } -static void ReadDir(const FunctionCallbackInfo& args) { +static void ScanDir(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Isolate* isolate = env->isolate(); @@ -1628,7 +1628,7 @@ static void ReadDir(const FunctionCallbackInfo& args) { bool with_types = args[2]->IsTrue(); FSReqBase* req_wrap_async = GetReqWrap(args, 3); - if (req_wrap_async != nullptr) { // readdir(path, encoding, withTypes, req) + if (req_wrap_async != nullptr) { // scandir(path, encoding, withTypes, req) if (with_types) { AsyncCall(env, req_wrap_async, args, "scandir", encoding, AfterScanDirWithTypes, uv_fs_scandir, *path, 0 /*flags*/); @@ -1636,13 +1636,13 @@ static void ReadDir(const FunctionCallbackInfo& args) { AsyncCall(env, req_wrap_async, args, "scandir", encoding, AfterScanDir, uv_fs_scandir, *path, 0 /*flags*/); } - } else { // readdir(path, encoding, withTypes, undefined, ctx) + } else { // scandir(path, encoding, withTypes, undefined, ctx) CHECK_EQ(argc, 5); FSReqWrapSync req_wrap_sync; - FS_SYNC_TRACE_BEGIN(readdir); + FS_SYNC_TRACE_BEGIN(scandir); int err = SyncCall(env, args[4], &req_wrap_sync, "scandir", uv_fs_scandir, *path, 0 /*flags*/); - FS_SYNC_TRACE_END(readdir); + FS_SYNC_TRACE_END(scandir); if (err < 0) { return; // syscall failed, no need to continue, error info is in ctx } @@ -1663,7 +1663,7 @@ static void ReadDir(const FunctionCallbackInfo& args) { ctx->Set(env->context(), env->errno_string(), Integer::New(isolate, r)).Check(); ctx->Set(env->context(), env->syscall_string(), - OneByteString(isolate, "readdir")).Check(); + OneByteString(isolate, "scandir")).Check(); return; } @@ -2416,7 +2416,7 @@ void Initialize(Local target, env->SetMethod(target, "ftruncate", FTruncate); env->SetMethod(target, "rmdir", RMDir); env->SetMethod(target, "mkdir", MKDir); - env->SetMethod(target, "readdir", ReadDir); + env->SetMethod(target, "scandir", ScanDir); env->SetMethod(target, "internalModuleReadJSON", InternalModuleReadJSON); env->SetMethod(target, "internalModuleStat", InternalModuleStat); env->SetMethod(target, "stat", Stat); diff --git a/test/parallel/test-fs-readdir-types.js b/test/parallel/test-fs-readdir-types.js index 9116a04f44e..2a1358a06fc 100644 --- a/test/parallel/test-fs-readdir-types.js +++ b/test/parallel/test-fs-readdir-types.js @@ -80,9 +80,9 @@ fs.readdir(readdirDir, { // Check for correct types when the binding returns unknowns const UNKNOWN = constants.UV_DIRENT_UNKNOWN; -const oldReaddir = binding.readdir; -process.on('beforeExit', () => { binding.readdir = oldReaddir; }); -binding.readdir = common.mustCall((path, encoding, types, req, ctx) => { +const oldScandir = binding.scandir; +process.on('beforeExit', () => { binding.scandir = oldScandir; }); +binding.scandir = common.mustCall((path, encoding, types, req, ctx) => { if (req) { const oldCb = req.oncomplete; req.oncomplete = (err, results) => { @@ -93,9 +93,9 @@ binding.readdir = common.mustCall((path, encoding, types, req, ctx) => { results[1] = results[1].map(() => UNKNOWN); oldCb(null, results); }; - oldReaddir(path, encoding, types, req); + oldScandir(path, encoding, types, req); } else { - const results = oldReaddir(path, encoding, types, req, ctx); + const results = oldScandir(path, encoding, types, req, ctx); results[1] = results[1].map(() => UNKNOWN); return results; } diff --git a/test/parallel/test-repl-underscore.js b/test/parallel/test-repl-underscore.js index c05c387471b..bbd61921271 100644 --- a/test/parallel/test-repl-underscore.js +++ b/test/parallel/test-repl-underscore.js @@ -160,7 +160,7 @@ function testError() { r.write(`_error; // initial value undefined throw new Error('foo'); // throws error _error; // shows error - fs.readdirSync('/nonexistent?'); // throws error, sync + fs.scandirSync('/nonexistent?'); // throws error, sync _error.code; // shows error code _error.syscall; // shows error syscall setImmediate(() => { throw new Error('baz'); }); undefined; @@ -178,7 +178,7 @@ function testError() { // The sync error, with individual property echoes /^Uncaught Error: ENOENT: no such file or directory, scandir '.*nonexistent\?'/, - /Object\.readdirSync/, + /Object\.scandirSync/, /^ errno: -(2|4058),$/, " syscall: 'scandir',", " code: 'ENOENT',", diff --git a/test/parallel/test-trace-events-fs-sync.js b/test/parallel/test-trace-events-fs-sync.js index d8e9ca30a8e..763b88bfa6d 100644 --- a/test/parallel/test-trace-events-fs-sync.js +++ b/test/parallel/test-trace-events-fs-sync.js @@ -76,7 +76,7 @@ tests['fs.sync.open'] = 'fs.writeFileSync("fs.txt", "123", "utf8");' + tests['fs.sync.read'] = 'fs.writeFileSync("fs.txt", "123", "utf8");' + 'fs.readFileSync("fs.txt");' + 'fs.unlinkSync("fs.txt")'; -tests['fs.sync.readdir'] = 'fs.readdirSync("./")'; +tests['fs.sync.scandir'] = 'fs.scandirSync("./")'; tests['fs.sync.realpath'] = 'fs.writeFileSync("fs.txt", "123", "utf8");' + 'fs.linkSync("fs.txt", "linkx");' + 'fs.realpathSync.native("linkx");' + diff --git a/tools/build-addons.js b/tools/build-addons.js index 1d4bcbc9179..4adb21ba41a 100644 --- a/tools/build-addons.js +++ b/tools/build-addons.js @@ -46,8 +46,11 @@ async function runner(directoryQueue) { } async function main(directory) { - const directoryQueue = (await fs.readdir(directory)) - .map((subdir) => path.join(directory, subdir)); + const dir = (await fs.opendir(directory)); + const directoryQueue = []; + for await (let subdir of dir) { + directoryQueue.push(path.join(directory, subdir.name)); + } const runners = []; for (let i = 0; i < parallelization; ++i)