Skip to content

Commit

Permalink
fs: add scandirSync(), rename internals to scandir
Browse files Browse the repository at this point in the history
This is a follow-up to
nodejs/node#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.
  • Loading branch information
Fishrock123 committed Nov 6, 2020
1 parent e6e6070 commit 35d1c50
Show file tree
Hide file tree
Showing 7 changed files with 30 additions and 26 deletions.
13 changes: 7 additions & 6 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -2061,8 +2061,8 @@ module.exports = fs = {
openSync,
opendir,
opendirSync,
readdir,
readdirSync,
readdir: scandir,
readdirSync: scandirSync,
read,
readSync,
readv,
Expand All @@ -2079,6 +2079,7 @@ module.exports = fs = {
rmSync,
rmdir,
rmdirSync,
scandirSync,
stat,
statSync,
symlink,
Expand Down
6 changes: 3 additions & 3 deletions lib/internal/fs/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -646,7 +646,7 @@ module.exports = {
rm,
rmdir,
mkdir,
readdir,
readdir: scandir,
readlink,
symlink,
lstat,
Expand Down
14 changes: 7 additions & 7 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1613,7 +1613,7 @@ static void RealPath(const FunctionCallbackInfo<Value>& args) {
}
}

static void ReadDir(const FunctionCallbackInfo<Value>& args) {
static void ScanDir(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
Isolate* isolate = env->isolate();

Expand All @@ -1628,21 +1628,21 @@ static void ReadDir(const FunctionCallbackInfo<Value>& 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*/);
} else {
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
}
Expand All @@ -1663,7 +1663,7 @@ static void ReadDir(const FunctionCallbackInfo<Value>& 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;
}

Expand Down Expand Up @@ -2416,7 +2416,7 @@ void Initialize(Local<Object> 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);
Expand Down
10 changes: 5 additions & 5 deletions test/parallel/test-fs-readdir-types.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand All @@ -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;
}
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-repl-underscore.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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',",
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-trace-events-fs-sync.js
Original file line number Diff line number Diff line change
Expand Up @@ -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");' +
Expand Down
7 changes: 5 additions & 2 deletions tools/build-addons.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit 35d1c50

Please sign in to comment.