From 35d1c503cb5601971c07f9ed1e84c173fef1707e Mon Sep 17 00:00:00 2001 From: Jeremiah Senkpiel Date: Fri, 6 Nov 2020 13:03:09 -0800 Subject: [PATCH 1/2] 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 abd12e131904c8..3263967352e74c 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 497605aaefa72a..e844afdc624023 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 de5c455c7a2a85..c2c59ea98db0b7 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 9116a04f44ed70..2a1358a06fc8bc 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 c05c387471b59f..bbd61921271c7b 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 d8e9ca30a8ebd3..763b88bfa6d5d4 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 1d4bcbc917972c..4adb21ba41abf7 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) From 792b6c4dd0422a48469de6c90310fd0ad36c121b Mon Sep 17 00:00:00 2001 From: Jeremiah Senkpiel Date: Fri, 6 Nov 2020 13:06:17 -0800 Subject: [PATCH 2/2] fs,doc: docs-deprecate `fs.readdir{Sync}()` It's time overdue for these to start going away. `fs.opendir()` was introduced over a year ago in https://github.com/nodejs/node/pull/29349 - which stated a follow-up of: "Aliasing fs.readdir to fs.scandir and doing a deprecation." This provides the intial docs deprecation to both `fs.readdir()` and `fs.readdirSync()`, both of which are misnamed, and the former of which is a trap as it is not fully async. --- doc/api/deprecations.md | 36 ++++++++++++++++++++++++++++++++++++ doc/api/fs.md | 32 ++++++++++++++++++++++++++++++-- 2 files changed, 66 insertions(+), 2 deletions(-) diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index 41ad56bc36b17c..70ae66c0a6ee60 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -2699,6 +2699,37 @@ resolutions not in `node_modules`. This means there will not be deprecation warnings for `"exports"` in dependencies. With `--pending-deprecation`, a runtime warning results no matter where the `"exports"` usage occurs. +### DEP0147: `fs.readdir()` + + +Type: Documentation-only + +[`fs.readdir()`][] is deprecated. Use [`fsPromises.opendir()`][] instead, +where possible, otherwise use [`fs.opendir()`][]. +`fs.readdir()` is not fully asynchronous. It is asynchronous while gathering +the list of directory entries but then sends them to JavaScript as one large +array. In cases where this array contains hndreds of entries, particularly if +`withFileTypes` is enabled, this can be quite slow and cause performance +degredations. + +### DEP0148: `fs.readdirSync()` + + +Type: Documentation-only + +[`fs.readdirSync()`][] is deprecated. Use [`fs.scandirSync()`][] instead. +`fs.readdirSync()` was always mis-named. + [Legacy URL API]: url.md#url_legacy_url_api [NIST SP 800-38D]: https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf [RFC 6066]: https://tools.ietf.org/html/rfc6066#section-3 @@ -2750,9 +2781,14 @@ runtime warning results no matter where the `"exports"` usage occurs. [`fs.lchmodSync(path, mode)`]: fs.md#fs_fs_lchmodsync_path_mode [`fs.lchown(path, uid, gid, callback)`]: fs.md#fs_fs_lchown_path_uid_gid_callback [`fs.lchownSync(path, uid, gid)`]: fs.md#fs_fs_lchownsync_path_uid_gid +[`fs.opendir()`]: fs.html#fs_fs_opendir_path_options_callback [`fs.read()`]: fs.md#fs_fs_read_fd_buffer_offset_length_position_callback [`fs.readSync()`]: fs.md#fs_fs_readsync_fd_buffer_offset_length_position +[`fs.readdir()`]: fs.html#fs_fs_readdir_path_options_callback +[`fs.readdirSync()`]: fs.html#fs_fs_readdirsync_path_options +[`fs.scandirSync()`]: fs.html#fs_fs_scandirsync_path_options [`fs.stat()`]: fs.md#fs_fs_stat_path_options_callback +[`fsPromises.opendir()`]: fs.html#fs_fspromises_opendir_path_options [`http.get()`]: http.md#http_http_get_options_callback [`http.request()`]: http.md#http_http_request_options_callback [`https.get()`]: https.md#https_https_get_options_callback diff --git a/doc/api/fs.md b/doc/api/fs.md index 24768ac17f97e3..dfed512f2115cb 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -2957,6 +2957,7 @@ If no `options` object is specified, it will default with the above values. ## `fs.readdir(path[, options], callback)` +> Stability: 0 - Deprecated. Use [`fsPromises.opendir()`][] instead, +where possible, otherwise use [`fs.opendir()`][]. + * `path` {string|Buffer|URL} * `options` {string|Object} * `encoding` {string} **Default:** `'utf8'` @@ -2986,7 +2990,7 @@ changes: * `err` {Error} * `files` {string[]|Buffer[]|fs.Dirent[]} -Asynchronous readdir(3). Reads the contents of a directory. +Asynchronous scandir(3). Reads the contents of a directory. The callback gets two arguments `(err, files)` where `files` is an array of the names of the files in the directory excluding `'.'` and `'..'`. @@ -3011,13 +3015,15 @@ changes: protocol. Support is currently still *experimental*. --> +> Stability: 0 - Deprecated. Use [`fs.scandirSync()`][] instead. + * `path` {string|Buffer|URL} * `options` {string|Object} * `encoding` {string} **Default:** `'utf8'` * `withFileTypes` {boolean} **Default:** `false` * Returns: {string[]|Buffer[]|fs.Dirent[]} -Synchronous readdir(3). +Synchronous scandir(3). The optional `options` argument can be a string specifying an encoding, or an object with an `encoding` property specifying the character encoding to use for @@ -3654,6 +3660,27 @@ added: v14.14.0 Synchronously removes files and directories (modeled on the standard POSIX `rm` utility). Returns `undefined`. +## `fs.scandirSync(path[, options])` + + +* `path` {string|Buffer|URL} +* `options` {string|Object} + * `encoding` {string} **Default:** `'utf8'` + * `withFileTypes` {boolean} **Default:** `false` +* Returns: {string[]|Buffer[]|fs.Dirent[]} + +Synchronous scandir(3). + +The optional `options` argument can be a string specifying an encoding, or an +object with an `encoding` property specifying the character encoding to use for +the filenames returned. If the `encoding` is set to `'buffer'`, +the filenames returned will be passed as `Buffer` objects. + +If `options.withFileTypes` is set to `true`, the result will contain +[`fs.Dirent`][] objects. + ## `fs.stat(path[, options], callback)`